* [PATCH] printk: Let %pR handle NULL pointers
@ 2009-01-03 10:42 Trent Piepho
2009-01-03 17:10 ` Linus Torvalds
0 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2009-01-03 10:42 UTC (permalink / raw)
To: linux-kernel; +Cc: torvalds, Trent Piepho
Have %pR print "[NULL]" for the resource range when passed a NULL pointer.
Trying to print a NULL pointer with %pR crashes, though printing a NULL
pointer with %p works fine. It isn't very helpful to put in a dev_dbg() to
print a resource and have the kernel crash because sometimes the resource
can be NULL.
Printing "[NULL]" is more useful than crashing when the resource isn't
supposed to be NULL and simplifies code in cases where one wants to print a
resource range than is allowed to be NULL.
Signed-off-by: Trent Piepho <xyzzy@speakeasy.org>
---
lib/vsprintf.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b77702..2879a1b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -566,6 +566,9 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
char *p = sym, *pend = sym + sizeof(sym);
int size = -1;
+ if (!res)
+ return string(buf, end, "[NULL]", field_width, precision, flags);
+
if (res->flags & IORESOURCE_IO)
size = IO_RSRC_PRINTK_SIZE;
else if (res->flags & IORESOURCE_MEM)
--
1.5.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-03 10:42 [PATCH] printk: Let %pR handle NULL pointers Trent Piepho
@ 2009-01-03 17:10 ` Linus Torvalds
2009-01-03 19:33 ` Harvey Harrison
2009-01-03 21:53 ` Benjamin Herrenschmidt
0 siblings, 2 replies; 9+ messages in thread
From: Linus Torvalds @ 2009-01-03 17:10 UTC (permalink / raw)
To: Trent Piepho
Cc: Linux Kernel Mailing List, Benjamin Herrenschmidt,
Harvey Harrison, David S. Miller
On Sat, 3 Jan 2009, Trent Piepho wrote:
>
> Have %pR print "[NULL]" for the resource range when passed a NULL pointer.
Wouldn't it be much nicer to just do it for _all_ pointer types?
IOW, a patch more like the appended.
Also, I'm not 100% sure that "[NULL]" is the right thing to print. Not
that I know if there's anything better. Testing glibc, it prints "(nil)"
for a NULL string (%s) and "(null)" for a NULL pointer (%p). Which makes
no more sense than anything else, but maybe we could make the NULL %p case
at least match that if for no other reason than the fact that it would
match _something_.
Added the other people who added %pX modifiers to the cc - I guess the
networking people probably never have NULL pointers there anyway, but
maybe they have opinions.
Linus
---
lib/vsprintf.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b77702..4df1884 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -661,6 +661,9 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width,
*/
static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags)
{
+ if (!ptr)
+ return string(buf, end, "[NULL]", field_width, precision, flags);
+
switch (*fmt) {
case 'F':
ptr = dereference_function_descriptor(ptr);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-03 17:10 ` Linus Torvalds
@ 2009-01-03 19:33 ` Harvey Harrison
2009-01-03 21:53 ` Benjamin Herrenschmidt
1 sibling, 0 replies; 9+ messages in thread
From: Harvey Harrison @ 2009-01-03 19:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Trent Piepho, Linux Kernel Mailing List, Benjamin Herrenschmidt,
David S. Miller
On Sat, 2009-01-03 at 09:10 -0800, Linus Torvalds wrote:
>
> On Sat, 3 Jan 2009, Trent Piepho wrote:
> >
> > Have %pR print "[NULL]" for the resource range when passed a NULL pointer.
>
> Wouldn't it be much nicer to just do it for _all_ pointer types?
>
> IOW, a patch more like the appended.
>
> Also, I'm not 100% sure that "[NULL]" is the right thing to print. Not
> that I know if there's anything better. Testing glibc, it prints "(nil)"
> for a NULL string (%s) and "(null)" for a NULL pointer (%p). Which makes
> no more sense than anything else, but maybe we could make the NULL %p case
> at least match that if for no other reason than the fact that it would
> match _something_.
>
> Added the other people who added %pX modifiers to the cc - I guess the
> networking people probably never have NULL pointers there anyway, but
> maybe they have opinions.
I'm of two minds here, the only reason I didn't add NULL handling to %pI[46] and
%pM is because they were previously printing values rather than pointers and
was guaranteed to never be null. Also, with the current values, the output
is always a fixed-width for %pI6, %pM. %pI4 can vary from 7-15 chars, but
[NULL] is only 6 chars (and obviously has a totally different format).
Of course it's hard to see how a caller could screw that up...but since I
didn't check everybody using it, I didn't risk it at the time.
On the other hand, I'm not sure if we want each %p formatting to have to deal
with its own null-formatting.
I'll do an audit of a handful of pI46, pM users currently, and try to find places were
a short string/completely different format string could cause problems...but
your patch is fine for today as all of the existing users [cs]houldn't be passing
in null anyways.
Harvey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-03 17:10 ` Linus Torvalds
2009-01-03 19:33 ` Harvey Harrison
@ 2009-01-03 21:53 ` Benjamin Herrenschmidt
2009-01-04 5:02 ` Trent Piepho
1 sibling, 1 reply; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2009-01-03 21:53 UTC (permalink / raw)
To: Linus Torvalds
Cc: Trent Piepho, Linux Kernel Mailing List, Harvey Harrison,
David S. Miller
On Sat, 2009-01-03 at 09:10 -0800, Linus Torvalds wrote:
>
> On Sat, 3 Jan 2009, Trent Piepho wrote:
> >
> > Have %pR print "[NULL]" for the resource range when passed a NULL pointer.
>
> Wouldn't it be much nicer to just do it for _all_ pointer types?
>
> IOW, a patch more like the appended.
Agreed.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-03 21:53 ` Benjamin Herrenschmidt
@ 2009-01-04 5:02 ` Trent Piepho
2009-01-04 5:14 ` Valdis.Kletnieks
2009-01-04 9:48 ` Benjamin Herrenschmidt
0 siblings, 2 replies; 9+ messages in thread
From: Trent Piepho @ 2009-01-04 5:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Linus Torvalds, Linux Kernel Mailing List, Harvey Harrison,
David S. Miller
On Sun, 4 Jan 2009, Benjamin Herrenschmidt wrote:
> On Sat, 2009-01-03 at 09:10 -0800, Linus Torvalds wrote:
> >
> > On Sat, 3 Jan 2009, Trent Piepho wrote:
> > >
> > > Have %pR print "[NULL]" for the resource range when passed a NULL pointer.
> >
> > Wouldn't it be much nicer to just do it for _all_ pointer types?
> >
> > IOW, a patch more like the appended.
>
> Agreed.
I thought of doing that too, but then all the various %pX formats would all
have to print the same for NULL pointers. For instance a resource prints
out like "[0x1000-0x100f]", so I chose "[NULL]" for printing a null
resource pointer. Maybe "[]" or "[-]" would be better? A null MAC address
could be ":::::" or "x:x:x:x:x:x". "N.U.L.L" or "x.x.x.x" for a null IP4
address. And so on. So the printout looks nicer when a NULL pointer isn't
a bug.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-04 5:02 ` Trent Piepho
@ 2009-01-04 5:14 ` Valdis.Kletnieks
2009-01-04 9:31 ` Johannes Berg
2009-01-04 9:48 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 9+ messages in thread
From: Valdis.Kletnieks @ 2009-01-04 5:14 UTC (permalink / raw)
To: Trent Piepho
Cc: Benjamin Herrenschmidt, Linus Torvalds,
Linux Kernel Mailing List, Harvey Harrison, David S. Miller,
netdev
[-- Attachment #1: Type: text/plain, Size: 841 bytes --]
On Sat, 03 Jan 2009 21:02:15 PST, Trent Piepho said:
> resource pointer. Maybe "[]" or "[-]" would be better? A null MAC address
> could be ":::::" or "x:x:x:x:x:x". "N.U.L.L" or "x.x.x.x" for a null IP4
> address. And so on. So the printout looks nicer when a NULL pointer isn't
> a bug.
Have to be careful for IPv6 addresses - '::' is a legal representation of
an all-zeros address. Also, N.U.L.L. may give indigestion to logfile parsers
that are expecting a numeric value in the IP address. It's however unclear
whether we should pick something that doesn't have 3 periods in it, so it
can't match, or whether *that* will give regexp-based logfile readers
indigestion when they don't pick up an IP address where they expected...
Do we want the %pI6 format to do the multiple-zeros -> :: compression?
(Adding netdev to cc: list)
[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-04 5:14 ` Valdis.Kletnieks
@ 2009-01-04 9:31 ` Johannes Berg
0 siblings, 0 replies; 9+ messages in thread
From: Johannes Berg @ 2009-01-04 9:31 UTC (permalink / raw)
To: Valdis.Kletnieks
Cc: Trent Piepho, Benjamin Herrenschmidt, Linus Torvalds,
Linux Kernel Mailing List, Harvey Harrison, David S. Miller,
netdev
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
On Sun, 2009-01-04 at 00:14 -0500, Valdis.Kletnieks@vt.edu wrote:
> On Sat, 03 Jan 2009 21:02:15 PST, Trent Piepho said:
> > resource pointer. Maybe "[]" or "[-]" would be better? A null MAC address
> > could be ":::::" or "x:x:x:x:x:x". "N.U.L.L" or "x.x.x.x" for a null IP4
> > address. And so on. So the printout looks nicer when a NULL pointer isn't
> > a bug.
>
> Have to be careful for IPv6 addresses - '::' is a legal representation of
> an all-zeros address. Also, N.U.L.L. may give indigestion to logfile parsers
> that are expecting a numeric value in the IP address. It's however unclear
> whether we should pick something that doesn't have 3 periods in it, so it
> can't match, or whether *that* will give regexp-based logfile readers
> indigestion when they don't pick up an IP address where they expected...
>
> Do we want the %pI6 format to do the multiple-zeros -> :: compression?
Eh, why ever care? We aren't passing NULL pointers in, hopefully, or the
code was crashing, at least before the conversion... So as long as we're
careful adding new users, I don't see a problem with not handling NULL.
FWIW, I'd think for MAC addresses there are multiple possible
interpretations, some wireless code assumes broadcast for NULL, and I'm
sure there are others like all-zeroes.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-04 5:02 ` Trent Piepho
2009-01-04 5:14 ` Valdis.Kletnieks
@ 2009-01-04 9:48 ` Benjamin Herrenschmidt
2009-01-04 20:41 ` Trent Piepho
1 sibling, 1 reply; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2009-01-04 9:48 UTC (permalink / raw)
To: Trent Piepho
Cc: Linus Torvalds, Linux Kernel Mailing List, Harvey Harrison,
David S. Miller
> I thought of doing that too, but then all the various %pX formats would all
> have to print the same for NULL pointers. For instance a resource prints
> out like "[0x1000-0x100f]", so I chose "[NULL]" for printing a null
> resource pointer. Maybe "[]" or "[-]" would be better? A null MAC address
> could be ":::::" or "x:x:x:x:x:x". "N.U.L.L" or "x.x.x.x" for a null IP4
> address. And so on. So the printout looks nicer when a NULL pointer isn't
> a bug.
And how often is that ?
Seriously...
Ben.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] printk: Let %pR handle NULL pointers
2009-01-04 9:48 ` Benjamin Herrenschmidt
@ 2009-01-04 20:41 ` Trent Piepho
0 siblings, 0 replies; 9+ messages in thread
From: Trent Piepho @ 2009-01-04 20:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Linus Torvalds, Linux Kernel Mailing List, Harvey Harrison,
David S. Miller
On Sun, 4 Jan 2009, Benjamin Herrenschmidt wrote:
> > I thought of doing that too, but then all the various %pX formats would all
> > have to print the same for NULL pointers. For instance a resource prints
> > out like "[0x1000-0x100f]", so I chose "[NULL]" for printing a null
> > resource pointer. Maybe "[]" or "[-]" would be better? A null MAC address
> > could be ":::::" or "x:x:x:x:x:x". "N.U.L.L" or "x.x.x.x" for a null IP4
> > address. And so on. So the printout looks nicer when a NULL pointer isn't
> > a bug.
>
> And how often is that ?
For resource pointers it's not uncommon. There are many cases when it's perfectly
ok for a resource pointer to be null, e.g. res->parent is often null, pci devices
and busses don't usually have all possible ranges, etc. It nice to get decent info
printouts without having the check for NULL pointers.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-01-04 20:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-03 10:42 [PATCH] printk: Let %pR handle NULL pointers Trent Piepho
2009-01-03 17:10 ` Linus Torvalds
2009-01-03 19:33 ` Harvey Harrison
2009-01-03 21:53 ` Benjamin Herrenschmidt
2009-01-04 5:02 ` Trent Piepho
2009-01-04 5:14 ` Valdis.Kletnieks
2009-01-04 9:31 ` Johannes Berg
2009-01-04 9:48 ` Benjamin Herrenschmidt
2009-01-04 20:41 ` Trent Piepho
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.