Messages in this thread | | | Date | Tue, 7 Jan 2020 14:19:54 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH v2] printk: Fix preferred console selection with multiple matches |
| |
On (20/01/06 11:25), Petr Mladek wrote: > On Mon 2020-01-06 14:15:08, Sergey Senozhatsky wrote: > > On (19/12/20 10:11), Petr Mladek wrote: > > [..] > > > > > > +enum con_match { > > > > > > + con_matched, > > > > > > + con_matched_preferred, > > > > > > + con_braille, > > > > > > + con_failed, > > > > > > + con_no_match, > > > > > > +}; > > > > > > > > > > Please, replace this with int, where: > > > > > > > > > > + con_matched -> 0 > > > > > + con_matched_preferred -> 0 and make "has_preferred" global variable > > > > > + con_braile -> 0 later check for CON_BRL flag > > > > > + con_failed -> -EFAULT > > > > > + con_no_match -> -ENOENT > > > > > > > > Not fan of using -EFAULT here, it's a detail since it's rather kernel > > > > internal, but I'd rather use -ENXIO for no match and -EIO for failed > > > > (or pass the original error code up if any). That said it's really bike > > > > shed painting at this point :-) > > > > > > Sigh, either variant is somehow confusing. > > > > > > I think that -ENOENT is a bit better than -EIO. It is abbreviation of > > > "No entry or No entity" which quite fits here. Also the device might > > > exist but it is not used when not requested. > > > > Can we please keep the enum? Enum is super self-descriptive, can't > > get any better. Any other alternative - be it -EFAULT or -EIO or > > -ENOENT - would force one to always look at what is actually going > > on in try_match_new_console() and what particular errno means. None > > of those errnos fit, they make things cryptic. IMHO. > > I agree that the enums are more self-descriptive. My problem with it is > that there are 5 values. I wanted to check how they were handled > and neither 'con_matched' nor 'con_failed' were later used.
Right, I also saw that not all con_match were used, but I didn't consider it to be an issue, con_match describes all possible cases (completeness) but not all of those cases exist in the code.
try_match_new_console() is going to return multiple error codes anyway, all of which should be handled. Switching to `int' (4 billion possible values) probably doesn't really help us.
> I though how to improve it. And I ended with feeling that the enum > did more harm then good. -E??? codes are a bit less descriptive > but there are only two. The meaning can be explained easily by > a comment above the function.
I understand it. It's just we don't have appropriate errnos. So instead of only documenting the logic (because enum is self-documenting), with errnos we also need to document the return values we check. IMHO.
-ss
| |