| Message ID | 1369861743.4188.33.camel@buesod1.americas.hpqcorp.net |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 05/30/2013 01:09 AM, Davidlohr Bueso wrote: > Use the already defined macro to pass the function return address. > > Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com> > --- > net/core/skbuff.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index af9185d..0d06850 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -116,22 +116,22 @@ static const struct pipe_buf_operations sock_pipe_buf_ops = { > * Keep out of line to prevent kernel bloat. > * __builtin_return_address is not used because it is not always reliable. > */ > -static void skb_panic(struct sk_buff *skb, unsigned int sz, void *addr, > +static void skb_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr, > const char msg[]) > { > - pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n", > + pr_emerg("%s: text:0x%lx len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n", Why not "text:%#lx" as already used in this string? It's equivalent to "0x%lx". WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: > Why not "text:%#lx" as already used in this string? It's > equivalent to "0x%lx". Well, I don't know the reasoning in this case, but I'd like to note that those are not strictly equivalent. Personally I find the formatting of 0 annoying enough to avoid %#x for any value which may be 0. It's especially bad if you try to line up things by adding leading zeros. I would expect these to produce the same result, but they don't: printf("0x%02hhx\n", 0); printf("%#04hhx\n", 0); Ending up with a 4 digit output when you expect 2 is very confusing. It doesn't matter that 0 is 0 in any case. Why doesn't the same happen to 1 then? This is just inconsistent behaviour, and I see no valid excuse for it. IMHO the single format character saved isn't worth this at all. I'll continue using 0x%x Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote: > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: > > > Why not "text:%#lx" as already used in this string? It's > > equivalent to "0x%lx". > > Well, I don't know the reasoning in this case, but I'd like to note that > those are not strictly equivalent. Personally I find the formatting of 0 > annoying enough to avoid %#x for any value which may be 0. It's > especially bad if you try to line up things by adding leading zeros. Yep, I found that 0x%lx produced the same output as %p. If you'd rather I use something else instead, I'm happy to update the patch. Thanks, Davidlohr -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-30 at 18:11 -0700, Davidlohr Bueso wrote: > On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote: > > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: > > > > > Why not "text:%#lx" as already used in this string? It's > > > equivalent to "0x%lx". > > > > Well, I don't know the reasoning in this case, but I'd like to note that > > those are not strictly equivalent. Personally I find the formatting of 0 > > annoying enough to avoid %#x for any value which may be 0. It's > > especially bad if you try to line up things by adding leading zeros. > > Yep, I found that 0x%lx produced the same output as %p. Don't use a standalone gcc compiled program to determine what the kernel outputs. lib/vsprintf.c does not output the same. (32 bit) The kernel output is; printk("0x%lx\n", 0x100ul) 0x100 printk("%p\n", (void *)0x100ul) 00000100 printk("%#p\n", (void *)0x100ul) 0x00000100 The last one isn't used at all in kernel source. (gcc complains) It's always "0x%p" -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> writes: > On Thu, 2013-05-30 at 18:11 -0700, Davidlohr Bueso wrote: >> On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote: >> > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: >> > >> > > Why not "text:%#lx" as already used in this string? It's >> > > equivalent to "0x%lx". >> > >> > Well, I don't know the reasoning in this case, but I'd like to note that >> > those are not strictly equivalent. Personally I find the formatting of 0 >> > annoying enough to avoid %#x for any value which may be 0. It's >> > especially bad if you try to line up things by adding leading zeros. >> >> Yep, I found that 0x%lx produced the same output as %p. > > Don't use a standalone gcc compiled program to > determine what the kernel outputs. That's a very good point. Sorry for mixing this up. You are of course right. The kernel does everything correctly, so there is no reason not to use %#x Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 31-05-2013 9:20, Joe Perches wrote: >>>> Why not "text:%#lx" as already used in this string? It's >>>> equivalent to "0x%lx". >>> Well, I don't know the reasoning in this case, but I'd like to note that >>> those are not strictly equivalent. Personally I find the formatting of 0 >>> annoying enough to avoid %#x for any value which may be 0. It's >>> especially bad if you try to line up things by adding leading zeros. >> Yep, I found that 0x%lx produced the same output as %p. > Don't use a standalone gcc compiled program to > determine what the kernel outputs. > lib/vsprintf.c does not output the same. (32 bit) > The kernel output is; > printk("0x%lx\n", 0x100ul) 0x100 > printk("%p\n", (void *)0x100ul) 00000100 > printk("%#p\n", (void *)0x100ul) 0x00000100 > The last one isn't used at all in kernel source. (gcc complains) > It's always "0x%p" I was talking about using "%#lx", not "%#p". I don't see it in your example. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-05-31 at 18:33 +0400, Sergei Shtylyov wrote: > On 31-05-2013 9:20, Joe Perches wrote: > > Don't use a standalone gcc compiled program to > > determine what the kernel outputs. [] > > The kernel output is; > > > printk("0x%lx\n", 0x100ul) 0x100 > > printk("%p\n", (void *)0x100ul) 00000100 > > printk("%#p\n", (void *)0x100ul) 0x00000100 > > > The last one isn't used at all in kernel source. (gcc complains) > > It's always "0x%p" > > I was talking about using "%#lx", not "%#p". I don't see it in your > example. "0x%lx" and "%#lx" produce the same output in the kernel. The latter isn't used very often though. I expect most coders don't know it exists/works. $ git grep -E "0x%l{0,2}x" | wc -l 12542 $ git grep -E "%#l{0,2}x" | wc -l 1737 (some false positives there of course) Also, some might expect that "%#08lx", is 10 chars wide, but it's only 8, so maybe "0x%08lx" is better used. The "%#08lx" width defect seems pretty common: $ git grep -E -i "%#0(8|16)l{0,2}x" | wc -l 253 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 05/31/2013 08:54 PM, Joe Perches wrote: >>> Don't use a standalone gcc compiled program to >>> determine what the kernel outputs. > [] >>> The kernel output is; >>> printk("0x%lx\n", 0x100ul) 0x100 >>> printk("%p\n", (void *)0x100ul) 00000100 >>> printk("%#p\n", (void *)0x100ul) 0x00000100 >>> The last one isn't used at all in kernel source. (gcc complains) >>> It's always "0x%p" >> I was talking about using "%#lx", not "%#p". I don't see it in your >> example. > "0x%lx" and "%#lx" produce the same output in the kernel. > > The latter isn't used very often though. It's already used in the same format string in this case. That's why I suggested not to deviate from the existing code. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Davidlohr Bueso <davidlohr.bueso@hp.com> Date: Wed, 29 May 2013 14:09:03 -0700 > Use the already defined macro to pass the function return address. > > Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com> I've decided that we should just leave this alone for now, because frankly the choice is arbitrary. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index af9185d..0d06850 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -116,22 +116,22 @@ static const struct pipe_buf_operations sock_pipe_buf_ops = { * Keep out of line to prevent kernel bloat. * __builtin_return_address is not used because it is not always reliable. */ -static void skb_panic(struct sk_buff *skb, unsigned int sz, void *addr, +static void skb_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr, const char msg[]) { - pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n", + pr_emerg("%s: text:0x%lx len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n", msg, addr, skb->len, sz, skb->head, skb->data, (unsigned long)skb->tail, (unsigned long)skb->end, skb->dev ? skb->dev->name : "<NULL>"); BUG(); } -static void skb_over_panic(struct sk_buff *skb, unsigned int sz, void *addr) +static void skb_over_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr) { skb_panic(skb, sz, addr, __func__); } -static void skb_under_panic(struct sk_buff *skb, unsigned int sz, void *addr) +static void skb_under_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr) { skb_panic(skb, sz, addr, __func__); } @@ -646,7 +646,7 @@ void kfree_skb(struct sk_buff *skb) smp_rmb(); else if (likely(!atomic_dec_and_test(&skb->users))) return; - trace_kfree_skb(skb, __builtin_return_address(0)); + trace_kfree_skb(skb, (void *) _RET_IP_); __kfree_skb(skb); } EXPORT_SYMBOL(kfree_skb); @@ -1279,7 +1279,7 @@ unsigned char *skb_put(struct sk_buff *skb, unsigned int len) skb->tail += len; skb->len += len; if (unlikely(skb->tail > skb->end)) - skb_over_panic(skb, len, __builtin_return_address(0)); + skb_over_panic(skb, len, _RET_IP_); return tmp; } EXPORT_SYMBOL(skb_put); @@ -1298,7 +1298,7 @@ unsigned char *skb_push(struct sk_buff *skb, unsigned int len) skb->data -= len; skb->len += len; if (unlikely(skb->data<skb->head)) - skb_under_panic(skb, len, __builtin_return_address(0)); + skb_under_panic(skb, len, _RET_IP_); return skb->data; } EXPORT_SYMBOL(skb_push);
Use the already defined macro to pass the function return address. Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com> --- net/core/skbuff.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)