[PATCH] printk: Let %pR handle NULL pointers

[PATCH] printk: Let %pR handle NULL pointers

[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

Re: [PATCH] printk: Let %pR handle NULL pointers

[Linus Torvalds]

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);

Re: [PATCH] printk: Let %pR handle NULL pointers

[Harvey Harrison]

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

Re: [PATCH] printk: Let %pR handle NULL pointers

[Benjamin Herrenschmidt]

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.

Re: [PATCH] printk: Let %pR handle NULL pointers

[Trent Piepho]

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.

Re: [PATCH] printk: Let %pR handle NULL pointers

[Valdis.Kletnieks]

[-- 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 --]

Re: [PATCH] printk: Let %pR handle NULL pointers

[Johannes Berg]

[-- 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 --]

Re: [PATCH] printk: Let %pR handle NULL pointers

[Benjamin Herrenschmidt]

> 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.

Re: [PATCH] printk: Let %pR handle NULL pointers

[Trent Piepho]

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.