[PATCH] Fix bytecount result from printk()

[PATCH] Fix bytecount result from printk()

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

printk() returns a bytecount, which nothing actually appears to use.

This count is generated internally in vprintk(),
and is off-by-3 for one particular path through that function.

This patch fixes it to be consistent with how it is calculated
for the other paths through that same function (vprintk).

Whether or not the count should even exist in the first place
is still a question for examination -- nothing appears to use it.

On a related note, WHY does the LOG LEVEL format <6> not get
interpreted correctly for the first printk() after an oops report?
As in this example -- the <6> is printed, instead of being interpreted:

	kernel:  <6>note: insmod[31060] exited with preempt_count 1

Here's the off-by-3 fix patch.

Signed-off-by:  Mark Lord <lkml@rtr.ca>

--- linux-2.6.15-rc3/kernel/printk.c.orig       2005-11-29 23:24:19.000000000 -0500
+++ linux/kernel/printk.c       2005-12-01 10:01:39.000000000 -0500
@@ -592,8 +592,8 @@
                                         emit_log_char(default_message_loglevel
                                                 + '0');
                                         emit_log_char('>');
-                               }
-                               printed_len += 3;
+                               } else
+                                       printed_len += 3;
                         }
                         log_level_unknown = 0;
                         if (!*p)

[-- Attachment #2: printk.patch --]
[-- Type: text/x-patch, Size: 358 bytes --]

--- linux-2.6.15-rc3/kernel/printk.c.orig	2005-11-29 23:24:19.000000000 -0500
+++ linux/kernel/printk.c	2005-12-01 10:01:39.000000000 -0500
@@ -592,8 +592,8 @@
 					emit_log_char(default_message_loglevel
 						+ '0');
 					emit_log_char('>');
-				}
-				printed_len += 3;
+				} else
+					printed_len += 3;
 			}
 			log_level_unknown = 0;
 			if (!*p)

Re: [PATCH] Fix bytecount result from printk()

[Linus Torvalds]

On Thu, 1 Dec 2005, Mark Lord wrote:
> 
> On a related note, WHY does the LOG LEVEL format <6> not get
> interpreted correctly for the first printk() after an oops report?

It never gets interpreted except at the behinning of a line. Sounds like 
the oops report perhaps prints a " " without a newline or something at the 
end, so that the next message after that isn't a new line?

		Linus

Re: [PATCH] Fix bytecount result from printk()

[Mark Lord]

Linus Torvalds wrote:
> 
> On Thu, 1 Dec 2005, Mark Lord wrote:
> 
>>On a related note, WHY does the LOG LEVEL format <6> not get
>>interpreted correctly for the first printk() after an oops report?
>
> It never gets interpreted except at the behinning of a line. Sounds like 
> the oops report perhaps prints a " " without a newline or something at the 
> end, so that the next message after that isn't a new line?

The oops report always ends with a simple newline:  printk("\n");

Re: [PATCH] Fix bytecount result from printk()

[Dave Jones]

On Thu, Dec 01, 2005 at 10:55:49AM -0500, Mark Lord wrote:
 > printk() returns a bytecount, which nothing actually appears to use.

We do check it in a few places.

arch/x86_64/kernel/traps.c:                             i += printk(" "); \
arch/x86_64/kernel/traps.c:                     i += printk(" <%s> ", id);
arch/x86_64/kernel/traps.c:                     i += printk(" <EOE> ");
arch/x86_64/kernel/traps.c:                             i += printk(" <IRQ> ");
arch/x86_64/kernel/traps.c:                             i += printk(" <EOI> ");
drivers/char/mem.c:             ret = printk("%s", tmp);

		Dave

Re: [PATCH] Fix bytecount result from printk()

[David S. Miller]

From: Dave Jones <davej@redhat.com>
Date: Thu, 1 Dec 2005 12:57:32 -0500

> On Thu, Dec 01, 2005 at 10:55:49AM -0500, Mark Lord wrote:
>  > printk() returns a bytecount, which nothing actually appears to use.
> 
> We do check it in a few places.
> 
> arch/x86_64/kernel/traps.c:                             i += printk(" "); \
> arch/x86_64/kernel/traps.c:                     i += printk(" <%s> ", id);
> arch/x86_64/kernel/traps.c:                     i += printk(" <EOE> ");
> arch/x86_64/kernel/traps.c:                             i += printk(" <IRQ> ");
> arch/x86_64/kernel/traps.c:                             i += printk(" <EOI> ");
> drivers/char/mem.c:             ret = printk("%s", tmp);

Wow, that's amazing. :)

I bet these can easily be removed, and since printk() is such
a core thing, simplifying it should trump whatever benfits
these few call sites have from getting a return byte count.

Re: [PATCH] Fix bytecount result from printk()

[Andi Kleen]

"David S. Miller" <davem@davemloft.net> writes:

