lkml.org 
[lkml]   [2020]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
    From
    Date
    On Sat, 2020-12-05 at 22:20 +0100, Ard Biesheuvel wrote:
    > On Sat, 5 Dec 2020 at 22:15, James Bottomley
    > <James.Bottomley@hansenpartnership.com> wrote:
    > > [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@hansenpartnership.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

    s/to put/not to put/

    > > horse.
    > >
    >
    > I am not disagreeing with any of this, but I simply don't see a point
    > in merging patches that apparently result in the exact same machine
    > code to be generated, and don't substantially make the code itself
    > any better.


    Well, I think the pattern

    if (strstarts(option, <string>)) {
    ...
    option += strlen(<same string>);

    is a bad one because one day <string> may get updated but not <same
    string>. And if <same string> is too far away in the code it might not
    even show up in the diff, leading to reviewers not noticing either. So
    I think eliminating the pattern is a definite improvement.

    Now whether the improvement is enough that we should churn the code
    base to fix it is another question.

    James


    \
     
     \ /
      Last update: 2020-12-06 00:07    [W:2.193 / U:0.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site