On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
<[email protected]> 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.
> +#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.
> +const char *errcode(int err)
We got long, why not to use long type for it?
> +{
> + /* 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?
> + if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> + return codes_512[err - 512];
> + /* But why? */
> + if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> + return "EDQUOT";
> + return NULL;
> +}
> + long err = PTR_ERR(ptr);
> + const char *sym = errcode(-err);
Do we need additional sign change if we already have such check inside
errcode()?
--
With Best Regards,
Andy Shevchenko
On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
> <[email protected]> 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.
> > +#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.
>
> > +const char *errcode(int err)
> We got long, why not to use long type for it?
>
> > +{
> > + /* 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?
>
> > + if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> > + return codes_512[err - 512];
> > + /* But why? */
> > + if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> > + return "EDQUOT";
> > + return NULL;
> > +}
> > + long err = PTR_ERR(ptr);
> > + const char *sym = errcode(-err);
>
> Do we need additional sign change if we already have such check inside
> errcode()?
How is EBUSY differentiated from ZERO_SIZE_PTR ?
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
>> <[email protected]> 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
On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
> 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
>>> <[email protected]> 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?
I think Joe means: What happens if codes_0[57] is "" because there is no
ESOMETHING with value 57.
Best regards
Uwe
On 11/09/2019 11.37, Uwe Kleine-K?nig wrote:
> On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
>> 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
>>>> <[email protected]> wrote:
>>>>> +{
>>>>> + /* 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?
>
> I think Joe means: What happens if codes_0[57] is "" because there is no
> ESOMETHING with value 57.
[That was Andy, not Joe, I was lazy and replied to both in one email
since Joe quoted all of Andy's questions].
So first, for good measure, codes_0[57] may be NULL but not "". Second,
if we're passed 57 but no Exxx on this architecture has the value 57,
then yes, we return NULL, just as if we're passed -18 or 0 or 1234. But
57 (or -57, or ERR_PTR(-57)) would realistically never find its way into
an err variable (be it pointer or integer) in the first place when no
ESOMETHING has the value 57.
Except for the case where somebody in the future adds #define ESOMETHING
57 to their asm/errno.h and starts using ESOMETHING, without updating
the table in errcode.c. But if that's the case, letting the caller
handle the "sorry, I can't translate 57 to some string" is still the
right thing to do.
Rasmus