[PATCH v2] vsprintf: introduce %dE for error constants

[PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

The new format specifier %dE introduced with this patch pretty-prints
the typical negative error values. So

	pr_info("probing failed (%dE)\n", ret);

yields

	probing failed (EIO)

if ret holds -EIO. This is easier to understand than the for now common

	probing failed (-5)

(which used %d instead of %dE) for kernel developers who don't know the
numbers by heart, The error names are also known and understood by
userspace coders as they match the values the errno variable can have.
Also to debug the sitation that resulted in the above message very
probably involves EIO, so the message already gives the string to look
out for.

glibc (and other libc variants)'s printf have a similar feature: %m
expands to strerror(errno). I preferred using %dE however because %m in
userspace doesn't consume an argument (which is however necessary in the
kernel as there is no global errno variable). Also this is handled
correctly by the compiler's format string checker as there is a matching
int variable for the %d the compiler notices.

There are quite some code locations that could be adapted to benefit
from this:

	$ 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' v5.3-rc5 | wc -l
	9141

I expect there are some false negatives where the match is distributed
over two or more lines and so isn't found.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

in v1 I handled both positive and negative errors which was challenged.
I decided to drop that and only handle the (common) negative values.
Readding handling of positive values later is easier than dropping it.

Sergey Senozhatsky and Enrico Weigelt suggested to use strerror as name
for the function that I called errno2errstr. (In v1 it was not a
separate function). I didn't pick this up because the semantic of
strerror in userspace is different. It returns something like
"Interrupted system call" while errno2errstr only yields "EINTR".

I dropped EWOULDBLOCK from the list of codes as it is on all Linux archs
identical to EAGAIN and the way the lookup works now breaks otherwise.
(And having it wasn't useful already in v1.)

Rasmus Villemoes pointed out that the way I wrote the resulting string
to the buffer was broken. This is fixed according to his suggestion by
using string_nocheck(). I also added a test to lib/test_printf.c as he
requested.

Petr Mladek had some concerns:
> The array is long, created by cpu&paste, the index of each code
> is not obvious.

Yeah right, the array is long. This cannot really be changed because we
have that many error codes. I don't understand your concern about the
index not being obvious. The array was just a list of (number, string)
pairs where the position in the array didn't have any semantic.

I agree it would be nice to have only one place that has a collection of
error codes. I thought about reorganizing how the error constants are
defined, i.e. doing something like:

        DEFINE_ERROR(EIO, 5, "I/O error")

but I don't see a possibility to let this expand to

        #define EIO 5

without resorting to another macro language. If that were possible the
list of DEFINE_ERRORs could be reused to help generating the code for
errno2errstr. So if you have a good idea, please speak up.

> There are ideas to make the code even more tricky to reduce
> the size, keep it fast.

I think Enrico Weigelt's suggestion to use a case is the best
performance-wise so that's what I picked up. Also I hope that
performance isn't that important because the need to print an error
should not be so common that it really hurts in production.

> Both, %dE modifier and the output format (ECODE) is non-standard.

Yeah, obviously right. The problem is that the new modifier does
something that wasn't implemented before, so it cannot match any
standard. %pI is only known on Linux either, so I think being
non-standard is a weak argument. For user consumption it would be nice
if we'd get

        probing failed (I/O Error)

from pr_info("probing failed (%dE)\n", -EIO); but that would be still
more expensive because the collection of string constants would become
bigger that it is already now and "EIO" is the right one to search for
when debugging the underlaying problem. So I think "EIO" is even more
useful than "I/O Error".

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

This isn't really an argument against the patch I think. Sure, if a
function returned (say) EIO while ETIMEOUT would be better, my patch
doesn't improve that detail. Still

        mydev: Failed to initialize blablub: EIO

is more expressive than

        mydev: Failed to initialize blablub: -5

. With "EIO" in the message it is not worse than with "-5" independant of the
question if EIO is the right error code used.

> 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 don't intend to mass convert existing code. I would restrict myself to
updating the documentation and then maybe send a patch per subsystem as an
example to let maintainers know and judge for themselves if they like it or
not. And if it doesn't get picked up, we can just remove the feature again next
year (or so).

I dropped the example conversion, I think the idea should be clear now
even without an explicit example.

Best regards
Uwe

---
 Documentation/core-api/printk-formats.rst |   3 +
 lib/test_printf.c                         |   1 +
 lib/vsprintf.c                            | 188 +++++++++++++++++++++-
 3 files changed, 191 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/test_printf.c b/lib/test_printf.c
index 944eb50f3862..9b0687928717 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -174,6 +174,7 @@ test_number(void)
 		test("0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val);
 	}
 #endif
+	test("EIO", "%dE", -EIO);
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..8fed03610402 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -682,6 +682,187 @@ static char *pointer_string(char *buf, char *end,
 	return number(buf, end, (unsigned long int)ptr, spec);
 }
 
+#define 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(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)
+
+#define ERRORCODE(x) 	case x: return #x;
+
+static const char *errint2errstr(int err)
+{
+	switch(-err) {
+	ERRORCODES
+	}
+	return NULL;
+}
+#undef ERRORCODE
+
+static noinline_for_stack
+char *errstr(char *buf, char *end, unsigned long long num,
+	     struct printf_spec spec)
+{
+	const char *errstr = errint2errstr(num);
+	static const struct printf_spec strspec = {
+		.field_width = -1,
+		.precision = 10,
+		.flags = LEFT,
+	};
+
+	if (!errstr) {
+		/* fall back to ordinary number */
+		return number(buf, end, num, spec);
+	}
+
+	return string_nocheck(buf, end, errstr, strspec);
+}
+
 /* Make pointers available for printing early in the boot sequence. */
 static int debug_boot_weak_hash __ro_after_init;
 
@@ -2566,7 +2747,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.23.0

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

Hello,

I fat-fingered Enrico Weigelt's e-mail address. Please when you reply to
my patch fix that up (i.e. drop Enrico@kleine-koenig.org and
Weigelt@kleine-koenig.org).

Thanks and sorry,
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Joe Perches]

