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 [off-list ref] wrote:

quoted

	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.

quoted

Signed-off-by: Uwe Kleine-König <redacted>
---
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.

quoted

--- 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.

quoted

+	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.
 

quoted

+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.

quoted

+	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