linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: laniel_francis@privacyrequired.com,
	linux-efi <linux-efi@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
Date: Sat, 05 Dec 2020 13:15:50 -0800	[thread overview]
Message-ID: <043040d9c092cedcab8bf88b0ec805616d3be44d.camel@HansenPartnership.com> (raw)
In-Reply-To: <CAMj1kXFZ1_05O=s37n7pJbFxRm_-hAVFHKqc6OtALfBMVZVYnQ@mail.gmail.com>

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

James



  reply	other threads:[~2020-12-05 21:16 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-04 17:03 [RFC PATCH v1 00/12] " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 01/12] arm: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 02/12] mips: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 03/12] crypto: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 04/12] device-mapper: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 05/12] renesas: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 06/12] omap: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 07/12] efi: " laniel_francis
2020-12-04 17:07   ` Ard Biesheuvel
2020-12-04 17:19     ` Francis Laniel
2020-12-04 18:02     ` James Bottomley
2020-12-05 19:08       ` Francis Laniel
2020-12-05 19:36       ` Ard Biesheuvel
2020-12-05 20:24         ` James Bottomley
2020-12-05 20:57           ` Ard Biesheuvel
2020-12-05 21:15             ` James Bottomley [this message]
2020-12-05 21:20               ` Ard Biesheuvel
2020-12-05 23:04                 ` James Bottomley
2020-12-07 15:10                   ` Steven Rostedt
2020-12-07 16:25                     ` David Laight
2020-12-05 20:28         ` Rasmus Villemoes
2020-12-10 18:14         ` Arvind Sankar
2020-12-11  9:45           ` David Laight
2020-12-11 16:10             ` Arvind Sankar
2020-12-04 17:03 ` [RFC PATCH v1 08/12] ide: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 09/12] mips: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 10/12] module: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 11/12] musb: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 12/12] string.h: Remove strstarts() laniel_francis
2020-12-04 17:56 ` [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() James Bottomley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=043040d9c092cedcab8bf88b0ec805616d3be44d.camel@HansenPartnership.com \
    --to=james.bottomley@hansenpartnership.com \
    --cc=ardb@kernel.org \
    --cc=laniel_francis@privacyrequired.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --subject='Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).