On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
> 
> 	pr_info("probing failed (%dE)\n", ret);
> 
> yields
> 
> 	probing failed (EIO)
> 
> if ret holds -EIO. This is easier to understand than the for now common
> 
> 	probing failed (-5)

I suggest using both outputs like '-5 -EIO'
rather than a single string

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]

Hello Joe,

On 8/27/19 11:22 PM, Joe Perches wrote:
> On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
>> The new format specifier %dE introduced with this patch pretty-prints
>> the typical negative error values. So
>>
>> 	pr_info("probing failed (%dE)\n", ret);
>>
>> yields
>>
>> 	probing failed (EIO)
>>
>> if ret holds -EIO. This is easier to understand than the for now common
>>
>> 	probing failed (-5)
> 
> I suggest using both outputs like '-5 -EIO'
> rather than a single string

I like it the way it is implemented as it is more flexible. If you want
to see both, you can still do

	pr_info("probing failed (%d %dE)\n", ret, ret);

and people (like me) who think that giving only EIO can still do just that.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Joe Perches]

On Tue, 2019-08-27 at 23:35 +0200, Uwe Kleine-König wrote:
> Hello Joe,
> 
> On 8/27/19 11:22 PM, Joe Perches wrote:
> > On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
> > > The new format specifier %dE introduced with this patch pretty-prints
> > > the typical negative error values. So
> > > 
> > > 	pr_info("probing failed (%dE)\n", ret);
> > > 
> > > yields
> > > 
> > > 	probing failed (EIO)
> > > 
> > > if ret holds -EIO. This is easier to understand than the for now common
> > > 
> > > 	probing failed (-5)
> > 
> > I suggest using both outputs like '-5 -EIO'
> > rather than a single string
> 
> I like it the way it is implemented as it is more flexible. If you want
> to see both, you can still do
> 
> 	pr_info("probing failed (%d %dE)\n", ret, ret);
> 
> and people (like me) who think that giving only EIO can still do just that.

<shrug>  Up to you.  Just a suggestion.

btw:

The test for %<dixu>E (FORMAT_TYPE_INT)
should probably include a test for

(spec->flags & SIGN)

so that it only is used for %d and %i and
disregarded for %x and %u

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Petr Mladek]

On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
> 
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.

I missed that the number was stored in the array as well. I somehow
expected that it was array of strings.


> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
> 
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.

I personally do not like switch/case. It is a lot of code.
I wonder if it even saved some space.

If you want to safe space, I would use u16 to store the numbers.
Or I would use array of strings. There will be only few holes.

You might also consider handling only the most commonly
used codes from errno.h and errno-base.h (1..133). There will
be no holes and the codes are stable.


> > Both, %dE modifier and the output format (ECODE) is non-standard.
> 
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument.

I am not completely sure that %p modifiers were a good idea.
They came before I started maintaining printk(). They add more
complex algorithms into paths where we could not report problems
easily (printk recursion). Also they are causing problems with
unit testing that might be done in userspace. These non-standard
formats cause that printk() can't be simply substituted by printf().

