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
