Re: [PATCH v4 02/28] printk: Add print format (%pra) for struct range

[Ira Weiny]

Rasmus Villemoes wrote:
> Ira Weiny <ira.weiny@xxxxxxxxx> writes:
> 
> > ---
> >  Documentation/core-api/printk-formats.rst | 13 ++++++++
> >  lib/test_printf.c                         | 26 +++++++++++++++
> >  lib/vsprintf.c                            | 55 +++++++++++++++++++++++++++----
> >  3 files changed, 88 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 14e093da3ccd..03b102fc60bb 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -231,6 +231,19 @@ width of the CPU data path.
> >  
> >  Passed by reference.
> >  
> > +Struct Range
> > +------------
> 
> Probably neither of those words should be capitalized.

I was following the format of the header of struct resource

	Struct Resources
	----------------

I can change it but I was trying to be consistent here.

[snip]

> > +static void __init
> > +struct_range(void)
> > +{
> > +	struct range test_range = {
> > +		.start = 0xc0ffee00ba5eba11,
> > +		.end = 0xc0ffee00ba5eba11,
> > +	};
> > +
> > +	test("[range 0xc0ffee00ba5eba11]", "%pra", &test_range);
> > +
> > +	test_range = (struct range) {
> > +		.start = 0xc0ffee,
> > +		.end = 0xba5eba11,
> > +	};
> > +	test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> > +	     "%pra", &test_range);
> > +
> > +	test_range = (struct range) {
> > +		.start = 0xba5eba11,
> > +		.end = 0xc0ffee,
> > +	};
> > +	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> > +	     "%pra", &test_range);
> > +}
> > +
> 
> Thanks for including tests!
> 
> Rather than the struct assignments, I think it's easier to read if you
> just do

I'm using Andy's suggestion of DEFINE_RANGE()

> 
>   struct range r;
> 
>   r.start = 0xc0ffee00ba5eba11;
>   r.end   = r.start;
>   ...
> 
>   r.start = 0xc0ffee;
>   r.end   = 0xba5eba11;
>   ...
> 
> which saves two lines per test and for the first one makes it more
> obvious that the start and end values are identical.
> 
> >  static void __init
> >  addr(void)
> >  {
> > @@ -807,6 +832,7 @@ test_pointer(void)
> >  	symbol_ptr();
> >  	kernel_ptr();
> >  	struct_resource();
> > +	struct_range();
> >  	addr();
> >  	escaped_str();
> >  	hex_string();
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 09f022ba1c05..f8f5ed8f4d39 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1039,6 +1039,19 @@ static const struct printf_spec default_dec04_spec = {
> >  	.flags = ZEROPAD,
> >  };
> >  
> > +static noinline_for_stack
> > +char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
> > +		struct printf_spec spec)
> > +{
> > +	buf = number(buf, end, start_val, spec);
> > +	if (start_val != end_val) {
> > +		if (buf < end)
> > +			*buf++ = '-';
> 
> No. Either all your callers pass a (probably stack-allocated) buffer
> which is guaranteed to be big enough, in which case you don't need the
> "if (buf < end)", or if some callers may "print" directly to the buffer
> passed to vsnprintf(), the buf++ must still be done unconditionally in
> order that vsnprintf(NULL, 0, ...) [used by fx kasprintf] can accurately
> determine how large the output string would be.
> 
> So, either
> 
>   *buf++ = '-'
> 
> or
> 
>   if (buf < end)
>     *buf = '-';
>   buf++;
> 
> Please don't mix the two. 

Ah ok yea fixed building on Andy's comment.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a7b5e4618f6a..7aa47f7d9d5b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1048,7 +1048,8 @@ char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
                return buf;

        if (buf < end)
-               *buf++ = '-';
+               *buf = '-';
+       ++buf;
        return number(buf, end, end_val, spec);
 }

> 
> 
> 
> > +		buf = number(buf, end, end_val, spec);
> > +	}
> > +	return buf;
> > +}
> > +
> >  static noinline_for_stack
> >  char *resource_string(char *buf, char *end, struct resource *res,
> >  		      struct printf_spec spec, const char *fmt)
> > @@ -1115,11 +1128,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >  		p = string_nocheck(p, pend, "size ", str_spec);
> >  		p = number(p, pend, resource_size(res), *specp);
> >  	} else {
> > -		p = number(p, pend, res->start, *specp);
> > -		if (res->start != res->end) {
> > -			*p++ = '-';
> > -			p = number(p, pend, res->end, *specp);
> > -		}
> > +		p = hex_range(p, pend, res->start, res->end, *specp);
> >  	}
> >  	if (decode) {
> >  		if (res->flags & IORESOURCE_MEM_64)
> > @@ -1140,6 +1149,34 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >  	return string_nocheck(buf, end, sym, spec);
> >  }
> >  
> > +static noinline_for_stack
> > +char *range_string(char *buf, char *end, const struct range *range,
> > +		   struct printf_spec spec, const char *fmt)
> > +{
> > +#define RANGE_DECODED_BUF_SIZE		((2 * sizeof(struct range)) + 4)
> > +#define RANGE_PRINT_BUF_SIZE		sizeof("[range -]")
> > +	char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> 
> I don't think these names or the split in two constants helps
> convincing that's the right amount. I have to think quite a bit to see
> that 2*sizeof is because struct range has two u64 and we're printing in
> hex so four-bits-per-char and probably the +4 are for two time "0x".

Yea.

> 
> Why not just size the buffer directly using an "example" string?
> 
>   char sym[sizeof("[range 0x0123456789abcdef-0x0123456789abcdef]")]

Ok that is simpler.

> 
> > +	char *p = sym, *pend = sym + sizeof(sym);
> > +
> > +	struct printf_spec range_spec = {
> > +		.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * 8 */
> > +		.flags = SPECIAL | SMALL | ZEROPAD,
> > +		.base = 16,
> > +		.precision = -1,
> > +	};
> > +
> > +	if (check_pointer(&buf, end, range, spec))
> > +		return buf;
> > +
> > +	*p++ = '[';
> > +	p = string_nocheck(p, pend, "range ", default_str_spec);
> 
> We really should have mempcpy or stpcpy. I don't see the point of using
> string_nocheck here, or not including the [ in the string copy (however
> it's done). But yeah, without stpcpy() that's a bit awkward. 

Added '[' to the string.  The prevalent use of string_nocheck() seems
reasonable to me but it is pretty heavyweight for this case.

Ira