I am not keen to spread these problems over more formats.
Also %d format is more complicated. It is often used with
already existing modifiers.


> > 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.
> 
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
>         mydev: Failed to initialize blablub: EIO
>
> is more expressive than
> 
>         mydev: Failed to initialize blablub: -5

OK, upper letters probably are not a problem.

But what about EWOULDBLOCK and EDEADLOCK? They have the same
error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
People might spend a lot of time searching for EAGAIN before they
notice that EWOULDBLOCK was used in the code instead.

Also you still did not answer the question where the idea came from.
Did it just look nice? Anyone asked for it? Who? Why?


> > 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 don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).

It looks like a lot of potentially useless work.


> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.

Please, do the opposite. Add conversion of few subsystems into the
patchset and add more people into CC. We will see immediately whether
it makes sense to spend time on this.

I personally think that this feature is not worth the code, data,
and bikeshedding.

Best Regards,
Petr

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Jani Nikula]

On Wed, 28 Aug 2019, Petr Mladek <pmladek@suse.com> wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
>> Petr Mladek had some concerns:
>> > The array is long, created by cpu&paste, the index of each code
>> > is not obvious.
>> 
>> Yeah right, the array is long. This cannot really be changed because we
>> have that many error codes. I don't understand your concern about the
>> index not being obvious. The array was just a list of (number, string)
>> pairs where the position in the array didn't have any semantic.
>
> I missed that the number was stored in the array as well. I somehow
> expected that it was array of strings.
>
>
>> > There are ideas to make the code even more tricky to reduce
>> > the size, keep it fast.
>> 
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
>
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.
>
> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
>
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.
>
>
>> > Both, %dE modifier and the output format (ECODE) is non-standard.
>> 
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
>
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().
>
> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.
>
>
>> > 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.
>> 
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>>         mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>> 
>>         mydev: Failed to initialize blablub: -5
>
> OK, upper letters probably are not a problem.
>
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.
>
> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?
>
>
>> > 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 don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
>
> It looks like a lot of potentially useless work.
>
>
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
>
> I personally think that this feature is not worth the code, data,
> and bikeshedding.

The obvious alternative, I think already mentioned, is to just add
strerror() or similar as a function. I doubt there'd be much opposition
to that. Folks could use %s and strerr(ret). And a follow-up could add
the special format specifier if needed.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Sergey Senozhatsky]

On (08/28/19 14:54), Jani Nikula wrote:
[..]
> > I personally think that this feature is not worth the code, data,
> > and bikeshedding.
> 
> The obvious alternative, I think already mentioned, is to just add
> strerror() or similar as a function. I doubt there'd be much opposition
> to that. Folks could use %s and strerr(ret). And a follow-up could add
> the special format specifier if needed.

Yeah, I'd say that strerror() would be a better alternative
to vsprintf() specifier. (if we decide to add such functionality).

	-ss

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Rasmus Villemoes]

On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> On (08/28/19 14:54), Jani Nikula wrote:
> [..]
>>> I personally think that this feature is not worth the code, data,
>>> and bikeshedding.
>>
>> The obvious alternative, I think already mentioned, is to just add
>> strerror() or similar as a function. I doubt there'd be much opposition
>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>> the special format specifier if needed.
> 
> Yeah, I'd say that strerror() would be a better alternative
> to vsprintf() specifier. (if we decide to add such functionality).

Please no. The .text footprint of the changes at the call sites to do
pr_err("...%s...", errcode(err)) instead of the current
pr_err("...%d...", err) would very soon dwarf whatever is necessary to
implement %pE or %dE. Also that would prevent any kind of graceful
fallback in case err doesn't have a known value - errcode() would have
to return some fixed "huh, unknown error number" string, so we'd lose
the actual number.

Rasmus

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Sergey Senozhatsky]

On (08/28/19 14:49), Rasmus Villemoes wrote:
> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> > On (08/28/19 14:54), Jani Nikula wrote:
> > [..]
> >>> I personally think that this feature is not worth the code, data,
> >>> and bikeshedding.
> >>
> >> The obvious alternative, I think already mentioned, is to just add
> >> strerror() or similar as a function. I doubt there'd be much opposition
> >> to that. Folks could use %s and strerr(ret). And a follow-up could add
> >> the special format specifier if needed.
> > 
> > Yeah, I'd say that strerror() would be a better alternative
> > to vsprintf() specifier. (if we decide to add such functionality).
> 
> Please no. The .text footprint of the changes at the call sites to do
> pr_err("...%s...", errcode(err)) instead of the current
> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
> implement %pE or %dE.

