Re: [PATCH] Add missing newlines to some uses of dev_<level> messages

[Jean Delvare <khali@linux-fr.org>]

Hi Nish,

On Tue, 7 Aug 2007 20:00:24 -0700, Nish Aravamudan wrote:
> On 8/7/07, Joe Perches <joe@perches.com> wrote:
> > Found these while looking at printk uses.
> >
> > Add missing newlines to dev_<level> uses
> > Add missing KERN_<level> prefixes to multiline dev_<level>s
> > Fixed a wierd->weird spelling typo
> > Added a newline to a printk
> 
> I think these should have been split logically into 4 groups of
> patches (except perhaps where they touched the same file for the first
> two and the last).

Not worth splitting IMHO, these are all misuses of printk.

> The third type should go through trivial.

Except for this one, which really doesn't belong to this patch, granted.

> And they should have been split logically by subsystem, most likely?

Depends. If Andrew is going to pick the patch quickly and apply it,
then it's fine (except that I see that Andrew wasn't Cc'd, so it's
unlikely to happen.) But if Andrew doesn't want to take this patch,
then indeed, the only way to get it applied is to split it by
subsystem.

> 
> <snip diffstat>
> 
> > diff --git a/arch/ia64/sn/kernel/xpnet.c b/arch/ia64/sn/kernel/xpnet.c
> > index e58fcad..a5df672 100644
> > --- a/arch/ia64/sn/kernel/xpnet.c
> > +++ b/arch/ia64/sn/kernel/xpnet.c
> > @@ -269,8 +269,9 @@ xpnet_receive(partid_t partid, int channel, struct xpnet_message *msg)
> >         skb->protocol = eth_type_trans(skb, xpnet_device);
> >         skb->ip_summed = CHECKSUM_UNNECESSARY;
> >
> > -       dev_dbg(xpnet, "passing skb to network layer; \n\tskb->head=0x%p "
> > -               "skb->data=0x%p skb->tail=0x%p skb->end=0x%p skb->len=%d\n",
> > +       dev_dbg(xpnet, "passing skb to network layer\n"
> > +               KERN_DEBUG "\tskb->head=0x%p skb->data=0x%p skb->tail=0x%p "
> > +               "skb->end=0x%p skb->len=%d\n",
> >                 (void *)skb->head, (void *)skb->data, skb_tail_pointer(skb),
> >                 skb_end_pointer(skb), skb->len);
> 
> <snip>
> 
> I'm not a maintainer of any code by any means, but it seems odd,
> redundant and visually confusing to me to use the dev_dbg() function
> and have KERN_DEBUG's inside the string -- makes me question every
> time why there isn't one for the first line too.

The code above looks correct to me.

> Perhaps these multi-line debug statements should simply be split into
> multiple dev_dbg() calls? Similar for the other dev_ functions. I
> think it might look cleaner, at least. Just my two cents, not a NAK or
> anything.

The benefit of having a single call is that other calls to printk()
won't be interlaced with our message. This has been discussed lately in
a different thread. So you may think that the code above is confusing,
but it is currently the best way to ensure that related lines stay
grouped in the kernel log.

-- 
Jean Delvare