LinuxLists.cc - [PATCH v2] vsprintf: introduce %dE for error constants

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

Hello,

I fat-fingered Enrico Weigelt's e-mail address. Please when you reply to
my patch fix that up (i.e. drop [email protected] and
[email protected]).

Thanks and sorry,
Uwe

Attachments:

2019-08-27 21:23:46

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-K?nig wrote:
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
>
> pr_info("probing failed (%dE)\n", ret);
>
> yields
>
> probing failed (EIO)
>
> if ret holds -EIO. This is easier to understand than the for now common
>
> probing failed (-5)

I suggest using both outputs like '-5 -EIO'
rather than a single string

2019-08-27 21:39:39

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

Hello Joe,

On 8/27/19 11:22 PM, Joe Perches wrote:
> On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
>> The new format specifier %dE introduced with this patch pretty-prints
>> the typical negative error values. So
>>
>> pr_info("probing failed (%dE)\n", ret);
>>
>> yields
>>
>> probing failed (EIO)
>>
>> if ret holds -EIO. This is easier to understand than the for now common
>>
>> probing failed (-5)
>
> I suggest using both outputs like '-5 -EIO'
> rather than a single string

I like it the way it is implemented as it is more flexible. If you want
to see both, you can still do

pr_info("probing failed (%d %dE)\n", ret, ret);

and people (like me) who think that giving only EIO can still do just that.

Best regards
Uwe

Attachments:

2019-08-27 23:12:30

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Tue, 2019-08-27 at 23:35 +0200, Uwe Kleine-K?nig wrote:
> Hello Joe,
>
> On 8/27/19 11:22 PM, Joe Perches wrote:
> > On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-K?nig wrote:
> > > The new format specifier %dE introduced with this patch pretty-prints
> > > the typical negative error values. So
> > >
> > > pr_info("probing failed (%dE)\n", ret);
> > >
> > > yields
> > >
> > > probing failed (EIO)
> > >
> > > if ret holds -EIO. This is easier to understand than the for now common
> > >
> > > probing failed (-5)
> >
> > I suggest using both outputs like '-5 -EIO'
> > rather than a single string
>
> I like it the way it is implemented as it is more flexible. If you want
> to see both, you can still do
>
> pr_info("probing failed (%d %dE)\n", ret, ret);
>
> and people (like me) who think that giving only EIO can still do just that.

<shrug> Up to you. Just a suggestion.

btw:

The test for %<dixu>E (FORMAT_TYPE_INT)
should probably include a test for

(spec->flags & SIGN)

so that it only is used for %d and %i and
disregarded for %x and %u

2019-08-28 11:33:25

by Petr Mladek

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Tue 2019-08-27 23:12:44, Uwe Kleine-K?nig wrote:
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
>
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.

I missed that the number was stored in the array as well. I somehow
expected that it was array of strings.

> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
>
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.

I personally do not like switch/case. It is a lot of code.
I wonder if it even saved some space.

If you want to safe space, I would use u16 to store the numbers.
Or I would use array of strings. There will be only few holes.

You might also consider handling only the most commonly
used codes from errno.h and errno-base.h (1..133). There will
be no holes and the codes are stable.

> > Both, %dE modifier and the output format (ECODE) is non-standard.
>
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument.

I am not completely sure that %p modifiers were a good idea.
They came before I started maintaining printk(). They add more
complex algorithms into paths where we could not report problems
easily (printk recursion). Also they are causing problems with
unit testing that might be done in userspace. These non-standard
formats cause that printk() can't be simply substituted by printf().

I am not keen to spread these problems over more formats.
Also %d format is more complicated. It is often used with
already existing modifiers.

> > Upper letters gain a lot of attention. But the error code is
> > only helper information. Also many error codes are misleading because
> > they are used either wrongly or there was no better available.
>
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
> mydev: Failed to initialize blablub: EIO
>
> is more expressive than
>
> mydev: Failed to initialize blablub: -5

OK, upper letters probably are not a problem.