New vsprintf() specifiers have some downsides as well. Should %dE
accidentally (via backport) make it to the -stable kernel, which
does not support %dE, and we are going to lose the actual error
code value as well.

	-ss

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1878 bytes --]

On 8/28/19 2:59 PM, Sergey Senozhatsky wrote:
> On (08/28/19 14:49), Rasmus Villemoes wrote:
>> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
>>> On (08/28/19 14:54), Jani Nikula wrote:
>>> [..]
>>>>> I personally think that this feature is not worth the code, data,
>>>>> and bikeshedding.
>>>>
>>>> The obvious alternative, I think already mentioned, is to just add
>>>> strerror() or similar as a function. I doubt there'd be much opposition
>>>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>>>> the special format specifier if needed.
>>>
>>> Yeah, I'd say that strerror() would be a better alternative
>>> to vsprintf() specifier. (if we decide to add such functionality).
>>
>> Please no. The .text footprint of the changes at the call sites to do
>> pr_err("...%s...", errcode(err)) instead of the current
>> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
>> implement %pE or %dE.

Yeah, that's what I think, too. I cannot imagine a user of strerror()
who needs the string representation for something different than to feed
it to one of the family members of printk. That's also why I think that
the other already existing format specifier are a good idea.

It might not be the nicest part of the printk code, but this way it is
at least concentrated in one place only.

> New vsprintf() specifiers have some downsides as well. Should %dE
> accidentally (via backport) make it to the -stable kernel, which
> does not support %dE, and we are going to lose the actual error
> code value as well.

That is wrong. When you do

	pr_err("There are no round tuits to give out: %dE\n", -ENOENT);

in a kernel that doesn't support %dE you get:

	There are no round tuits to give out: -2E

That's a bit ugly but I can still work out what the original value was.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5889 bytes --]

Hello Petr,

On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
>> Petr Mladek had some concerns:
>>> There are ideas to make the code even more tricky to reduce
>>> the size, keep it fast.
>>
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
> 
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.

I guess we have to die either way. Either it is quick or it is space
efficient. With the big case I trust the compiler to pick something
sensible expecting that it adapts for example to -Osize.

> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
> 
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.

I'd like to postpone the discussion about "how" until we agreed about
the "if at all".

>>> Both, %dE modifier and the output format (ECODE) is non-standard.
>>
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
> 
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().

In my eyes the wish to have printk() and userspace's printf
feature-identical isn't helpful because they have similar but not equal
purposes. And if a driver author cares about being able to use their
code 1:1 in userspace they could just not use %dE, %pI and whatever
there is additionally.

> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.

I don't understand what you want to tell me here.

>>> 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.
>>
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>>         mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>>
>>         mydev: Failed to initialize blablub: -5
> 
> OK, upper letters probably are not a problem.
> 
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
>
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.

It already does now. If you start to debug an error message that tells
you the error is -11, you might well come to the conclusion you have to
look for EDEADLK. IMHO the right approach here should be to declare one
of the duplicates as the "right" one and replace the wrong one in the
complete tree (apart from the definition for user space of course).
After that the problem with these duplicates is completely orthogonal
(it is already now mostly orthogonal in my eyes) and also solved for
those who picked the wrong one by hand.

> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?

It was my idea, and I didn't talk about it before creating a patch. You
asked in reply to v1: "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?"

So yes, having to lookup the right error code from messages is something
that annoys me, especially because there are at least two files you have
to check for the definition. I consider myself to have all three hats
(developer, supporter and user), and your feedback set apart my
impression is that all people replying to this thread consider it a good
idea, too.

>>> 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 don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
> 
> It looks like a lot of potentially useless work.

If the idea will not gain speed, removing the then few users is straight
forward (just a question of calling sed s/%dE/%d/ on all affected
files). So at least it shouldn't be your useless work and time.

>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
> 
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
> 
> I personally think that this feature is not worth the code, data,
> and bikeshedding.

I agree about the bike shedding ;-)

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1292 bytes --]

On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
> 
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.

For now I asked in the arm linux irc channel and got two people replying
(both added to Cc:):

Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:

1567019926 < broonie> ukleinek: I think that's a great idea and have
	thought about trying to implement it in the past.
1567019937 < broonie> ukleinek: Making the logs more directly readable
	is enormously helpful.

and Alexandre Belloni (arm/at91, mips/microsemi, rtc) said:

1567021451 < abelloni> ukleinek: seems good to me but it would probably
	be better to be able to generate the list

