lkml.org 
[lkml]   [2019]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
    From
    Date
    On 25/08/2019 01.37, Uwe Kleine-König wrote:
    > pr_info("probing failed (%dE)\n", ret);
    >
    > expands to
    >
    > probing failed (EIO)
    >
    > if ret holds -EIO (or EIO). This introduces an array of error codes. If
    > the error code is missing, %dE falls back to %d and so prints the plain
    > number.
    >
    > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
    > ---
    > Hello
    >
    > there are many code sites that benefit from this. Just grep for
    > "(%d)" ...
    >
    > As an example the follow up patch converts a printk to use this new
    > format escape.
    >
    > Best regards
    > Uwe
    >
    > Documentation/core-api/printk-formats.rst | 3 +
    > lib/vsprintf.c | 193 +++++++++++++++++++++-
    > 2 files changed, 195 insertions(+), 1 deletion(-)
    >
    > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
    > index c6224d039bcb..81002414f956 100644
    > --- a/Documentation/core-api/printk-formats.rst
    > +++ b/Documentation/core-api/printk-formats.rst
    > @@ -35,6 +35,9 @@ Integer types
    > u64 %llu or %llx
    >
    >
    > +To print the name that corresponds to an integer error constant, use %dE and
    > +pass the int.
    > +
    > If <type> is dependent on a config option for its size (e.g., sector_t,
    > blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
    > format specifier of its largest possible type and explicitly cast to it.
    > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
    > index b0967cf17137..672eab8dab84 100644
    > --- a/lib/vsprintf.c
    > +++ b/lib/vsprintf.c
    > @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
    > return buf;
    > }
    >
    > +#define ERRORCODE(x) { .str = #x, .err = x }
    > +
    > +static const struct {
    > + const char *str;
    > + int err;
    > +} errorcodes[] = {
    > + ERRORCODE(EPERM),
    ...
    > + ERRORCODE(ERECALLCONFLICT),
    > +};
    > +
    > +static noinline_for_stack
    > +char *errstr(char *buf, char *end, unsigned long long num,
    > + struct printf_spec spec)
    > +{
    > + char *errname = NULL;
    > + size_t errnamelen, copy;
    > + int i;
    > +
    > + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
    > + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
    > + errname = errorcodes[i].str;
    > + break;
    > + }
    > + }

    Not sure I'm a fan of iterating this array. Yes, the errno values are a
    bit sparse, but maybe one could use a dense array with O(1) lookup for
    those < 128 and then have an exceptional table like yours for the rest.
    But if you do keep this whole array thing, please do as Andrew suggested
    and compact it somewhat (4 bytes per entry plus the storage for the
    strings themselves is enough, see e.g. e1f0bce3), and put EINVAL and
    other common things near the start (at least EINVAL is a lot more common
    than ENOEXEC).

    > + if (!errname) {
    > + /* fall back to ordinary number */
    > + return number(buf, end, num, spec);
    > + }
    > +
    > + copy = errnamelen = strlen(errname);
    > + if (copy > end - buf)
    > + copy = end - buf;
    > + buf = memcpy(buf, errname, copy);

    This is buggy, AFAICT. buf may be beyond end (that's the convention), so
    end-buf (which is a ptrdiff_t, which is probably a signed type, but it
    gets converted to a size_t when compared to copy) can be a huge number,
    so copy>end-buf is false.

    Please just use the string() helper that gets used for printing other
    fixed strings (as well as %s input).

    And add tests to lib/test_printf.c, that would catch this sort of thing
    immediately.

    > + return buf + errnamelen;
    > +}
    > +
    > static noinline_for_stack
    > char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
    > {
    > @@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
    > num = va_arg(args, unsigned int);
    > }
    >
    > - str = number(str, end, num, spec);
    > + if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
    > + fmt++;
    > + str = errstr(str, end, num, spec);

    drivers/staging/speakup/speakup_bns.c has a %dE, have you checked
    whether you're breaking that one? It's hard to grep for all the
    variations, %-0*.5dE is also legal and would be caught here.

    This has previously been suggested as a %p extension, and I think users
    would just as often have an ERR_PTR() as a plain int or long. Since we
    already have %p[alphanumeric] as a special kernel thing, why not do
    that? It's more convenient to convert from ints/longs to error pointers

    pr_err("Uh-oh: %pE", ERR_PTR(ret));

    than the other way around

    pr_err("Uh-oh: %dE", PTR_ERR(p)); // oops, must be %ldE

    Kernel size impact? Have you considered making it possible to opt-out
    and have %dE just mean %d?

    Rasmus

    \
     
     \ /
      Last update: 2019-08-26 12:14    [W:3.679 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site