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

[Andy Shevchenko]

On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Dec 28 2015, Joe Perches <joe@xxxxxxxxxxx> 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 last byte ('78') as it appears in the string carrying the number
'0x12345678'. Yeah, might be confusing, I'm open for suggestion how to
phrase it.

>
>>> 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@xxxxxxxxxxxxxxx>
>>> ---
>>>  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?

Wow, good catch!
I slightly suspected something like that, but didn't made up my mind
to check this.

>
>> xnumber isn't a great name.
>
> Maybe 'hexnumber'.

We already have similar for %*ph. And as you noticed belowâ

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

âisn't only about hex. I don't know how to play on words the all three
flags including 16 base.

>
>> 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.

Exactly, the result anyway as for now only 8 bits as a part of unsigned int.

>
>>> 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 ;-)

Will try that.

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

Thanks, Rasmus, for review.

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