(I fully agree to the wish to generate the list, as I already wrote
before, I don't have a good idea how to do that without generating
C-Code by some means which is ugly and also complicated by the fact that
there are several locations (at least for now) that have definitions for
error codes.)

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On Wed, Aug 28, 2019 at 09:59:16PM +0200, Uwe Kleine-König wrote:
> On 8/28/19 1:32 PM, Petr Mladek wrote:

> > Please, do the opposite. Add conversion of few subsystems into the
> > patchset and add more people into CC. We will see immediately whether
> > it makes sense to spend time on this.

> For now I asked in the arm linux irc channel and got two people replying
> (both added to Cc:):

> Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:

> 1567019926 < broonie> ukleinek: I think that's a great idea and have
> 	thought about trying to implement it in the past.
> 1567019937 < broonie> ukleinek: Making the logs more directly readable
> 	is enormously helpful.

In the past I've actually implemented error code decoders like this for
some other systems I've worked on due to annoyance with having to look
up codes, as far as I can tell they went over quite well with other
developers and I certainly found them helpful myself.  They're also
useful to end users looking at logs, they're not always aware of how to
find the relevant error code definitions so a slightly more descriptive
messages can help people figure things out.

It does also occur to me that this might be useful to the people who
want to work on making probe deferral quieter since it gives them an
annotation that the number going in is an error code.  There's a bunch
more work to do there though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Sergey Senozhatsky]

On (08/28/19 18:22), Uwe Kleine-König wrote:
> That is wrong. When you do
> 
> 	pr_err("There are no round tuits to give out: %dE\n", -ENOENT);
> 
> in a kernel that doesn't support %dE you get:
> 
> 	There are no round tuits to give out: -2E

OK. Good point.

	-ss

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Petr Mladek]

On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:
> Hello Petr,
> 
> On 8/28/19 1:32 PM, Petr Mladek wrote:
> > On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
> >> Petr Mladek had some concerns:
> >>> There are ideas to make the code even more tricky to reduce
> >>> the size, keep it fast.
> >>
> >> I think Enrico Weigelt's suggestion to use a case is the best
> >> performance-wise so that's what I picked up. Also I hope that
> >> performance isn't that important because the need to print an error
> >> should not be so common that it really hurts in production.

This is contadicting. The "best" performance-wise solution was
choosen in favor of space. The next sentence says that performance
is not important.

> > I personally do not like switch/case. It is a lot of code.
> > I wonder if it even saved some space.
> 
> I guess we have to die either way. Either it is quick or it is space
> efficient.

I am more concerned about the size. Well, array of strings will
be both fast and size efficient.

> With the big case I trust the compiler to pick something
> sensible expecting that it adapts for example to -Osize.

I am not sure what are the expectations here. I can't imagine
another translation than:

   if (val == 1)
     str = "EPERM";
   else if (val == 2)
     str = "ENOENT"
   else if (val == 3)
     str = "ESRCH"
   ...

It means that all constans will be hardcoded in the code instead
of in data section. Plus there will be instructions for each
if/else part.

> > If you want to safe space, I would use u16 to store the numbers.
> > Or I would use array of strings. There will be only few holes.
> > 
> > You might also consider handling only the most commonly
> > used codes from errno.h and errno-base.h (1..133). There will
> > be no holes and the codes are stable.
> 
> I'd like to postpone the discussion about "how" until we agreed about
> the "if at all".

It seems that all people like this feature.

BTW: I though more about generating or cut&pasting the arrary.
I can't find any reasonable way how to generate it.

But both, errno.h and errno-base.h, are super stable. Only
comments were modified or new codes added. Most of them
are defined by POSIX so they should remain stable.

Therefore cut&pasted array of strings looks acceptable.
We should only allow to easily check numbers for each code,
e.g. by defining the array as

const err_str * [] {
	"0"			/*   0  Success */
	"EPERM",		/*   1	Operation not permitted */
	"ENOENT",		/*   2	No such file or directory */
	"ESRCH",		/*   3	No such process */
	...


If there is a hole, we could use something like:

	"-41",			/*  41  Skipped. EWOULDBLOCK is
	defined as EAGAIN.  Operation would block */

Best Regards,
Petr

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Juergen Gross]

