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

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

[Andy Shevchenko]

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.

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)
+{
+	spec.field_width = 2 + 2 * type;
+	spec.flags |= SPECIAL | SMALL | ZEROPAD;
+	spec.base = 16;
+	return number(buf, end, value, spec);
+}
+
 static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
@@ -649,11 +659,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 
 	return string(buf, end, sym, spec);
 #else
-	spec.field_width = 2 * sizeof(void *);
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
-	spec.base = 16;
-
-	return number(buf, end, value, spec);
+	return xnumber(buf, end, value, sizeof(void *), spec);
 #endif
 }
 
@@ -1318,36 +1324,20 @@ static
 char *netdev_feature_string(char *buf, char *end, const u8 *addr,
 		      struct printf_spec spec)
 {
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
-	if (spec.field_width == -1)
-		spec.field_width = 2 + 2 * sizeof(netdev_features_t);
-	spec.base = 16;
-
-	return number(buf, end, *(const netdev_features_t *)addr, spec);
+	return xnumber(buf, end, *(const netdev_features_t *)addr, sizeof(netdev_features_t), spec);
 }
 
 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);
 }
 
 static noinline_for_stack
@@ -1366,10 +1356,7 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
 		return string(buf, end, __clk_get_name(clk), spec);
 #else
-		spec.base = 16;
-		spec.field_width = sizeof(unsigned long) * 2 + 2;
-		spec.flags |= SPECIAL | SMALL | ZEROPAD;
-		return number(buf, end, (unsigned long)clk, spec);
+		return xnumber(buf, end, (unsigned long)clk, sizeof(unsigned long), spec);
 #endif
 	}
 }
-- 
2.6.4

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

[Joe Perches]

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

xnumber isn't a great name.

unsigned int type should probably be size_t size

> +{
> +	spec.field_width = 2 + 2 * type;
> +	spec.flags |= SPECIAL | SMALL | ZEROPAD;
> +	spec.base = 16;
> +	return number(buf, end, value, spec);
> +}
> +
>  static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
>  {
>  	size_t size;
> @@ -649,11 +659,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
>  
>  	return string(buf, end, sym, spec);
>  #else
> -	spec.field_width = 2 * sizeof(void *);
> -	spec.flags |= SPECIAL | SMALL | ZEROPAD;
> -	spec.base = 16;
> -
> -	return number(buf, end, value, spec);
> +	return xnumber(buf, end, value, sizeof(void *), spec);

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

[Andy Shevchenko]

On Mon, Dec 28, 2015 at 8:25 PM, 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.
>>
>> 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)
>
> xnumber isn't a great name.

I rather agree, however had nothing yet to replace. Any ideas?

> unsigned int type should probably be size_t size

Used to be :-), though I decided to move it to unsigned int since the
resulting field is anyway 8 bits of unsigned int.
If you think it's better to do all conversion inside xnumber (or
whatever name it would be), I redo this.

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Mon, 2015-12-28 at 21:02 +0200, Andy Shevchenko wrote:
> On Mon, Dec 28, 2015 at 8:25 PM, 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.
> > > 
> > > 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)
> > 
> > xnumber isn't a great name.
> 
> I rather agree, however had nothing yet to replace. Any ideas?

prefixed_hex_number?

> > unsigned int type should probably be size_t size
> 
> Used to be :-), though I decided to move it to unsigned int since the
> resulting field is anyway 8 bits of unsigned int.
> If you think it's better to do all conversion inside xnumber (or
> whatever name it would be), I redo this.

It's going to get cast to that field size anyway
as either size_t or unsigned int

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

[Rasmus Villemoes]

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

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@rasmusvillemoes.dk> wrote:
> 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 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@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?

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

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

[Rasmus Villemoes]

On Mon, Dec 28 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> 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 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.

Maybe just don't mention "last byte" (I thought it was referring to the
final '\0' terminator, and the "78" is actually the first byte on
little-endian, so there's lots of ways to interpret this wrongly...),
and say that the width doesn't take the prefix into account (which is
obviously what has been forgotten).

BTW, thinking a bit more about this, using the field width+ZEROPAD is
arguably wrong. It would be better to set the precision to
2*sizeof(type), since for numeric conversions the precision precisely
means "the minimum number of digits to appear". That also avoids the
annoying interactions with a user-supplied field width, and actually
allows the user to do

%-20pNF

to get "0x00abcdef" padded with 10 spaces on the right (provided we
do pass through the original spec). So I now think xnumber should do

spec.base = 16;
spec.flags |= SMALL | SPECIAL;
spec.precision = 2*size;

Since gcc complains about the 0 flag passed to %p, that will never be
set, so any field width padding either left or right will be by spaces.

But I agree that it should be explicitly documented which %p extensions
accept and honour a field width and which that don't (some out of
necessity, since it's overloaded to pass e.g. a bitmap size or buffer
size).

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

It's a helper local to that file, so I'm not too worried about whatever
name is chosen. full_width_lower_case_hexnumber is obviously way too
verbose. I suppose most people instinctively expect hex numbers to be in
lower case, but full_width_hexnumber is still too much. (also, I think
you misinterpreted me: I wasn't complaining about 'x' not saying
everything, I was complaining about 'x' not saying anything at all. IOW,
what I meant was that, taken out of context, a function called 'xnumber'
doesn't immediately tell me that it has anything to do with printing a
number in hexadecimal, so it would be better to spend two more
characters on its name to at least carry one aspect of what it does.).

Rasmus

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

[Andy Shevchenko]

On Tue, 2015-12-29 at 00:20 +0200, Andy Shevchenko wrote:
> On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > 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 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@linux.intel.c
> > > > om>
> > > > ---
> > > >  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.

Oops, 24 bits of signed int. Incorrectly caught wrong line.
So, I will change this to be int size then.

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Rasmus Villemoes]

On Mon, Dec 28 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

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

Actually, LEFT can be passed to %p (or get set by passing a negative
field width via %*p), which would be actively harmful: When LEFT is set,
number() explicitly removes the ZEROPAD flag, so we'd get "0xabcdef  "
instead of "0x00abcdef".

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

[Andy Shevchenko]

On Tue, Dec 29, 2015 at 12:20 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Mon, Dec 28 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>>
>> 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
>
> Actually, LEFT can be passed to %p (or get set by passing a negative
> field width via %*p), which would be actively harmful: When LEFT is set,
> number() explicitly removes the ZEROPAD flag, so we'd get "0xabcdef  "
> instead of "0x00abcdef".

My opinion we have to establish strict rules what we print in case of
prefixed pointer (when #ifdef is false in some cases, e.g. struct clk)
or fixed type values, such as phys_addr_t. I mean to always have a
maximum width for the type on the running architecture.

-- 
With Best Regards,
Andy Shevchenko