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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On 11/09/2019 02.15, Joe Perches wrote:
> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:

>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>
>> From long term prospective 300 and 550 hard coded here may be forgotten.

No? The point of the BUILD_BUG_ON_ZEROs is that if you add a new
Esomething, you'll get an instant build error if Esomething doesn't fit
nicely in the array you put it in. Then one can go back and figure out
whether the limit should be raised, a new codes_foo should be created,
or if it's early enough so it's not ABI yet, simply change Esomething to
a saner value.

A much bigger problem is that it's possible to add something to some
errno.h without updating this table, but there's no good solution for
that, I'm afraid. However, new Esomething are very rarely added, and
printf() will still handle it gracefully until somebody notices.

>>> +const char *errcode(int err)
>> We got long, why not to use long type for it?

Because errno values by definition have type int - and the linux syscall
ABI very clearly limits values to [1,4095]. I can change the type used
in vsnprintf.c if you prefer.

>>> +{
>>> +       /* Might as well accept both -EIO and EIO. */
>>> +       if (err < 0)
>>> +               err = -err;
>>> +       if (err <= 0) /* INT_MIN or 0 */
>>> +               return NULL;
>>> +       if (err < ARRAY_SIZE(codes_0))
>>> +               return codes_0[err];
>>
>> It won't work if one of the #ifdef:s in the array fails.
>> Would it?

I don't understand what you mean. How can an ifdef fail(?), and what
exactly won't work?

>>> +}
>>> +               long err = PTR_ERR(ptr);
>>> +               const char *sym = errcode(-err);
>>
>> Do we need additional sign change if we already have such check inside
>> errcode()?

Nah, but I went back and forth on this and ended up falling between two
stools. I think I'll drop the handling of negative arguments to
errcode(), the INT_MIN case makes that slightly ugly anyway.

> How is EBUSY differentiated from ZERO_SIZE_PTR ?

Huh? ZERO_SIZE_PTR aka (void*)(16L) is not IS_ERR(), so we won't get
here in that case.

Rasmus