But what about EWOULDBLOCK and EDEADLOCK? They have the same
error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
People might spend a lot of time searching for EAGAIN before they
notice that EWOULDBLOCK was used in the code instead.

Also you still did not answer the question where the idea came from.
Did it just look nice? Anyone asked for it? Who? Why?

> > There is no proof that this approach would be widely acceptable for
> > subsystem maintainers. Some might not like mass and "blind" code
> > changes. Some might not like the output at all.
>
> I don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).

It looks like a lot of potentially useless work.

> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.

Please, do the opposite. Add conversion of few subsystems into the
patchset and add more people into CC. We will see immediately whether
it makes sense to spend time on this.

I personally think that this feature is not worth the code, data,
and bikeshedding.

Best Regards,
Petr

2019-08-28 11:56:10

by Jani Nikula

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Wed, 28 Aug 2019, Petr Mladek <[email protected]> wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>> Petr Mladek had some concerns:
>> > The array is long, created by cpu&paste, the index of each code
>> > is not obvious.
>>
>> Yeah right, the array is long. This cannot really be changed because we
>> have that many error codes. I don't understand your concern about the
>> index not being obvious. The array was just a list of (number, string)
>> pairs where the position in the array didn't have any semantic.
>
> I missed that the number was stored in the array as well. I somehow
> expected that it was array of strings.
>
>
>> > There are ideas to make the code even more tricky to reduce
>> > the size, keep it fast.
>>
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
>
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.
>
> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
>
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.
>
>
>> > Both, %dE modifier and the output format (ECODE) is non-standard.
>>
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
>
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().
>
> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.
>
>
>> > Upper letters gain a lot of attention. But the error code is
>> > only helper information. Also many error codes are misleading because
>> > they are used either wrongly or there was no better available.
>>
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>> mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>>
>> mydev: Failed to initialize blablub: -5
>
> OK, upper letters probably are not a problem.
>
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.
>
> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?
>
>
>> > There is no proof that this approach would be widely acceptable for
>> > subsystem maintainers. Some might not like mass and "blind" code
>> > changes. Some might not like the output at all.
>>
>> I don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
>
> It looks like a lot of potentially useless work.
>
>
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
>
> I personally think that this feature is not worth the code, data,
> and bikeshedding.

The obvious alternative, I think already mentioned, is to just add
strerror() or similar as a function. I doubt there'd be much opposition
to that. Folks could use %s and strerr(ret). And a follow-up could add
the special format specifier if needed.

BR,
Jani.

--
Jani Nikula, Intel Open Source Graphics Center

2019-08-28 12:04:20

by Sergey Senozhatsky

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On (08/28/19 14:54), Jani Nikula wrote:
[..]
> > I personally think that this feature is not worth the code, data,
> > and bikeshedding.
>
> The obvious alternative, I think already mentioned, is to just add
> strerror() or similar as a function. I doubt there'd be much opposition
> to that. Folks could use %s and strerr(ret). And a follow-up could add
> the special format specifier if needed.

Yeah, I'd say that strerror() would be a better alternative
to vsprintf() specifier. (if we decide to add such functionality).

-ss

2019-08-28 12:50:47

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> On (08/28/19 14:54), Jani Nikula wrote:
> [..]
>>> I personally think that this feature is not worth the code, data,
>>> and bikeshedding.
>>
>> The obvious alternative, I think already mentioned, is to just add
>> strerror() or similar as a function. I doubt there'd be much opposition
>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>> the special format specifier if needed.
>
> Yeah, I'd say that strerror() would be a better alternative
> to vsprintf() specifier. (if we decide to add such functionality).

Please no. The .text footprint of the changes at the call sites to do
pr_err("...%s...", errcode(err)) instead of the current
pr_err("...%d...", err) would very soon dwarf whatever is necessary to
implement %pE or %dE. Also that would prevent any kind of graceful
fallback in case err doesn't have a known value - errcode() would have
to return some fixed "huh, unknown error number" string, so we'd lose
the actual number.

Rasmus

2019-08-28 13:01:11

