| Message ID | 20190824233724.1775-1-uwe@kleine-koenig.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] vsprintf: introduce %dE for error constants | expand |
(cc printk maintainers). On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> 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. Huh. I'm surprised we don't already have this. Seems that this will be applicable in a lot of places? Although we shouldn't go blindly converting everything in sight - that would risk breaking userspace which parses kernel strings. Is it really necessary to handle the positive errnos? Does much kernel code actually do that (apart from kernel code which is buggy)? > 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)" ... Yup. This observation shouldn't be below the "^---$" ;) An approximate grep|wc would be interesting. > --- 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[] = { It's a bit of a hack, but an array of char*'s and a separate array of ushorts would save a bit of space. > + ERRORCODE(EPERM), > + ERRORCODE(ENOENT), > + ERRORCODE(ESRCH), > > ... > > +static noinline_for_stack Why this? I'm suspecting this will actually increase stack use? > +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; > + } > + } > + > + 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); > + > + return buf + errnamelen; > +} OK, I guess `errstr' is an OK name for a static function and we can use this to add a new strerror() should the need arise. > > ... >
Hello Andrew, On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote: > (cc printk maintainers). Ah, I wasn't aware there is something like them. Thanks > On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> 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. > > Huh. I'm surprised we don't already have this. Seems that this will > be applicable in a lot of places? Although we shouldn't go blindly > converting everything in sight - that would risk breaking userspace > which parses kernel strings. Uah, even the kernel log is API? But I agree so far that this is merge window material and shouldn't make it into v5.3 :-) > Is it really necessary to handle the positive errnos? Does much kernel > code actually do that (apart from kernel code which is buggy)? I didn't check; probably not. But the whole positive range seems so unused and interpreting EIO (and not only -EIO) as "EIO" seems straight forward. But I don't feel strong either way. > > 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)" ... > > Yup. This observation shouldn't be below the "^---$" ;) An approximate > grep|wc would be interesting. I didn't check how many false positives there are using "(%d)", but I'd use an a bit more elaborate expression for the commit log: $ git grep '(%d)' | wc -l 7336 $ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))\(.*(\(%d\)|: %d)\\n' | wc -l 9140 The latter matches several printk-variants emitting a string ending in one of '(%d)\n' (1839 matches) ': %d\n' (7301 matches) . I would expect that many of the 7336 matches for '(%d)' that are not matched by the longer expression are false negatives because the function name is in the previous line. So I would estimate around 10k strings that could benefit from %dE. > > --- 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[] = { > > It's a bit of a hack, but an array of char*'s and a separate array of > ushorts would save a bit of space. Hmm, true. Currently we have 150 entries taking 150 * (sizeof(void *) * 2). Is it worth to think about the hacky solution to go down to 150 * (sizeof(void *) + 2)? For an ARM build bloat-o-meter reports (comparing linus/master to linus/master + my patch): add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480) Function old new delta errorcodes - 1200 +1200 errstr - 200 +200 vsnprintf 884 960 +76 set_precision 148 152 +4 resource_string 1380 1384 +4 flags_string 400 404 +4 num_to_str 288 284 -4 format_decode 1024 1020 -4 Total: Before=21686, After=23166, chg +6.82% But that doesn't seem to include the size increase for all the added strings which seems to be around another 1300 bytes. > > + ERRORCODE(EPERM), > > + ERRORCODE(ENOENT), > > + ERRORCODE(ESRCH), > > > > ... > > > > +static noinline_for_stack > > Why this? I'm suspecting this will actually increase stack use? I don't know what it does, just copied it from number() which is used similarly. > > +char *errstr(char *buf, char *end, unsigned long long num, > > + struct printf_spec spec) > > +{ > > + char *errname = NULL; I missed a warning during my tests, there is a const missing in this line. > > + 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; > > + } > > + } > > + > > + 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); > > + > > + return buf + errnamelen; > > +} > > OK, I guess `errstr' is an OK name for a static function IMHO the name is very generic (which is bad), but it is in good company, as there is also pointer() and number(). > and we can use this to add a new strerror() should the need arise. In userspace the purpose of strerror is different. It would yield "I/O error" not "EIO". So strerror using this array would only be a "strerror light". In my first prototype I even used %m instead of %dE, but as %m (in glibc) doesn't consume an argument and produces the more verbose variant, I changed my mind and went for %dE. (Also my patch has the undocumented side effect that you can use %ldE if the error is held by a long int. I didn't test that though.) Best regards Uwe
On (08/24/19 16:58), Andrew Morton wrote: > On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> 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. > > Huh. I'm surprised we don't already have this. Seems that this will > be applicable in a lot of places? Although we shouldn't go blindly > converting everything in sight - that would risk breaking userspace > which parses kernel strings. > > Is it really necessary to handle the positive errnos? Does much kernel > code actually do that (apart from kernel code which is buggy)? Good point. POSIX functions on error usually return -1 (negative value) and set errno (positive value). Positive errno value can be passed to strerror() or strerror_r() that decode that value and return a human readable representation. E.g. strerr(9) returns "Bad file descriptor". We don't have errno. Instead, and I may be wrong on this, kernel functions are expected to return negative error codes. A very quick grep shows that there are, however, patterns like "return positive errno". E.g. drivers/xen/xenbus/xenbus_xs.c: get_error() return EINVAL; But this EINVAL eventually becomes negative err = get_error(ret); return ERR_PTR(-err); or net/bluetooth/lib.c: bt_to_errno(). But, once again, bt_to_errno() return value eventually becomes negative: err = -bt_to_errno(hdev->req_result); So errstr() probably can handle only negative values. And, may be, I'd rename errstr() to strerror(); just because there is a well known function, which "translates" errnos. Unlike strerror(), errstr() just returns a macro name. Example: "Request failed: EJUKEBOX" EJUKEBOX does not tell me anything. A quick way to find out what does EJUKEBOX stand for is to grep include/linux/errno.h #define EJUKEBOX 528 /* Request initiated, but will not complete before timeout */ One still has to grep; either for 528 or for EJUKEBOX. I think that it might be simpler, however, to grep for EJUKEBOX, because one can grep the source code immediately, while in case of 528 one has to map 528 to the corresponding macro first and then grep the source code for EJUKEBOX. Overall %dE looks interesting. -ss
On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote: >> --- 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[] = { > > It's a bit of a hack, but an array of char*'s and a separate array of > ushorts would save a bit of space. Or just #define ERRORCODE(x) [x] = #x static const char * const errorcodes[] = { ERRORCODE(EPERM), ERRORCODE(ENOENT), ... }; Saves space, faster lookup, discovers at build time why EWOULDBLOCK would always show up as EAGAIN in the logs. We don't have holes to speak of in the error codes. BR, Jani.
On Mon, 26 Aug 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote: >>> --- 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[] = { >> >> It's a bit of a hack, but an array of char*'s and a separate array of >> ushorts would save a bit of space. > > Or just > > #define ERRORCODE(x) [x] = #x > > static const char * const errorcodes[] = { > ERRORCODE(EPERM), > ERRORCODE(ENOENT), > ... > }; > > Saves space, faster lookup, discovers at build time why EWOULDBLOCK > would always show up as EAGAIN in the logs. We don't have holes to speak > of in the error codes. Meh, failed to notice the range ERESTARTSYS..ERECALLCONFLICT. Other than that, it's nicer. ;) BR, Jani.
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
On Sun 2019-08-25 11:14:42, Uwe Kleine-König wrote: > Hello Andrew, > > On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote: > > (cc printk maintainers). > > Ah, I wasn't aware there is something like them. Thanks > > > On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> 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. What was the motivation for this patch, please? Did it look like a good idea? Did anyone got tired by searching for the error codes many times a day? Did the idea came from a developer, support, or user, please? > add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480) > Function old new delta > errorcodes - 1200 +1200 > errstr - 200 +200 > vsnprintf 884 960 +76 > set_precision 148 152 +4 > resource_string 1380 1384 +4 > flags_string 400 404 +4 > num_to_str 288 284 -4 > format_decode 1024 1020 -4 > Total: Before=21686, After=23166, chg +6.82% > > But that doesn't seem to include the size increase for all the added > strings which seems to be around another 1300 bytes. This non-trivial increase of the size and the table still includes only part of the error codes. The array is long, created by cpu&paste, the index of each code is not obvious. There are ideas to make the code even more tricky to reduce the size, keep it fast. Both, %dE modifier and the output format (ECODE) is non-standard. Upper letters gain a lot of attention. But the error code is only helper information. Also many error codes are misleading because they are used either wrongly or there was no better available. There is no proof that this approach would be widely acceptable for subsystem maintainers. Some might not like mass and "blind" code changes. Some might not like the output at all. I am not persuaded that all this is worth it. Also I do not like the non-standard solution. Best Regards, Petr
On 25.08.19 01:37, Uwe Kleine-König wrote: Hi, > +static noinline_for_stack > +char *errstr(char *buf, char *end, unsigned long long num,> + struct printf_spec spec)> +{ #1: why not putting that into some separate strerror() lib function ? This is something I've been looking for quite some time (actually already hacked it up somewhere, sometime, but forgotten ...) #2: why not just having a big case statement and leave the actual lookup logic to the compiler ? IMHO, could be written in a very compact way by some macro magic > + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) { > + if (num == errorcodes[i].err || num == -errorcodes[i].err) { why not taking the abs value only once, instead of duplicate comp on each iteration ? --mtx
On Sun, Aug 25, 2019 at 2:40 AM Uwe Kleine-König <uwe@kleine-koenig.org> 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. > Let's not do this! We have already a lot of pain with pointer extension, but here is just a misleading stuff. Besides above, how you print (float) number out of kernel in sysfs in very well standard format? Please, use %p<SMTH> instead. > 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(ENOENT), > + ERRORCODE(ESRCH), > + ERRORCODE(EINTR), > + ERRORCODE(EIO), > + ERRORCODE(ENXIO), > + ERRORCODE(E2BIG), > + ERRORCODE(ENOEXEC), > + ERRORCODE(EBADF), > + ERRORCODE(ECHILD), > + ERRORCODE(EAGAIN), > + ERRORCODE(ENOMEM), > + ERRORCODE(EACCES), > + ERRORCODE(EFAULT), > + ERRORCODE(ENOTBLK), > + ERRORCODE(EBUSY), > + ERRORCODE(EEXIST), > + ERRORCODE(EXDEV), > + ERRORCODE(ENODEV), > + ERRORCODE(ENOTDIR), > + ERRORCODE(EISDIR), > + ERRORCODE(EINVAL), > + ERRORCODE(ENFILE), > + ERRORCODE(EMFILE), > + ERRORCODE(ENOTTY), > + ERRORCODE(ETXTBSY), > + ERRORCODE(EFBIG), > + ERRORCODE(ENOSPC), > + ERRORCODE(ESPIPE), > + ERRORCODE(EROFS), > + ERRORCODE(EMLINK), > + ERRORCODE(EPIPE), > + ERRORCODE(EDOM), > + ERRORCODE(ERANGE), > + ERRORCODE(EDEADLK), > + ERRORCODE(ENAMETOOLONG), > + ERRORCODE(ENOLCK), > + ERRORCODE(ENOSYS), > + ERRORCODE(ENOTEMPTY), > + ERRORCODE(ELOOP), > + ERRORCODE(EWOULDBLOCK), > + ERRORCODE(ENOMSG), > + ERRORCODE(EIDRM), > + ERRORCODE(ECHRNG), > + ERRORCODE(EL2NSYNC), > + ERRORCODE(EL3HLT), > + ERRORCODE(EL3RST), > + ERRORCODE(ELNRNG), > + ERRORCODE(EUNATCH), > + ERRORCODE(ENOCSI), > + ERRORCODE(EL2HLT), > + ERRORCODE(EBADE), > + ERRORCODE(EBADR), > + ERRORCODE(EXFULL), > + ERRORCODE(ENOANO), > + ERRORCODE(EBADRQC), > + ERRORCODE(EBADSLT), > + ERRORCODE(EBFONT), > + ERRORCODE(ENOSTR), > + ERRORCODE(ENODATA), > + ERRORCODE(ETIME), > + ERRORCODE(ENOSR), > + ERRORCODE(ENONET), > + ERRORCODE(ENOPKG), > + ERRORCODE(EREMOTE), > + ERRORCODE(ENOLINK), > + ERRORCODE(EADV), > + ERRORCODE(ESRMNT), > + ERRORCODE(ECOMM), > + ERRORCODE(EPROTO), > + ERRORCODE(EMULTIHOP), > + ERRORCODE(EDOTDOT), > + ERRORCODE(EBADMSG), > + ERRORCODE(EOVERFLOW), > + ERRORCODE(ENOTUNIQ), > + ERRORCODE(EBADFD), > + ERRORCODE(EREMCHG), > + ERRORCODE(ELIBACC), > + ERRORCODE(ELIBBAD), > + ERRORCODE(ELIBSCN), > + ERRORCODE(ELIBMAX), > + ERRORCODE(ELIBEXEC), > + ERRORCODE(EILSEQ), > + ERRORCODE(ERESTART), > + ERRORCODE(ESTRPIPE), > + ERRORCODE(EUSERS), > + ERRORCODE(ENOTSOCK), > + ERRORCODE(EDESTADDRREQ), > + ERRORCODE(EMSGSIZE), > + ERRORCODE(EPROTOTYPE), > + ERRORCODE(ENOPROTOOPT), > + ERRORCODE(EPROTONOSUPPORT), > + ERRORCODE(ESOCKTNOSUPPORT), > + ERRORCODE(EOPNOTSUPP), > + ERRORCODE(EPFNOSUPPORT), > + ERRORCODE(EAFNOSUPPORT), > + ERRORCODE(EADDRINUSE), > + ERRORCODE(EADDRNOTAVAIL), > + ERRORCODE(ENETDOWN), > + ERRORCODE(ENETUNREACH), > + ERRORCODE(ENETRESET), > + ERRORCODE(ECONNABORTED), > + ERRORCODE(ECONNRESET), > + ERRORCODE(ENOBUFS), > + ERRORCODE(EISCONN), > + ERRORCODE(ENOTCONN), > + ERRORCODE(ESHUTDOWN), > + ERRORCODE(ETOOMANYREFS), > + ERRORCODE(ETIMEDOUT), > + ERRORCODE(ECONNREFUSED), > + ERRORCODE(EHOSTDOWN), > + ERRORCODE(EHOSTUNREACH), > + ERRORCODE(EALREADY), > + ERRORCODE(EINPROGRESS), > + ERRORCODE(ESTALE), > + ERRORCODE(EUCLEAN), > + ERRORCODE(ENOTNAM), > + ERRORCODE(ENAVAIL), > + ERRORCODE(EISNAM), > + ERRORCODE(EREMOTEIO), > + ERRORCODE(EDQUOT), > + ERRORCODE(ENOMEDIUM), > + ERRORCODE(EMEDIUMTYPE), > + ERRORCODE(ECANCELED), > + ERRORCODE(ENOKEY), > + ERRORCODE(EKEYEXPIRED), > + ERRORCODE(EKEYREVOKED), > + ERRORCODE(EKEYREJECTED), > + ERRORCODE(EOWNERDEAD), > + ERRORCODE(ENOTRECOVERABLE), > + ERRORCODE(ERFKILL), > + ERRORCODE(EHWPOISON), > + ERRORCODE(ERESTARTSYS), > + ERRORCODE(ERESTARTNOINTR), > + ERRORCODE(ERESTARTNOHAND), > + ERRORCODE(ENOIOCTLCMD), > + ERRORCODE(ERESTART_RESTARTBLOCK), > + ERRORCODE(EPROBE_DEFER), > + ERRORCODE(EOPENSTALE), > + ERRORCODE(ENOPARAM), > + ERRORCODE(EBADHANDLE), > + ERRORCODE(ENOTSYNC), > + ERRORCODE(EBADCOOKIE), > + ERRORCODE(ENOTSUPP), > + ERRORCODE(ETOOSMALL), > + ERRORCODE(ESERVERFAULT), > + ERRORCODE(EBADTYPE), > + ERRORCODE(EJUKEBOX), > + ERRORCODE(EIOCBQUEUED), > + 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; > + } > + } > + > + 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); > + > + 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); > + } else { > + str = number(str, end, num, spec); > + } > } > } > > -- > 2.20.1 >
From: Enrico Weigelt, metux IT consult > Sent: 26 August 2019 14:29 > On 25.08.19 01:37, Uwe Kleine-König wrote: > > Hi, > > > +static noinline_for_stack > +char *errstr(char *buf, char *end, unsigned long long num,> + > struct printf_spec spec)> +{ > #1: why not putting that into some separate strerror() lib function ? > This is something I've been looking for quite some time (actually > already hacked it up somewhere, sometime, but forgotten ...) > > #2: why not just having a big case statement and leave the actual lookup > logic to the compiler ? IMHO, could be written in a very compact way > by some macro magic And generate an enormous amount of code and long chains of mispredicted branches. Is it also worth looking at how long the strings are. If they can be truncated to 16 bytes then char[][16] will generate much better code than the array of pointers. OTOH I'm not really sure it is all a good idea. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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(ENOENT), + ERRORCODE(ESRCH), + ERRORCODE(EINTR), + ERRORCODE(EIO), + ERRORCODE(ENXIO), + ERRORCODE(E2BIG), + ERRORCODE(ENOEXEC), + ERRORCODE(EBADF), + ERRORCODE(ECHILD), + ERRORCODE(EAGAIN), + ERRORCODE(ENOMEM), + ERRORCODE(EACCES), + ERRORCODE(EFAULT), + ERRORCODE(ENOTBLK), + ERRORCODE(EBUSY), + ERRORCODE(EEXIST), + ERRORCODE(EXDEV), + ERRORCODE(ENODEV), + ERRORCODE(ENOTDIR), + ERRORCODE(EISDIR), + ERRORCODE(EINVAL), + ERRORCODE(ENFILE), + ERRORCODE(EMFILE), + ERRORCODE(ENOTTY), + ERRORCODE(ETXTBSY), + ERRORCODE(EFBIG), + ERRORCODE(ENOSPC), + ERRORCODE(ESPIPE), + ERRORCODE(EROFS), + ERRORCODE(EMLINK), + ERRORCODE(EPIPE), + ERRORCODE(EDOM), + ERRORCODE(ERANGE), + ERRORCODE(EDEADLK), + ERRORCODE(ENAMETOOLONG), + ERRORCODE(ENOLCK), + ERRORCODE(ENOSYS), + ERRORCODE(ENOTEMPTY), + ERRORCODE(ELOOP), + ERRORCODE(EWOULDBLOCK), + ERRORCODE(ENOMSG), + ERRORCODE(EIDRM), + ERRORCODE(ECHRNG), + ERRORCODE(EL2NSYNC), + ERRORCODE(EL3HLT), + ERRORCODE(EL3RST), + ERRORCODE(ELNRNG), + ERRORCODE(EUNATCH), + ERRORCODE(ENOCSI), + ERRORCODE(EL2HLT), + ERRORCODE(EBADE), + ERRORCODE(EBADR), + ERRORCODE(EXFULL), + ERRORCODE(ENOANO), + ERRORCODE(EBADRQC), + ERRORCODE(EBADSLT), + ERRORCODE(EBFONT), + ERRORCODE(ENOSTR), + ERRORCODE(ENODATA), + ERRORCODE(ETIME), + ERRORCODE(ENOSR), + ERRORCODE(ENONET), + ERRORCODE(ENOPKG), + ERRORCODE(EREMOTE), + ERRORCODE(ENOLINK), + ERRORCODE(EADV), + ERRORCODE(ESRMNT), + ERRORCODE(ECOMM), + ERRORCODE(EPROTO), + ERRORCODE(EMULTIHOP), + ERRORCODE(EDOTDOT), + ERRORCODE(EBADMSG), + ERRORCODE(EOVERFLOW), + ERRORCODE(ENOTUNIQ), + ERRORCODE(EBADFD), + ERRORCODE(EREMCHG), + ERRORCODE(ELIBACC), + ERRORCODE(ELIBBAD), + ERRORCODE(ELIBSCN), + ERRORCODE(ELIBMAX), + ERRORCODE(ELIBEXEC), + ERRORCODE(EILSEQ), + ERRORCODE(ERESTART), + ERRORCODE(ESTRPIPE), + ERRORCODE(EUSERS), + ERRORCODE(ENOTSOCK), + ERRORCODE(EDESTADDRREQ), + ERRORCODE(EMSGSIZE), + ERRORCODE(EPROTOTYPE), + ERRORCODE(ENOPROTOOPT), + ERRORCODE(EPROTONOSUPPORT), + ERRORCODE(ESOCKTNOSUPPORT), + ERRORCODE(EOPNOTSUPP), + ERRORCODE(EPFNOSUPPORT), + ERRORCODE(EAFNOSUPPORT), + ERRORCODE(EADDRINUSE), + ERRORCODE(EADDRNOTAVAIL), + ERRORCODE(ENETDOWN), + ERRORCODE(ENETUNREACH), + ERRORCODE(ENETRESET), + ERRORCODE(ECONNABORTED), + ERRORCODE(ECONNRESET), + ERRORCODE(ENOBUFS), + ERRORCODE(EISCONN), + ERRORCODE(ENOTCONN), + ERRORCODE(ESHUTDOWN), + ERRORCODE(ETOOMANYREFS), + ERRORCODE(ETIMEDOUT), + ERRORCODE(ECONNREFUSED), + ERRORCODE(EHOSTDOWN), + ERRORCODE(EHOSTUNREACH), + ERRORCODE(EALREADY), + ERRORCODE(EINPROGRESS), + ERRORCODE(ESTALE), + ERRORCODE(EUCLEAN), + ERRORCODE(ENOTNAM), + ERRORCODE(ENAVAIL), + ERRORCODE(EISNAM), + ERRORCODE(EREMOTEIO), + ERRORCODE(EDQUOT), + ERRORCODE(ENOMEDIUM), + ERRORCODE(EMEDIUMTYPE), + ERRORCODE(ECANCELED), + ERRORCODE(ENOKEY), + ERRORCODE(EKEYEXPIRED), + ERRORCODE(EKEYREVOKED), + ERRORCODE(EKEYREJECTED), + ERRORCODE(EOWNERDEAD), + ERRORCODE(ENOTRECOVERABLE), + ERRORCODE(ERFKILL), + ERRORCODE(EHWPOISON), + ERRORCODE(ERESTARTSYS), + ERRORCODE(ERESTARTNOINTR), + ERRORCODE(ERESTARTNOHAND), + ERRORCODE(ENOIOCTLCMD), + ERRORCODE(ERESTART_RESTARTBLOCK), + ERRORCODE(EPROBE_DEFER), + ERRORCODE(EOPENSTALE), + ERRORCODE(ENOPARAM), + ERRORCODE(EBADHANDLE), + ERRORCODE(ENOTSYNC), + ERRORCODE(EBADCOOKIE), + ERRORCODE(ENOTSUPP), + ERRORCODE(ETOOSMALL), + ERRORCODE(ESERVERFAULT), + ERRORCODE(EBADTYPE), + ERRORCODE(EJUKEBOX), + ERRORCODE(EIOCBQUEUED), + 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; + } + } + + 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); + + 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); + } else { + str = number(str, end, num, spec); + } } }
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(-)