Message-ID: <74303921-aa95-9962-2254-27e556af54f4@kleine-koenig.org>
Date:   Wed, 28 Aug 2019 21:18:37 +0200
From:   Uwe Kleine-König <uwe@...ine-koenig.org>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        "Enrico Weigelt, metux IT consult" <lkml@...ux.net>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
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



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists