linux-kernel - Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has

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: <043040d9c092cedcab8bf88b0ec805616d3be44d.camel@HansenPartnership.com>
Date:   Sat, 05 Dec 2020 13:15:50 -0800
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     laniel_francis@...vacyrequired.com,
        linux-efi <linux-efi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by
 str_has_prefix().

[Rostedt added because this is all his fault]
On Sat, 2020-12-05 at 21:57 +0100, Ard Biesheuvel wrote:
> On Sat, 5 Dec 2020 at 21:24, James Bottomley
> <James.Bottomley@...senpartnership.com> wrote:
[...]
> > > So I don't object to using str_has_prefix() in new code in this
> > > way, but I really don't see the point of touching existing code.
> > 
> > That's your prerogative as a Maintainer ... I was just explaining
> > what the original author had in mind when str_has_prefix() was
> > created.
> > 
> 
> Sure, I fully understand you are not the one proposing these changes.
> 
> But if the pattern in question is so common, couldn't we go one step
> further and define something like
> 
> static inline const u8 *skip_prefix_or_null(const u8 *str, const u8
> *prefix)
> {
> }
> 
> which returns a pointer into the original string, or NULL if the
> prefix is not present.
> 
> The current patch as proposed has no benefit whatsoever, but even the
> meaningful alternative you are proposing is not actually an
> improvement, given that it is not self-explanatory from the name
> 'str_has_prefix' what it returns, and so the code becomes more
> difficult to understand.

Ah, so this is the kernel maintainer's syndrome: you see an API which
isn't quite right for your use case, so you update or change it.  Then
you see other use cases for it and suddenly to you it becomes the best
thing since sliced bread and with a one ring to rule them all mentality
you exhort everyone to use this new API everywhere.  See this comment
in the merge commit (495d714ad1400) which comes from the merge cover
letter:

>     - Addition of str_has_prefix() and a few use cases. There
>       currently is a similar function strstart() that is used in a 
>       few places, but only returns a bool and not a length. These 
>       instances will be removed in the future to use 
>       str_has_prefix() instead.

Then you forget about it until someone else acts on your somewhat ill
considered instruction and actually tries the replacement.  Once
someone takes up your cause, the API shows up in dozens of emails and
the actual debate about whether or not this is such a good API really
begins, with the poor person who picked it up caught in the crossfire.

As maintainers we really should learn to put the cart before the horse.

James