On 29.08.19 10:12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:
>> Hello Petr,
>>
>> On 8/28/19 1:32 PM, Petr Mladek wrote:
>>> On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
>>>> Petr Mladek had some concerns:
>>>>> There are ideas to make the code even more tricky to reduce
>>>>> the size, keep it fast.
>>>>
>>>> I think Enrico Weigelt's suggestion to use a case is the best
>>>> performance-wise so that's what I picked up. Also I hope that
>>>> performance isn't that important because the need to print an error
>>>> should not be so common that it really hurts in production.
> 
> This is contadicting. The "best" performance-wise solution was
> choosen in favor of space. The next sentence says that performance
> is not important.
> 
>>> I personally do not like switch/case. It is a lot of code.
>>> I wonder if it even saved some space.
>>
>> I guess we have to die either way. Either it is quick or it is space
>> efficient.
> 
> I am more concerned about the size. Well, array of strings will
> be both fast and size efficient.
> 
>> With the big case I trust the compiler to pick something
>> sensible expecting that it adapts for example to -Osize.
> 
> I am not sure what are the expectations here. I can't imagine
> another translation than:
> 
>     if (val == 1)
>       str = "EPERM";
>     else if (val == 2)
>       str = "ENOENT"
>     else if (val == 3)
>       str = "ESRCH"
>     ...
> 
> It means that all constans will be hardcoded in the code instead
> of in data section. Plus there will be instructions for each
> if/else part.
> 
>>> If you want to safe space, I would use u16 to store the numbers.
>>> Or I would use array of strings. There will be only few holes.
>>>
>>> You might also consider handling only the most commonly
>>> used codes from errno.h and errno-base.h (1..133). There will
>>> be no holes and the codes are stable.
>>
>> I'd like to postpone the discussion about "how" until we agreed about
>> the "if at all".
> 
> It seems that all people like this feature.

Hmm, what about already existing format strings conatining "%dE"?

Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
nevertheless...

> 
> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.

Generate the array and errno.h/errno-base.h from the same source?


Juergen

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Rasmus Villemoes]

On 29/08/2019 10.12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:

> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.

Something like this seems to work, though it probably needs some massage
to be accepted by kbuild folks:

define filechk_errcode.h
	echo '#include <linux/errno.h>' | \
	$(CPP) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) -dM - | \
	grep 'define E' | sort -k3,3n | \
	awk '{print "errcode("$$2")\t/* "$$3" */"}'
endef

include/generated/errcode.h: FORCE
	$(call filechk,errcode.h)

Then one can just #define errcode(foo) ... right before #include
<generated/errcode.h>. It cannot be used to generate cases in a switch()
because some expand to the same number, but that's ok, because I can't
imagine the switch actually generating good or small code. I haven't
checked how or whether it works in a cross-compile situation.

Rasmus

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Rasmus Villemoes]

On 29/08/2019 10.27, Juergen Gross wrote:
> On 29.08.19 10:12, Petr Mladek wrote:
>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:
>>>
>>> I'd like to postpone the discussion about "how" until we agreed about
>>> the "if at all".
>>
>> It seems that all people like this feature.
> 
> Hmm, what about already existing format strings conatining "%dE"?
> 
> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
> nevertheless...

Indeed, Uwe still needs to respond to how he wants to handle that. I
still prefer making it %pE, both because it's easier to convert integers
to ERR_PTRs than having to worry about the type of PTR_ERR() being long
and not int, and because alphanumerics after %p have been ignored for a
long time (10 years?) whether or not those characters have been
recognized as a %p extension, so nobody relies on %pE putting an E after
the %p output. It also keeps the non-standard extensions in the same
"namespace", so to speak.

Oh, 'E' is taken, well, make it 'e' then.

Rasmus

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Andy Shevchenko]

