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.
Or did you mean %pe with the argument being an (int*), so one would do
if (err < 0)
pr_err("bad: %pe\n", &err);
Maybe I'd buy that one, though I don't think it's much worse to do
if (err < 0)
pr_err("bad: %p\n", ERR_PTR(err));
Also, the former has less type safety/type genericity than the latter;
if err happens to be a long (or s8 or s16) the former won't work while
the latter will.
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".
> 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).
> Other than that I wonder how we can go forward from here. So I think it
> is time for v3 which picks up the few suggestions.
Yes, I have actually prepared a v3, was just waiting for additional
comments on my responses to the v2 review comments.
Rasmus