by Sergey Senozhatsky

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On (08/28/19 14:49), Rasmus Villemoes wrote:
> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> > On (08/28/19 14:54), Jani Nikula wrote:
> > [..]
> >>> I personally think that this feature is not worth the code, data,
> >>> and bikeshedding.
> >>
> >> The obvious alternative, I think already mentioned, is to just add
> >> strerror() or similar as a function. I doubt there'd be much opposition
> >> to that. Folks could use %s and strerr(ret). And a follow-up could add
> >> the special format specifier if needed.
> >
> > Yeah, I'd say that strerror() would be a better alternative
> > to vsprintf() specifier. (if we decide to add such functionality).
>
> Please no. The .text footprint of the changes at the call sites to do
> pr_err("...%s...", errcode(err)) instead of the current
> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
> implement %pE or %dE.

New vsprintf() specifiers have some downsides as well. Should %dE
accidentally (via backport) make it to the -stable kernel, which
does not support %dE, and we are going to lose the actual error
code value as well.

-ss

2019-08-28 16:24:12

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 8/28/19 2:59 PM, Sergey Senozhatsky wrote:
> On (08/28/19 14:49), Rasmus Villemoes wrote:
>> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
>>> On (08/28/19 14:54), Jani Nikula wrote:
>>> [..]
>>>>> I personally think that this feature is not worth the code, data,
>>>>> and bikeshedding.
>>>>
>>>> The obvious alternative, I think already mentioned, is to just add
>>>> strerror() or similar as a function. I doubt there'd be much opposition
>>>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>>>> the special format specifier if needed.
>>>
>>> Yeah, I'd say that strerror() would be a better alternative
>>> to vsprintf() specifier. (if we decide to add such functionality).
>>
>> Please no. The .text footprint of the changes at the call sites to do
>> pr_err("...%s...", errcode(err)) instead of the current
>> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
>> implement %pE or %dE.

Yeah, that's what I think, too. I cannot imagine a user of strerror()
who needs the string representation for something different than to feed
it to one of the family members of printk. That's also why I think that
the other already existing format specifier are a good idea.

It might not be the nicest part of the printk code, but this way it is
at least concentrated in one place only.

> New vsprintf() specifiers have some downsides as well. Should %dE
> accidentally (via backport) make it to the -stable kernel, which
> does not support %dE, and we are going to lose the actual error
> code value as well.

That is wrong. When you do

pr_err("There are no round tuits to give out: %dE\n", -ENOENT);

in a kernel that doesn't support %dE you get:

There are no round tuits to give out: -2E

That's a bit ugly but I can still work out what the original value was.

Best regards
Uwe

Attachments:

2019-08-28 19:21:55

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

Hello Petr,

On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>> Petr Mladek had some concerns:
>>> There are ideas to make the code even more tricky to reduce
>>> the size, keep it fast.
>>
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
>
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.

I guess we have to die either way. Either it is quick or it is space
efficient. With the big case I trust the compiler to pick something
sensible expecting that it adapts for example to -Osize.

> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
>
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.

I'd like to postpone the discussion about "how" until we agreed about
the "if at all".

>>> Both, %dE modifier and the output format (ECODE) is non-standard.
>>
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
>
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().

In my eyes the wish to have printk() and userspace's printf
feature-identical isn't helpful because they have similar but not equal
purposes. And if a driver author cares about being able to use their
code 1:1 in userspace they could just not use %dE, %pI and whatever
there is additionally.

> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.

I don't understand what you want to tell me here.

>>> Upper letters gain a lot of attention. But the error code is
>>> only helper information. Also many error codes are misleading because
>>> they are used either wrongly or there was no better available.
>>
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>> mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>>
>> mydev: Failed to initialize blablub: -5
>
> OK, upper letters probably are not a problem.
>
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
>
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.

