Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to xnumber()

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Mon, Dec 28 2015, Joe Perches <joe@perches.com> wrote:

> On Mon, 2015-12-28 at 20:18 +0200, Andy Shevchenko wrote:
>> xnumber() is a special helper to print a fixed size type in a hex format with
>> '0x' prefix with padding and reduced size. In the module we have already
>> several copies of such code. Consolidate them under xnumber() helper.
>> 
>> There are couple of differences though.
>> 
>> It seems nobody cared about the output in case of CONFIG_KALLSYMS=n when
>> printing symbol address because the asked width is not enough to care either
>> prefix or last byte. Fixed here.

ok, though I'm curious what 'last byte' refers to here?
 
>> The %pNF specifier used to be allowed with a specific field width, though there
>> is neither any user of it nor mention in the documentation.
>> 
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  lib/vsprintf.c | 43 +++++++++++++++----------------------------
>>  1 file changed, 15 insertions(+), 28 deletions(-)
>> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index dcf5646..e971549 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -514,6 +514,16 @@ char *number(char *buf, char *end, unsigned long long num,
>>  	return buf;
>>  }
>>  
>> +static noinline_for_stack
>> +char *xnumber(char *buf, char *end, unsigned long long value, unsigned int type,
>> +	      struct printf_spec spec)

Is there any aspect of the passed-through printf_spec which isn't
overridden in xnumber? The users are/will be various %p extensions,
which probably means that no-one passes a non-default precision (gcc
complains about %.*p), and the remaining possible flags (PLUS, LEFT,
SPACE) are useless and/or impossible to pass to %p without gcc
complaining. In other words, why pass the spec at all instead of just
building it inside xnumber?

> xnumber isn't a great name.

Maybe 'hexnumber'. That's a bit further away from 'number', and 'x'
might stand for something other than hex.

> unsigned int type should probably be size_t size

Compromise: 'unsigned int size'. The name should be size since it's
supposed to be the size of the actual type being printed. But the type
carrying that information need not be 8 bytes wide on 64bits.

>> static noinline_for_stack
>>  char *address_val(char *buf, char *end, const void *addr,
>>  		  struct printf_spec spec, const char *fmt)
>>  {
>> -	unsigned long long num;
>> -
>> -	spec.flags |= SPECIAL | SMALL | ZEROPAD;
>> -	spec.base = 16;
>> -
>>  	switch (fmt[1]) {
>>  	case 'd':
>> -		num = *(const dma_addr_t *)addr;
>> -		spec.field_width = sizeof(dma_addr_t) * 2 + 2;
>> -		break;
>> +		return xnumber(buf, end, *(const dma_addr_t *)addr, sizeof(dma_addr_t), spec);
>>  	case 'p':
>>  	default:
>> -		num = *(const phys_addr_t *)addr;
>> -		spec.field_width = sizeof(phys_addr_t) * 2 + 2;
>> -		break;
>> +		return xnumber(buf, end, *(const phys_addr_t *)addr, sizeof(phys_addr_t), spec);
>>  	}
>> -
>> -	return number(buf, end, num, spec);
>>  }

Nit: I think it would be a bit easier to read if the cast+dereference
are kept outside the function calls. I'd suggest just introducing
'unsigned int size', assign the appropriate value in the two cases, and
fall through to a common 'xnumber(buf, end, num, size);'. It'll even
line up nicely ;-)

num = *(const dma_addr_t *)addr;
size = sizeof(dma_addr_t);

Rasmus