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
Attachment:
signature.asc
Description: OpenPGP digital signature