It already does now. If you start to debug an error message that tells
you the error is -11, you might well come to the conclusion you have to
look for EDEADLK. IMHO the right approach here should be to declare one
of the duplicates as the "right" one and replace the wrong one in the
complete tree (apart from the definition for user space of course).
After that the problem with these duplicates is completely orthogonal
(it is already now mostly orthogonal in my eyes) and also solved for
those who picked the wrong one by hand.

> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?

It was my idea, and I didn't talk about it before creating a patch. You
asked in reply to v1: "Did it look like a good idea?
Did anyone got tired by searching for the error codes many
times a day? Did the idea came from a developer, support, or user, please?"

So yes, having to lookup the right error code from messages is something
that annoys me, especially because there are at least two files you have
to check for the definition. I consider myself to have all three hats
(developer, supporter and user), and your feedback set apart my
impression is that all people replying to this thread consider it a good
idea, too.

>>> There is no proof that this approach would be widely acceptable for
>>> subsystem maintainers. Some might not like mass and "blind" code
>>> changes. Some might not like the output at all.
>>
>> I don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
>
> It looks like a lot of potentially useless work.

If the idea will not gain speed, removing the then few users is straight
forward (just a question of calling sed s/%dE/%d/ on all affected
files). So at least it shouldn't be your useless work and time.

>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
>
> I personally think that this feature is not worth the code, data,
> and bikeshedding.

I agree about the bike shedding ;-)

Best regards
Uwe

Attachments:

2019-08-28 20:00:57

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.

For now I asked in the arm linux irc channel and got two people replying
(both added to Cc:):

Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:

1567019926 < broonie> ukleinek: I think that's a great idea and have
thought about trying to implement it in the past.
1567019937 < broonie> ukleinek: Making the logs more directly readable
is enormously helpful.

and Alexandre Belloni (arm/at91, mips/microsemi, rtc) said:

1567021451 < abelloni> ukleinek: seems good to me but it would probably
be better to be able to generate the list

(I fully agree to the wish to generate the list, as I already wrote
before, I don't have a good idea how to do that without generating
C-Code by some means which is ugly and also complicated by the fact that
there are several locations (at least for now) that have definitions for
error codes.)

Best regards
Uwe

Attachments:

2019-08-28 20:53:32

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Wed, Aug 28, 2019 at 09:59:16PM +0200, Uwe Kleine-K?nig wrote:
> On 8/28/19 1:32 PM, Petr Mladek wrote:

> > Please, do the opposite. Add conversion of few subsystems into the
> > patchset and add more people into CC. We will see immediately whether
> > it makes sense to spend time on this.

> For now I asked in the arm linux irc channel and got two people replying
> (both added to Cc:):

> Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:

> 1567019926 < broonie> ukleinek: I think that's a great idea and have
> thought about trying to implement it in the past.
> 1567019937 < broonie> ukleinek: Making the logs more directly readable
> is enormously helpful.

In the past I've actually implemented error code decoders like this for
some other systems I've worked on due to annoyance with having to look
up codes, as far as I can tell they went over quite well with other
developers and I certainly found them helpful myself. They're also
useful to end users looking at logs, they're not always aware of how to
find the relevant error code definitions so a slightly more descriptive
messages can help people figure things out.

It does also occur to me that this might be useful to the people who
want to work on making probe deferral quieter since it gives them an
annotation that the number going in is an error code. There's a bunch
more work to do there though.

Attachments:

(No filename) (1.39 kB)
signature.asc (499.00 B)
Download all attachments

2019-08-29 00:29:49

by Sergey Senozhatsky

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On (08/28/19 18:22), Uwe Kleine-K?nig wrote:
> That is wrong. When you do
>
> pr_err("There are no round tuits to give out: %dE\n", -ENOENT);
>
> in a kernel that doesn't support %dE you get:
>
> There are no round tuits to give out: -2E

OK. Good point.

-ss

2019-08-29 08:14:05

