Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Aug 27, 2024 at 11:12:47PM -0500, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote:
> > > Andy Shevchenko wrote:
> > > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > > > Petr Mladek wrote:
> > > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:

[snip]

> > > +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];
> > > +       char *p = sym, *pend = sym + sizeof(sym);
> > 
> > Missing check for pointer, but it's not that I wanted to tell.
> 
> No it was not missing.  It was checked in address_val() already.  However, with
> %pra I'll have to add it in.

Ah, I haven't noticed the address_val() implementation details, thanks for
elaborating!

> > > +       *p++ = '[';
> > > +       p = string_nocheck(p, pend, "range ", default_str_spec);
> > 
> > Hmm... %pr uses str_spec, what the difference can be here?
> 
> str_spec is designed for variable length strings which are used based on the
> struct resource flags.  Struct range does not vary so default_str_spec works.

Okay, makes sense.

> > > +       p = special_hex_number(p, pend, range->start, sizeof(range->start));
> > > +       *p++ = '-';
> > > +       p = special_hex_number(p, pend, range->end, sizeof(range->end));
> > 
> > This is basically the copy of %pr implementation.
> 
> Only at a very basic level.  struct resource has a variable spec while struct
> range does not.  This causes complexity to make the code the same.

Fair enough, that's why I said "as much as possible to deduplicate". If you
think this is not worth it, let's do without an additional complications then.

> > 	p = number(p, pend, res->start, *specp);
> > 	if (res->start != res->end) {
> > 		*p++ = '-';
> > 		p = number(p, pend, res->end, *specp);
> > 	}
> > 
> > Would it be possible to unify? I think so, but it requires a bit of thinking.
> 
> Not much thinking.  But the issue is that they are not close enough to justify
> the extra complexity IMHO.

Okay!

> Making the outputs match with a common function takes 13 lines of code[1]
> including the declaration of a print specification which, as this thread
> already showed, is non-trivial to understand.

> __Also__ this is currently crashing on me and I can't figure out why.
> 
> $ git diff --stat
>  lib/vsprintf.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> OTOH to force a unified output, only takes 2 lines of duplicated code.[2]  This
> is a very minor expense of duplicate code which is much easier to follow.
> 
> $ git diff --stat
>  lib/vsprintf.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Yep, got it.

> > That's why testing is very important in this kind of generic code.
> 
> Yep.  But the struct resource test was stubbed out.  I've added some basic
> ones.  But there are many more variations of struct resource prints.  I'm not
> sure I've not broken them.

Yeah, so make it then separated branches for %pr and %pra. You will take the
correct argument type in each of them. There are existing examples there.

Probably an initial 'r'/'R' parsing should be moved to pointer().

> > > +       *p++ = ']';
> > > +       *p = '\0';
> > > +
> > > +       return string_nocheck(buf, end, sym, spec);
> > > +}

...

> > > +       struct range test_range = {
> > > +               .start = 0xc0ffee00ba5eba11,
> > > +               .end = 0xc0ffee00ba5eba11,
> > > +       };
> > > +
> > > +       test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]",
> > > +            "%par", &test_range);
> > > +
> > > +       test_range = (struct range) {
> > > +               .start = 0xc0ffee,
> > > +               .end = 0xba5eba11,
> > > +       };
> > > +       test("[range 0x0000000000c0ffee-0x00000000ba5eba11]",
> > > +            "%par", &test_range);
> > 
> > Case when start == end?
> 
> Yes, that is the 1st case.

Thumb up!

> > Case when end < start?
> 
> I had no intention of having the output dictated by the values.
> 
> 	test("[range 0x0000000000c0ffee-0x0000000000c0ffee]",
> and
> 	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",
> 
> ... are acceptable to me.

But it seems the %pr in the first case doesn't do range, just a single value,
which makes sense to me (and this thread proved it) to avoid needless pedantic
checking of each value. It means that at a glance you may tell start == end.
Not sure about end < start case, but the point is just let's make it mimicing
%pr behaviour.

...

> +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) {
> +               *buf++ = '-';
> +               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 +1127,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)
> @@ -1149,11 +1157,19 @@ char *range_string(char *buf, char *end, const struct range *range,
>         char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
>         char *p = sym, *pend = sym + sizeof(sym);
>  
> +       struct printf_spec range_spec = {
> +               spec.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * u64 */
> +               spec.flags = SPECIAL | SMALL | ZEROPAD,
> +               spec.base = 16,
> +               spec.precision = -1,
> +       };

But this can be deduplicated from special_hex_number(), no?
Something like

fill_special_hex_number_spec()
{
}

special_hex_number()
{
	fill_special_hex_number_spec();
}

special_hex_range()
{
	fill_special_hex_number_spec();
}

Would it be better?

> +       if (check_pointer(&buf, end, range, spec))
> +               return buf;
> +
>         *p++ = '[';
>         p = string_nocheck(p, pend, "range ", default_str_spec);
> -       p = special_hex_number(p, pend, range->start, sizeof(range->start));
> -       *p++ = '-';
> -       p = special_hex_number(p, pend, range->end, sizeof(range->end));
> +       p = hex_range(p, pend, range->start, range->end, range_spec);
>         *p++ = ']';
>         *p = '\0';

so, can you check if with the above implemented we can actually enforce unified
format for %pr and %pra?

...

> [2] sample diff

>         p = special_hex_number(p, pend, range->start, sizeof(range->start));
> -       *p++ = '-';
> -       p = special_hex_number(p, pend, range->end, sizeof(range->end));
> +       if (range->start != range->end) {
> +               *p++ = '-';
> +               p = special_hex_number(p, pend, range->end, sizeof(range->end));
> +       }

There is a possibility to supply a callback, but it seems to me much
overcomplicated approach.

...

If we go the second way (the latter one here) can you add a comment in both
%pr/%pra code excerpts to point to each other that the format is unified
between them? It might help in the future to optimise the code if needed at
all.

-- 
With Best Regards,
Andy Shevchenko