On Wed, Aug 28, 2019 at 12:14 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
>
>         pr_info("probing failed (%dE)\n", ret);
>
> yields
>
>         probing failed (EIO)
>
> if ret holds -EIO. This is easier to understand than the for now common
>
>         probing failed (-5)
>
> (which used %d instead of %dE) for kernel developers who don't know the
> numbers by heart, The error names are also known and understood by
> userspace coders as they match the values the errno variable can have.
> Also to debug the sitation that resulted in the above message very
> probably involves EIO, so the message already gives the string to look
> out for.
>
> glibc (and other libc variants)'s printf have a similar feature: %m
> expands to strerror(errno). I preferred using %dE however because %m in
> userspace doesn't consume an argument (which is however necessary in the
> kernel as there is no global errno variable). Also this is handled
> correctly by the compiler's format string checker as there is a matching
> int variable for the %d the compiler notices.
>
> There are quite some code locations that could be adapted to benefit
> from this:
>
>         $ 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' v5.3-rc5 | wc -l
>         9141
>
> I expect there are some false negatives where the match is distributed
> over two or more lines and so isn't found.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
>
> in v1 I handled both positive and negative errors which was challenged.
> I decided to drop that and only handle the (common) negative values.
> Readding handling of positive values later is easier than dropping it.
>
> Sergey Senozhatsky and Enrico Weigelt suggested to use strerror as name
> for the function that I called errno2errstr. (In v1 it was not a
> separate function). I didn't pick this up because the semantic of
> strerror in userspace is different. It returns something like
> "Interrupted system call" while errno2errstr only yields "EINTR".
>
> I dropped EWOULDBLOCK from the list of codes as it is on all Linux archs
> identical to EAGAIN and the way the lookup works now breaks otherwise.
> (And having it wasn't useful already in v1.)
>
> Rasmus Villemoes pointed out that the way I wrote the resulting string
> to the buffer was broken. This is fixed according to his suggestion by
> using string_nocheck(). I also added a test to lib/test_printf.c as he
> requested.
>
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
>
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.
>
> I agree it would be nice to have only one place that has a collection of
> error codes. I thought about reorganizing how the error constants are
> defined, i.e. doing something like:
>
>         DEFINE_ERROR(EIO, 5, "I/O error")
>
> but I don't see a possibility to let this expand to
>
>         #define EIO 5
>
> without resorting to another macro language. If that were possible the
> list of DEFINE_ERRORs could be reused to help generating the code for
> errno2errstr. So if you have a good idea, please speak up.
>
> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
>
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.
>
> > Both, %dE modifier and the output format (ECODE) is non-standard.
>
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument. For user consumption it would be nice
> if we'd get
>
>         probing failed (I/O Error)
>
> from pr_info("probing failed (%dE)\n", -EIO); but that would be still
> more expensive because the collection of string constants would become
> bigger that it is already now and "EIO" is the right one to search for
> when debugging the underlaying problem. So I think "EIO" is even more
> useful than "I/O Error".
>
> > 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.
>
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
>         mydev: Failed to initialize blablub: EIO
>
> is more expressive than
>
>         mydev: Failed to initialize blablub: -5
>
> . With "EIO" in the message it is not worse than with "-5" independant of the
> question if EIO is the right error code used.
>
> > 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 don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).
>
> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.
>

Hold on, can you in less than 20 words put WHY this is needed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1404 bytes --]

On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
> On 29/08/2019 10.27, Juergen Gross wrote:
>> On 29.08.19 10:12, Petr Mladek wrote:
>>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:
>>>>
>>>> I'd like to postpone the discussion about "how" until we agreed about
>>>> the "if at all".
>>>
>>> It seems that all people like this feature.
>>
>> Hmm, what about already existing format strings conatining "%dE"?
>>
>> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
>> nevertheless...
> 
> Indeed, Uwe still needs to respond to how he wants to handle that. I

This is indeed bad and I didn't expect that. I just took a quick look
and this string is indeed used as sprintf format string.

> still prefer making it %pE, both because it's easier to convert integers
> to ERR_PTRs than having to worry about the type of PTR_ERR() being long
> and not int, and because alphanumerics after %p have been ignored for a
> long time (10 years?) whether or not those characters have been
> recognized as a %p extension, so nobody relies on %pE putting an E after
> the %p output. It also keeps the non-standard extensions in the same
> "namespace", so to speak.
> 
> Oh, 'E' is taken, well, make it 'e' then.

I like having %pe to print error valued pointers. Then maybe we could
have both %de for ints and %pe for pointers. :-)

Best regards
Uwe




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Petr Mladek]

On Thu 2019-08-29 19:39:45, Uwe Kleine-König  wrote:
> On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
> > On 29/08/2019 10.27, Juergen Gross wrote:
> >> On 29.08.19 10:12, Petr Mladek wrote:
> >>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König  wrote:
> >>>>
> >>>> I'd like to postpone the discussion about "how" until we agreed about
> >>>> the "if at all".
> >>>
> >>> It seems that all people like this feature.
> >>
> >> Hmm, what about already existing format strings conatining "%dE"?
> >>
> >> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
> >> nevertheless...
> > 
> > Indeed, Uwe still needs to respond to how he wants to handle that. I
> 
> This is indeed bad and I didn't expect that. I just took a quick look
> and this string is indeed used as sprintf format string.

Hmm, it seems that solving this might be pretty tricky.

I see this as a warning that we should not play with fire.
There might be a reason why all format modifiers are put
between % and the format identifier.

> > still prefer making it %pE, both because it's easier to convert integers
> > to ERR_PTRs than having to worry about the type of PTR_ERR() being long
> > and not int, and because alphanumerics after %p have been ignored for a
> > long time (10 years?) whether or not those characters have been
> > recognized as a %p extension, so nobody relies on %pE putting an E after
> > the %p output. It also keeps the non-standard extensions in the same
> > "namespace", so to speak.
>
> > Oh, 'E' is taken, well, make it 'e' then.
> 
> I like having %pe to print error valued pointers. Then maybe we could
> have both %de for ints and %pe for pointers. :-)