by Petr Mladek

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Wed 2019-08-28 21:18:37, Uwe Kleine-K?nig wrote:
> Hello Petr,
>
> On 8/28/19 1:32 PM, Petr Mladek wrote:
> > On Tue 2019-08-27 23:12:44, Uwe Kleine-K?nig wrote:
> >> Petr Mladek had some concerns:
> >>> There are ideas to make the code even more tricky to reduce
> >>> the size, keep it fast.
> >>
> >> I think Enrico Weigelt's suggestion to use a case is the best
> >> performance-wise so that's what I picked up. Also I hope that
> >> performance isn't that important because the need to print an error
> >> should not be so common that it really hurts in production.

This is contadicting. The "best" performance-wise solution was
choosen in favor of space. The next sentence says that performance
is not important.

> > I personally do not like switch/case. It is a lot of code.
> > I wonder if it even saved some space.
>
> I guess we have to die either way. Either it is quick or it is space
> efficient.

I am more concerned about the size. Well, array of strings will
be both fast and size efficient.

> With the big case I trust the compiler to pick something
> sensible expecting that it adapts for example to -Osize.

I am not sure what are the expectations here. I can't imagine
another translation than:

if (val == 1)
str = "EPERM";
else if (val == 2)
str = "ENOENT"
else if (val == 3)
str = "ESRCH"
...

It means that all constans will be hardcoded in the code instead
of in data section. Plus there will be instructions for each
if/else part.

> > If you want to safe space, I would use u16 to store the numbers.
> > Or I would use array of strings. There will be only few holes.
> >
> > You might also consider handling only the most commonly
> > used codes from errno.h and errno-base.h (1..133). There will
> > be no holes and the codes are stable.
>
> I'd like to postpone the discussion about "how" until we agreed about
> the "if at all".

It seems that all people like this feature.

BTW: I though more about generating or cut&pasting the arrary.
I can't find any reasonable way how to generate it.

But both, errno.h and errno-base.h, are super stable. Only
comments were modified or new codes added. Most of them
are defined by POSIX so they should remain stable.

Therefore cut&pasted array of strings looks acceptable.
We should only allow to easily check numbers for each code,
e.g. by defining the array as

const err_str * [] {
"0" /* 0 Success */
"EPERM", /* 1 Operation not permitted */
"ENOENT", /* 2 No such file or directory */
"ESRCH", /* 3 No such process */
...

If there is a hole, we could use something like:

"-41", /* 41 Skipped. EWOULDBLOCK is
defined as EAGAIN. Operation would block */

Best Regards,
Petr

2019-08-29 08:28:10

by Jürgen Groß

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 29.08.19 10:12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
>> Hello Petr,
>>
>> On 8/28/19 1:32 PM, Petr Mladek wrote:
>>> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>>>> Petr Mladek had some concerns:
>>>>> There are ideas to make the code even more tricky to reduce
>>>>> the size, keep it fast.
>>>>
>>>> I think Enrico Weigelt's suggestion to use a case is the best
>>>> performance-wise so that's what I picked up. Also I hope that
>>>> performance isn't that important because the need to print an error
>>>> should not be so common that it really hurts in production.
>
> This is contadicting. The "best" performance-wise solution was
> choosen in favor of space. The next sentence says that performance
> is not important.
>
>>> I personally do not like switch/case. It is a lot of code.
>>> I wonder if it even saved some space.
>>
>> I guess we have to die either way. Either it is quick or it is space
>> efficient.
>
> I am more concerned about the size. Well, array of strings will
> be both fast and size efficient.
>
>> With the big case I trust the compiler to pick something
>> sensible expecting that it adapts for example to -Osize.
>
> I am not sure what are the expectations here. I can't imagine
> another translation than:
>
> if (val == 1)
> str = "EPERM";
> else if (val == 2)
> str = "ENOENT"
> else if (val == 3)
> str = "ESRCH"
> ...
>
> It means that all constans will be hardcoded in the code instead
> of in data section. Plus there will be instructions for each
> if/else part.
>
>>> If you want to safe space, I would use u16 to store the numbers.
>>> Or I would use array of strings. There will be only few holes.
>>>
>>> You might also consider handling only the most commonly
>>> used codes from errno.h and errno-base.h (1..133). There will
>>> be no holes and the codes are stable.
>>
>> I'd like to postpone the discussion about "how" until we agreed about
>> the "if at all".
>
> It seems that all people like this feature.

Hmm, what about already existing format strings conatining "%dE"?

Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
nevertheless...

>
> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.

Generate the array and errno.h/errno-base.h from the same source?

Juergen

2019-08-29 09:02:34

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 29/08/2019 10.12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-K?nig wrote:

> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.

Something like this seems to work, though it probably needs some massage
to be accepted by kbuild folks:

define filechk_errcode.h
echo '#include <linux/errno.h>' | \
$(CPP) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) -dM - | \
grep 'define E' | sort -k3,3n | \
awk '{print "errcode("$$2")\t/* "$$3" */"}'
endef

