Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().

[Ard Biesheuvel]

On Sat, 5 Dec 2020 at 21:24, James Bottomley
<James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sat, 2020-12-05 at 20:36 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 19:02, James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@xxxxxxxxxxxxxxxxxxx>
> > > > wrote:
> > > > > From: Francis Laniel <laniel_francis@xxxxxxxxxxxxxxxxxxx>
> > > > >
> > > > > The two functions indicates if a string begins with a given
> > > > > prefix. The only difference is that strstarts() returns a bool
> > > > > while str_has_prefix() returns the length of the prefix if the
> > > > > string begins with it or 0 otherwise.
> > > > >
> > > >
> > > > Why?
> > >
> > > I think I can answer that.  If the conversion were done properly
> > > (which it's not) you could get rid of the double strings in the
> > > code which are error prone if you update one and forget
> > > another.  This gives a good example: 3d739c1f6156 ("tracing: Use
> > > the return of str_has_prefix() to remove open coded numbers"). so
> > > in your code you'd replace things like
> > >
> > >     if (strstarts(option, "rgb")) {
> > >         option += strlen("rgb");
> > >         ...
> > >
> > > with
> > >
> > >     len = str_has_prefix(option, "rgb");
> > >     if (len) {
> > >         option += len
> > >         ...
> > >
> > > Obviously you also have cases where strstart is used as a boolean
> > > with no need to know the length ... I think there's no value to
> > > converting those.
> > >
> >
> > This will lead to worse code being generated. strlen() is evaluated
> > at build time by the compiler if the argument is a string literal, so
> > your 'before' version gets turned into 'option += 3', whereas the
> > latter needs to use a runtime variable.
>
> str_has_prefix() is an always_inline function so it should be build
> time evaluated as well.  I think most compilers see len as being a
> constant and unchanged, so elide the variable.  This means the code
> generated should be the same.
>

Fair enough. I wasn't aware str_has_prefix() was __always_inline.

> > 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.