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

[Uwe Kleine-KÃnig]

On 9/16/19 3:23 PM, Rasmus Villemoes wrote:
> On 16/09/2019 14.23, Uwe Kleine-KÃnig wrote:
>> Hello Rasmus,
>>
>> On 9/9/19 10:38 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.
>>>
>>> With my embedded hat on, I've made it possible to remove this.
>>>
>>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>>> 0day bot will tell me which ones I've missed.
>>>
>>> The symbols to include have been found by massaging the output of
>>>
>>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>>
>>> In the cases where some common aliasing exists
>>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>>> to the bottom so that one takes precedence.
>>>
>>> Acked-by: Uwe Kleine-KÃnig <uwe@xxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
>>
>> Even with my ack already given I still think having %pE (or %pe) for
>> ints holding an error code is sensible. 
> 
> I don't understand why you'd want an explicit %p<something> to do what
> %p does by itself - in fact, with the current vsnprintf implementation,
> "%pe", ERR_PTR(-EFOO) would already do what you want (since after %p is
> processed, all alphanumeric are skipped whether they were interpreted or
> not). So we could "reserve" %pe perhaps in order to make the call sites
> a little more readable, but no code change in vsnprintf.c would be
> necessary.

Sorry, I meant I still consider %de (or %dE) sensible which I suggested
at the start of this thread.

> Or perhaps you meant introduce a %d<something> extension? I still think
> that's a bad idea, and I've in the meantime found another reason
> (covering %d in particular): Netdevices can be given a name containing
> exactly one occurrence of %d (or no % at all), and then the actual name
> will be determined based on that pattern. These patterns are settable
> from userspace. And everything of course breaks horribly if somebody set
> a name to "bla%deth" and that got turned into "blaEPERMth".

Sure, this should not happen. I don't see imminent danger though.
(ethernet IDs are usually positive, right?)

I think having a possibility to print error codes in an int is
beneficial, as otherwise I'd have to convert to a pointer first when
printing the code which is IMHO unnecessary burden.

>> So I wonder if it would be a
>> good idea to split this patch into one that introduces errcode() and
>> then the patch that teaches vsprintf about emitting its return value for
>> error valued pointers. Then I could rebase my initial patch for %pe on
>> top of your first one.
> 
> Well, I think my patch as-is is simple enough, there's not much point
> separating the few lines in vsnprintf() from the introduction of
> errcode() (which, realistically, will never have other callers).

Fine if your series goes in soon. If not I'd like to use errcode()
without having to discuss the changes to how pointers are printed.

Best regards
Uwe

Attachment: signature.asc
Description: OpenPGP digital signature