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