> From: Dave Jones <davej@redhat.com>
> Date: Thu, 1 Dec 2005 12:57:32 -0500
> 
> > On Thu, Dec 01, 2005 at 10:55:49AM -0500, Mark Lord wrote:
> >  > printk() returns a bytecount, which nothing actually appears to use.
> > 
> > We do check it in a few places.
> > 
> > arch/x86_64/kernel/traps.c:                             i += printk(" "); \
> > arch/x86_64/kernel/traps.c:                     i += printk(" <%s> ", id);
> > arch/x86_64/kernel/traps.c:                     i += printk(" <EOE> ");
> > arch/x86_64/kernel/traps.c:                             i += printk(" <IRQ> ");
> > arch/x86_64/kernel/traps.c:                             i += printk(" <EOI> ");
> > drivers/char/mem.c:             ret = printk("%s", tmp);
> 
> Wow, that's amazing. :)

Taking the blame.

> I bet these can easily be removed, and since printk() is such
> a core thing, simplifying it should trump whatever benfits
> these few call sites have from getting a return byte count.

I used it for linewrapping in the oops output.

Actually I would expect more users from sprintf and snprintf
(e.g. common in /proc output to compute the return value of the read) 
and that is exactly the same code path.

If you do the same grep for sn?printf I bet there will be much more hits.

-Andi

Re: [PATCH] Fix bytecount result from printk()

[Mark Lord]

Andi Kleen wrote:
> "David S. Miller" <davem@davemloft.net> writes:
..
>>Wow, that's amazing. :)
> 
> Taking the blame.
> 
>>I bet these can easily be removed, and since printk() is such
>>a core thing, simplifying it should trump whatever benfits
>>these few call sites have from getting a return byte count.
> 
> I used it for linewrapping in the oops output.
...
> Actually I would expect more users from sprintf and snprintf
> (e.g. common in /proc output to compute the return value of the read) 
> and that is exactly the same code path.

When I grep the 2.6.15-rc3 kernel tree, the *only* use of vprintk
seems to be for doing printk().  It does not seem to be used for
the sprintf/snprintf functions.  Actually it is the other way around,
where vprintk() calls those functions.

So no problem there, and vprintk() really doesn't need to return anything.

Cheers

Re: [PATCH] Fix bytecount result from printk()

[Andi Kleen]

> When I grep the 2.6.15-rc3 kernel tree, the *only* use of vprintk
> seems to be for doing printk().  It does not seem to be used for
> the sprintf/snprintf functions.  Actually it is the other way around,
> where vprintk() calls those functions.
> 
> So no problem there, and vprintk() really doesn't need to return anything.

That seems wrong. I'm pretty sure we don't have two different printf formatting engines.

-Andi

Re: [PATCH] Fix bytecount result from printk()

[Andrew Morton]

Dave Jones <davej@redhat.com> wrote:
>
> On Thu, Dec 01, 2005 at 10:55:49AM -0500, Mark Lord wrote:
>  > printk() returns a bytecount, which nothing actually appears to use.
> 
> We do check it in a few places.
> 
> arch/x86_64/kernel/traps.c:                             i += printk(" "); \
> arch/x86_64/kernel/traps.c:                     i += printk(" <%s> ", id);
> arch/x86_64/kernel/traps.c:                     i += printk(" <EOE> ");
> arch/x86_64/kernel/traps.c:                             i += printk(" <IRQ> ");
> arch/x86_64/kernel/traps.c:                             i += printk(" <EOI> ");
> drivers/char/mem.c:             ret = printk("%s", tmp);
> 

And if you don't fix kmsg_write(), this patch will actually break /dev/kmsg
- userspace complains about the write() return value.

Please review these patches, queued since 2.6.15-rc1-mm1:

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc3/2.6.15-rc3-mm1/broken-out/printk-return-value-fix-it.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc3/2.6.15-rc3-mm1/broken-out/kmsg_write-dont-return-printk-return-value.patch

Re: [PATCH] Fix bytecount result from printk()

[Mark Lord]

Andrew Morton wrote:
>
> Please review these patches, queued since 2.6.15-rc1-mm1:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc3/2.6.15-rc3-mm1/broken-out/printk-return-value-fix-it.patch
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc3/2.6.15-rc3-mm1/broken-out/kmsg_write-dont-return-printk-return-value.patch

Yes, those patches actually appear to fix the counting.

The patch I submitted merely makes it internally consistent
within printk/vprintk(), but does not change it to actually
be correct.

Cheers

Re: [PATCH] Fix bytecount result from printk()

[David S. Miller]

From: Mark Lord <lkml@rtr.ca>
Date: Thu, 01 Dec 2005 10:55:49 -0500

> Whether or not the count should even exist in the first place
> is still a question for examination -- nothing appears to use it.

I think it should be killed, in all of my Linux kernel
hacking I've never seen a user of this return value. :-)