include/generated/errcode.h: FORCE
$(call filechk,errcode.h)

Then one can just #define errcode(foo) ... right before #include
<generated/errcode.h>. It cannot be used to generate cases in a switch()
because some expand to the same number, but that's ok, because I can't
imagine the switch actually generating good or small code. I haven't
checked how or whether it works in a cross-compile situation.

Rasmus

2019-08-29 09:11:20

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 29/08/2019 10.27, Juergen Gross wrote:
> On 29.08.19 10:12, Petr Mladek wrote:
>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
>>>
>>> I'd like to postpone the discussion about "how" until we agreed about
>>> the "if at all".
>>
>> It seems that all people like this feature.
>
> Hmm, what about already existing format strings conatining "%dE"?
>
> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
> nevertheless...

Indeed, Uwe still needs to respond to how he wants to handle that. I
still prefer making it %pE, both because it's easier to convert integers
to ERR_PTRs than having to worry about the type of PTR_ERR() being long
and not int, and because alphanumerics after %p have been ignored for a
long time (10 years?) whether or not those characters have been
recognized as a %p extension, so nobody relies on %pE putting an E after
the %p output. It also keeps the non-standard extensions in the same
"namespace", so to speak.

Oh, 'E' is taken, well, make it 'e' then.

Rasmus

