linux-kernel - Re: [PATCH v2] printk: Fix preferred console selection with multiple matches

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200106102537.nirnfcauqdh4olgv@pathway.suse.cz>
Date:   Mon, 6 Jan 2020 11:25:37 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2] printk: Fix preferred console selection with multiple
 matches

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.

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.

If you want to keep the enum then please handle the return values
by switch(). Or make it clear that all possible return values
are handled properly.

Best Regards,
Petr