I would prefer to avoid %pe. It would make sense only when the value
really contains error id. It means that it has to be used as:

    if (IS_ERR(p)) {
       pr_warn(...);

The error path might handle the error using PTR_ERR() also
on other locations. Also PTR_ERR() will make it clear that we
are trying to print the error code.

Best Regards,
Petr

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3022 bytes --]

Hello Petr,

On 8/30/19 11:06 AM, Petr Mladek wrote:
> On Thu 2019-08-29 19:39:45, Uwe Kleine-König  wrote:
>> On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
>>> On 29/08/2019 10.27, Juergen Gross wrote:
>>>> Hmm, what about already existing format strings conatining "%dE"?
>>>>
>>>> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
>>>> nevertheless...
>>>
>>> Indeed, Uwe still needs to respond to how he wants to handle that. I
>>
>> This is indeed bad and I didn't expect that. I just took a quick look
>> and this string is indeed used as sprintf format string.
> 
> Hmm, it seems that solving this might be pretty tricky.

I thought about this and we could make it possible by some syntax. Such that

	someint = 42
	pr_info("%d}E\n", someint)

emits

	42E

(I'm open to use a different "end-of-fmt-specifier" char, a } might
confuse source editors when highlighting matching braces. Maybe '#'?

This idea could be transferred to %p, too, which then lift the
limitation that some strings cannot easily be produced by printk et al.
(Of course this makes the format string parsing still more complicated
which I expect you won't like.) This would make it possible then to
adapt drivers/staging/speakup/speakup_bns.c before introducing the
format for error ints.

> I see this as a warning that we should not play with fire.
> There might be a reason why all format modifiers are put
> between % and the format identifier.

AFAIK they are put after the format specifier in the kernel to still be
able to benefit from the compiler's printf attribute.

>>> still prefer making it %pE, both because it's easier to convert integers
>>> to ERR_PTRs than having to worry about the type of PTR_ERR() being long
>>> and not int, and because alphanumerics after %p have been ignored for a
>>> long time (10 years?) whether or not those characters have been
>>> recognized as a %p extension, so nobody relies on %pE putting an E after
>>> the %p output. It also keeps the non-standard extensions in the same
>>> "namespace", so to speak.
>>
>>> Oh, 'E' is taken, well, make it 'e' then.
>>
>> I like having %pe to print error valued pointers. Then maybe we could
>> have both %de for ints and %pe for pointers. :-)
> 
> I would prefer to avoid %pe. It would make sense only when the value
> really contains error id.

The same holds true for %dE. Something has to happen if an int is passed
that isn't an error code. I'm emitting it as is in my patch, the same
could be done for a pointer.

> It means that it has to be used as:
> 
>     if (IS_ERR(p)) {
>        pr_warn(...);
> 
> The error path might handle the error using PTR_ERR() also
> on other locations. Also PTR_ERR() will make it clear that we
> are trying to print the error code.

I suggest to postpone this until we have %dE. (But I consider using %de
instead as then if we later chose that %pe is a nice idea they can use
the same modifier.)

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vsprintf: introduce %dE for error constants

[Rasmus Villemoes]

On 29/08/2019 19.39, Uwe Kleine-König wrote:
> On 8/29/19 11:09 AM, Rasmus Villemoes wrote:

>> still prefer making it %pE, both because it's easier to convert integers
>> to ERR_PTRs than having to worry about the type of PTR_ERR() being long
>> and not int, and because alphanumerics after %p have been ignored for a
>> long time (10 years?) whether or not those characters have been
>> recognized as a %p extension, so nobody relies on %pE putting an E after
>> the %p output. It also keeps the non-standard extensions in the same
>> "namespace", so to speak.
>>
>> Oh, 'E' is taken, well, make it 'e' then.
> 
> I like having %pe to print error valued pointers. Then maybe we could
> have both %de for ints and %pe for pointers. :-)

Oh no. And actually, come to think of it, we don't even need to extend
%p at all (taking away yet another letter for future expansion...), we
should simply make %p DTRT when passed an ERR_PTR - currently, if it's
some %p<extension> that would normally derefence it, there's sanity
checking in place which makes it print (efault), while if it's plain %p,
it just does the hashing, making it impossible to figure out that it was
an errno value (or which it was).

I've cooked up what I mean, sending in separate thread.

Rasmus