On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:

Petr Mladek had some concerns:

quoted

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.

quoted

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.

quoted

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.

quoted

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?

quoted

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