Re: [PATCH] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

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@xxxxxxxxxxxxxxxxx>

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