2019-08-29 13:41:12

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Wed, Aug 28, 2019 at 12:14 AM Uwe Kleine-König <[email protected]> wrote:
>
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
>
> pr_info("probing failed (%dE)\n", ret);
>
> yields
>
> probing failed (EIO)
>
> if ret holds -EIO. This is easier to understand than the for now common
>
> probing failed (-5)
>
> (which used %d instead of %dE) for kernel developers who don't know the
> numbers by heart, The error names are also known and understood by
> userspace coders as they match the values the errno variable can have.
> Also to debug the sitation that resulted in the above message very
> probably involves EIO, so the message already gives the string to look
> out for.
>
> glibc (and other libc variants)'s printf have a similar feature: %m
> expands to strerror(errno). I preferred using %dE however because %m in
> userspace doesn't consume an argument (which is however necessary in the
> kernel as there is no global errno variable). Also this is handled
> correctly by the compiler's format string checker as there is a matching
> int variable for the %d the compiler notices.
>
> There are quite some code locations that could be adapted to benefit
> from this:
>
> $ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))$.*(\(%d$|: %d)\\n' v5.3-rc5 | wc -l
> 9141
>
> I expect there are some false negatives where the match is distributed
> over two or more lines and so isn't found.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> Hello,
>
> in v1 I handled both positive and negative errors which was challenged.
> I decided to drop that and only handle the (common) negative values.
> Readding handling of positive values later is easier than dropping it.
>
> Sergey Senozhatsky and Enrico Weigelt suggested to use strerror as name
> for the function that I called errno2errstr. (In v1 it was not a
> separate function). I didn't pick this up because the semantic of
> strerror in userspace is different. It returns something like
> "Interrupted system call" while errno2errstr only yields "EINTR".
>
> I dropped EWOULDBLOCK from the list of codes as it is on all Linux archs
> identical to EAGAIN and the way the lookup works now breaks otherwise.
> (And having it wasn't useful already in v1.)
>
> Rasmus Villemoes pointed out that the way I wrote the resulting string
> to the buffer was broken. This is fixed according to his suggestion by
> using string_nocheck(). I also added a test to lib/test_printf.c as he
> requested.
>
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
>
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.
>
> I agree it would be nice to have only one place that has a collection of
> error codes. I thought about reorganizing how the error constants are
> defined, i.e. doing something like:
>
> DEFINE_ERROR(EIO, 5, "I/O error")
>
> but I don't see a possibility to let this expand to
>
> #define EIO 5
>
> without resorting to another macro language. If that were possible the
> list of DEFINE_ERRORs could be reused to help generating the code for
> errno2errstr. So if you have a good idea, please speak up.
>
> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
>
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.
>
> > Both, %dE modifier and the output format (ECODE) is non-standard.
>
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument. For user consumption it would be nice
> if we'd get
>
> probing failed (I/O Error)
>
> from pr_info("probing failed (%dE)\n", -EIO); but that would be still
> more expensive because the collection of string constants would become
> bigger that it is already now and "EIO" is the right one to search for
> when debugging the underlaying problem. So I think "EIO" is even more
> useful than "I/O Error".
>
> > Upper letters gain a lot of attention. But the error code is
> > only helper information. Also many error codes are misleading because
> > they are used either wrongly or there was no better available.
>
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
> mydev: Failed to initialize blablub: EIO
>
> is more expressive than
>
> mydev: Failed to initialize blablub: -5
>
> . With "EIO" in the message it is not worse than with "-5" independant of the
> question if EIO is the right error code used.
>
> > There is no proof that this approach would be widely acceptable for
> > subsystem maintainers. Some might not like mass and "blind" code
> > changes. Some might not like the output at all.
>
> I don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).
>
> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.
>

Hold on, can you in less than 20 words put WHY this is needed?

--
With Best Regards,
Andy Shevchenko

2019-08-29 17:41:54

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
> On 29/08/2019 10.27, Juergen Gross wrote:
>> On 29.08.19 10:12, Petr Mladek wrote:
>>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
>>>>
>>>> I'd like to postpone the discussion about "how" until we agreed about
>>>> the "if at all".
>>>
>>> It seems that all people like this feature.
>>
>> Hmm, what about already existing format strings conatining "%dE"?
>>
>> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
>> nevertheless...
>
> Indeed, Uwe still needs to respond to how he wants to handle that. I

This is indeed bad and I didn't expect that. I just took a quick look
and this string is indeed used as sprintf format string.

> still prefer making it %pE, both because it's easier to convert integers
> to ERR_PTRs than having to worry about the type of PTR_ERR() being long
> and not int, and because alphanumerics after %p have been ignored for a
> long time (10 years?) whether or not those characters have been
> recognized as a %p extension, so nobody relies on %pE putting an E after
> the %p output. It also keeps the non-standard extensions in the same
> "namespace", so to speak.
>
> Oh, 'E' is taken, well, make it 'e' then.

I like having %pe to print error valued pointers. Then maybe we could
have both %de for ints and %pe for pointers. :-)

Best regards
Uwe

Attachments:

2019-08-30 09:07:46

by Petr Mladek

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

On Thu 2019-08-29 19:39:45, Uwe Kleine-K?nig wrote:
> On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
> > On 29/08/2019 10.27, Juergen Gross wrote:
> >> On 29.08.19 10:12, Petr Mladek wrote:
> >>> On Wed 2019-08-28 21:18:37, Uwe Kleine-K?nig? wrote:
> >>>>
> >>>> I'd like to postpone the discussion about "how" until we agreed about
> >>>> the "if at all".
> >>>
> >>> It seems that all people like this feature.
> >>
> >> Hmm, what about already existing format strings conatining "%dE"?
> >>
> >> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
> >> nevertheless...
> >
> > Indeed, Uwe still needs to respond to how he wants to handle that. I
>
> This is indeed bad and I didn't expect that. I just took a quick look
> and this string is indeed used as sprintf format string.

