Message-ID: <20120709120326.GH2627@aemeltch-MOBL1>
Date:	Mon, 9 Jul 2012 15:03:33 +0300
From:	Andrei Emeltchenko <andrei.emeltchenko@...el.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Andy Shevchenko <andy.shevchenko@...il.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>, proski@....org,
	linux-wireless@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

Hi,

On Tue, Jul 03, 2012 at 11:48:00AM -0700, Joe Perches wrote:
> On Tue, 2012-07-03 at 21:32 +0300, Andy Shevchenko wrote:
> > On Tue, Jul 3, 2012 at 6:33 PM, Joe Perches <joe@...ches.com> wrote:
> > > On Tue, 2012-07-03 at 13:06 +0300, Andy Shevchenko wrote:
> > >> There are many places in the kernel where the drivers print small buffers as a
> > >> hex string. This patch adds a support of the variable width buffer to print it
> > >> as a hex string with a delimiter. The idea came from Pavel Roskin here:
> > >> http://www.digipedia.pl/usenet/thread/18835/17449/
> > >>
> > >> Sample output of
> > >>       pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
> > >> could be look like this:
> > >>       [ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
> > >>       [ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
> > >>       [ 0.757602] buf[17:5] ac:16:d5:2c:ef

The idea is really good and can make redundant lots of print_hex
functions.

> > 
> > > It might be more sensible to use new, distinct
> > > "%*pH" and "%*ph" functions and not touch the
> > > mac address function at all.  Will anyone ever
> > > really want to emit the buffer in reverse?
> > > I don't think so.
> > Yeah, probably it's only the case for the Bluetooth addresses.
> > 
> > > Perhaps when using a hex_string_buffer func the
> > > separator should be a space/no-space with %*pHh.
> > What I learned from today's linux-next is the most used separators are
> > ' ' (space), '' (nothing), ':' and '-'. We have dozens of the cases
> > for first three. The '-' support could not be implemented
> > nevertheless.
> > So, might be %*pHh[CDS] C for 'colon', S for 'space', D for 'dash' looks better.
> 
> Maybe use a space default.

I do not think we need other specifier then space at all.

> 
> > 'Hh' for capital/small letters than?
> 
> If you want, though I'd hope nobody uses upper case.
> 
> > > You could extend the max to 128 or larger now.
> > I don't think it is really needed.
> 
> I hope it's not, but I just don't see the need to limit it.
> 
> > Most of the current cases usually
> > print not more than ~30bytes (in average) per time. And I couldn't
> > imagine good looking printing for long lines anyway.

Maybe insert '\n' after 16 numbers?

> 
> Yup, they'd be ugly.
> print_hex_dump() should be favored anyway.

For print_hex_dump we would need to invent unreadable constructions with
defines like you mentioned in other thread:

#ifdef SOME_BLUETOOTH_DEBUG_FLAG
#define bt_hex_dump_dbg(...)    \
        print_hex_dump(...)
#else
#define bt_hex_dump_dbg(...)    \
        do { } while (0)
#endif

Best regards 
Andrei Emeltchenko 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists