Message-ID: <4d085535-3ff1-c53c-b032-42bdfcf1bddd@rasmusvillemoes.dk>
Date:   Wed, 4 Sep 2019 11:13:38 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Uwe Kleine-König <uwe@...ine-koenig.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Petr Mladek <pmladek@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joe Perches <joe@...ches.com>, Juergen Gross <jgross@...e.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printf: add support for printing symbolic error codes

On 02/09/2019 17.29, Uwe Kleine-König wrote:
> Hello Rasmus,
> 
> On 8/30/19 11:46 PM, Rasmus Villemoes wrote:
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>> another attempt. Rather than adding another %p extension, simply teach
>> plain %p to convert ERR_PTRs. While the primary use case is
>>
>>   if (IS_ERR(foo)) {
>>     pr_err("Sorry, can't do that: %p\n", foo);
>>     return PTR_ERR(foo);
>>   }
>>
>> it is also more helpful to get a symbolic error code (or, worst case,
>> a decimal number) in case an ERR_PTR is accidentally passed to some
>> %p<something>, rather than the (efault) that check_pointer() would
>> result in.
> 
> Your text suggests that only cases that formerly emitted "(efault)" are
> affected. I didn't check this but if this is the case, I like your patch.

Well, sort of. Depends on whether this is plain %p or %p<something>.

In the former case, the pointer would have been treated as any other
"valid" kernel pointer, hence passed through the ptr_to_id() and printed
as the result of, roughly, siphash((long)ptr) - i.e. some hex number
that has nothing directly to do with the -EIO that was passed in.
Moreover, while the printed value would be the same for a given error
code during a given boot, on the next boot, the hashing would use a
different seed, so would result in another "random" hex value being
printed - which one can easily imagine makes debugging harder. With my
patch, these would always result in "EIO" (or its decimal
representation) being printed.

In the latter case, yes, all the %p extensions that would somehow
dereference ptr would then be caught in the check_pointer() and instead
of printing (efault), we'd (again) get the symbolic error code.

In both cases, I see printing the symbolic errno as a clear improvement
- completely independent of whether we somehow want to make it
"officially allowed" to deliberately pass ERR_PTRs (and I see that I
forgot to update Documentation). So while that is really the main point
of the patch, IMO the patch can already be justified by the above.

[A few of the %p extensions do not dereference ptr (e.g. the %pS aka
print the symbol name) - I think they just print ptr as a hex value if
no symbol is found (or !CONFIG_KALLSYMS). I can't imagine how an ERR_PTR
would be passed to %pS, though, and again, getting the symbolic error
(or the decimal -22) should be better than getting -22 printed as a hex
string.]

>> With my embedded hat on, I've made it possible to remove this.
> 
> I wonder if the config item should only be configurable if EXPERT is
> enabled.

Maybe. Or one could make it default y and then only make it possible to
deselect if CONFIG_EXPERT - only really space-constrained embedded
devices would want this disabled, I think. But I prefer keeping it
simple, i.e. keeping it as-is for now.

> Apart from the above minor issues:
> 
> Acked-by: Uwe Kleine-König <uwe@...ine-koenig.org>

Thanks. The buildbot apparently tried to compile the errcode.h header by
itself and complained that NULL was not defined, so I'll respin to make
it happy, and add a note to Documentation/. Can I include that ack even
if I don't change the Kconfig logic?

Thanks,
Rasmus

Powered by blists - more mailing lists