Hmm, it seems that solving this might be pretty tricky.

I see this as a warning that we should not play with fire.
There might be a reason why all format modifiers are put
between % and the format identifier.

> > still prefer making it %pE, both because it's easier to convert integers
> > to ERR_PTRs than having to worry about the type of PTR_ERR() being long
> > and not int, and because alphanumerics after %p have been ignored for a
> > long time (10 years?) whether or not those characters have been
> > recognized as a %p extension, so nobody relies on %pE putting an E after
> > the %p output. It also keeps the non-standard extensions in the same
> > "namespace", so to speak.
>
> > Oh, 'E' is taken, well, make it 'e' then.
>
> I like having %pe to print error valued pointers. Then maybe we could
> have both %de for ints and %pe for pointers. :-)

I would prefer to avoid %pe. It would make sense only when the value
really contains error id. It means that it has to be used as:

if (IS_ERR(p)) {
pr_warn(...);

The error path might handle the error using PTR_ERR() also
on other locations. Also PTR_ERR() will make it clear that we
are trying to print the error code.

Best Regards,
Petr

2019-08-30 15:48:30

[permalink] [raw]

Subject: Re: [PATCH v2] vsprintf: introduce %dE for error constants

Hello Petr,

On 8/30/19 11:06 AM, Petr Mladek wrote:
> On Thu 2019-08-29 19:39:45, Uwe Kleine-König wrote:
>> On 8/29/19 11:09 AM, Rasmus Villemoes wrote:
>>> On 29/08/2019 10.27, Juergen Gross wrote:
>>>> Hmm, what about already existing format strings conatining "%dE"?
>>>>
>>>> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
>>>> nevertheless...
>>>
>>> Indeed, Uwe still needs to respond to how he wants to handle that. I
>>
>> This is indeed bad and I didn't expect that. I just took a quick look
>> and this string is indeed used as sprintf format string.
>
> Hmm, it seems that solving this might be pretty tricky.

I thought about this and we could make it possible by some syntax. Such that

someint = 42
pr_info("%d}E\n", someint)

emits

42E

(I'm open to use a different "end-of-fmt-specifier" char, a } might
confuse source editors when highlighting matching braces. Maybe '#'?

This idea could be transferred to %p, too, which then lift the
limitation that some strings cannot easily be produced by printk et al.
(Of course this makes the format string parsing still more complicated
which I expect you won't like.) This would make it possible then to
adapt drivers/staging/speakup/speakup_bns.c before introducing the
format for error ints.

> I see this as a warning that we should not play with fire.
> There might be a reason why all format modifiers are put
> between % and the format identifier.

AFAIK they are put after the format specifier in the kernel to still be
able to benefit from the compiler's printf attribute.

>>> still prefer making it %pE, both because it's easier to convert integers
>>> to ERR_PTRs than having to worry about the type of PTR_ERR() being long
>>> and not int, and because alphanumerics after %p have been ignored for a
>>> long time (10 years?) whether or not those characters have been
>>> recognized as a %p extension, so nobody relies on %pE putting an E after
>>> the %p output. It also keeps the non-standard extensions in the same
>>> "namespace", so to speak.
>>
>>> Oh, 'E' is taken, well, make it 'e' then.
>>
>> I like having %pe to print error valued pointers. Then maybe we could
>> have both %de for ints and %pe for pointers. :-)
>
> I would prefer to avoid %pe. It would make sense only when the value
> really contains error id.

The same holds true for %dE. Something has to happen if an int is passed
that isn't an error code. I'm emitting it as is in my patch, the same
could be done for a pointer.

> It means that it has to be used as:
>
> if (IS_ERR(p)) {
> pr_warn(...);
>
> The error path might handle the error using PTR_ERR() also
> on other locations. Also PTR_ERR() will make it clear that we
> are trying to print the error code.

I suggest to postpone this until we have %dE. (But I consider using %de
instead as then if we later chose that %pe is a nice idea they can use
the same modifier.)

Best regards
Uwe

Attachments:

2019-08-30 21:46:52