All of lore.kernel.org
 help / color / mirror / Atom feed
* [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	[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	[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.