From: Francis Laniel <[email protected]>
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.
Signed-off-by: Francis Laniel <[email protected]>
---
drivers/firmware/efi/libstub/efi-stub-helper.c | 2 +-
drivers/firmware/efi/libstub/gop.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index aa8da0a49829..a502f549d900 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -230,7 +230,7 @@ efi_status_t efi_parse_options(char const *cmdline)
if (parse_option_str(val, "debug"))
efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
} else if (!strcmp(param, "video") &&
- val && strstarts(val, "efifb:")) {
+ val && str_has_prefix(val, "efifb:")) {
efi_parse_option_graphics(val + strlen("efifb:"));
}
}
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index ea5da307d542..fbe95b3cc96a 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -39,7 +39,7 @@ static bool parse_modenum(char *option, char **next)
{
u32 m;
- if (!strstarts(option, "mode="))
+ if (!str_has_prefix(option, "mode="))
return false;
option += strlen("mode=");
m = simple_strtoull(option, &option, 0);
@@ -65,10 +65,10 @@ static bool parse_res(char *option, char **next)
h = simple_strtoull(option, &option, 10);
if (*option == '-') {
option++;
- if (strstarts(option, "rgb")) {
+ if (str_has_prefix(option, "rgb")) {
option += strlen("rgb");
pf = PIXEL_RGB_RESERVED_8BIT_PER_COLOR;
- } else if (strstarts(option, "bgr")) {
+ } else if (str_has_prefix(option, "bgr")) {
option += strlen("bgr");
pf = PIXEL_BGR_RESERVED_8BIT_PER_COLOR;
} else if (isdigit(*option))
@@ -90,7 +90,7 @@ static bool parse_res(char *option, char **next)
static bool parse_auto(char *option, char **next)
{
- if (!strstarts(option, "auto"))
+ if (!str_has_prefix(option, "auto"))
return false;
option += strlen("auto");
if (*option && *option++ != ',')
@@ -103,7 +103,7 @@ static bool parse_auto(char *option, char **next)
static bool parse_list(char *option, char **next)
{
- if (!strstarts(option, "list"))
+ if (!str_has_prefix(option, "list"))
return false;
option += strlen("list");
if (*option && *option++ != ',')
--
2.20.1
On Fri, 4 Dec 2020 at 18:06, <[email protected]> wrote:
>
> From: Francis Laniel <[email protected]>
>
> 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?
> Signed-off-by: Francis Laniel <[email protected]>
> ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 2 +-
> drivers/firmware/efi/libstub/gop.c | 10 +++++-----
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index aa8da0a49829..a502f549d900 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -230,7 +230,7 @@ efi_status_t efi_parse_options(char const *cmdline)
> if (parse_option_str(val, "debug"))
> efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> } else if (!strcmp(param, "video") &&
> - val && strstarts(val, "efifb:")) {
> + val && str_has_prefix(val, "efifb:")) {
> efi_parse_option_graphics(val + strlen("efifb:"));
> }
> }
> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> index ea5da307d542..fbe95b3cc96a 100644
> --- a/drivers/firmware/efi/libstub/gop.c
> +++ b/drivers/firmware/efi/libstub/gop.c
> @@ -39,7 +39,7 @@ static bool parse_modenum(char *option, char **next)
> {
> u32 m;
>
> - if (!strstarts(option, "mode="))
> + if (!str_has_prefix(option, "mode="))
> return false;
> option += strlen("mode=");
> m = simple_strtoull(option, &option, 0);
> @@ -65,10 +65,10 @@ static bool parse_res(char *option, char **next)
> h = simple_strtoull(option, &option, 10);
> if (*option == '-') {
> option++;
> - if (strstarts(option, "rgb")) {
> + if (str_has_prefix(option, "rgb")) {
> option += strlen("rgb");
> pf = PIXEL_RGB_RESERVED_8BIT_PER_COLOR;
> - } else if (strstarts(option, "bgr")) {
> + } else if (str_has_prefix(option, "bgr")) {
> option += strlen("bgr");
> pf = PIXEL_BGR_RESERVED_8BIT_PER_COLOR;
> } else if (isdigit(*option))
> @@ -90,7 +90,7 @@ static bool parse_res(char *option, char **next)
>
> static bool parse_auto(char *option, char **next)
> {
> - if (!strstarts(option, "auto"))
> + if (!str_has_prefix(option, "auto"))
> return false;
> option += strlen("auto");
> if (*option && *option++ != ',')
> @@ -103,7 +103,7 @@ static bool parse_auto(char *option, char **next)
>
> static bool parse_list(char *option, char **next)
> {
> - if (!strstarts(option, "list"))
> + if (!str_has_prefix(option, "list"))
> return false;
> option += strlen("list");
> if (*option && *option++ != ',')
> --
> 2.20.1
>
Le vendredi 4 d?cembre 2020, 18:07:11 CET Ard Biesheuvel a ?crit :
> On Fri, 4 Dec 2020 at 18:06, <[email protected]> wrote:
> > From: Francis Laniel <[email protected]>
> >
> > 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?
The code works fine actually with the two functions, it is a fact.
Not long ago, I read string.h and saw that there was two functions doing the
same thing.
At this moment, I thought that it can be a good idea to replace one by another
so there is only one remaining function.
I think the benefit of this patch is that it makes the code a bit simpler.
I agree that this is not a huge benefit and that the code can stay in its
actual state.
I also marked this patch as RFC to get people's opinion about if they find it
useful or not.
> > Signed-off-by: Francis Laniel <[email protected]>
> > ---
> >
> > drivers/firmware/efi/libstub/efi-stub-helper.c | 2 +-
> > drivers/firmware/efi/libstub/gop.c | 10 +++++-----
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > b/drivers/firmware/efi/libstub/efi-stub-helper.c index
> > aa8da0a49829..a502f549d900 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -230,7 +230,7 @@ efi_status_t efi_parse_options(char const *cmdline)
> >
> > if (parse_option_str(val, "debug"))
> >
> > efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> >
> > } else if (!strcmp(param, "video") &&
> >
> > - val && strstarts(val, "efifb:")) {
> > + val && str_has_prefix(val, "efifb:")) {
> >
> > efi_parse_option_graphics(val + strlen("efifb:"));
> >
> > }
> >
> > }
> >
> > diff --git a/drivers/firmware/efi/libstub/gop.c
> > b/drivers/firmware/efi/libstub/gop.c index ea5da307d542..fbe95b3cc96a
> > 100644
> > --- a/drivers/firmware/efi/libstub/gop.c
> > +++ b/drivers/firmware/efi/libstub/gop.c
> > @@ -39,7 +39,7 @@ static bool parse_modenum(char *option, char **next)
> >
> > {
> >
> > u32 m;
> >
> > - if (!strstarts(option, "mode="))
> > + if (!str_has_prefix(option, "mode="))
> >
> > return false;
> >
> > option += strlen("mode=");
> > m = simple_strtoull(option, &option, 0);
> >
> > @@ -65,10 +65,10 @@ static bool parse_res(char *option, char **next)
> >
> > h = simple_strtoull(option, &option, 10);
> > if (*option == '-') {
> >
> > option++;
> >
> > - if (strstarts(option, "rgb")) {
> > + if (str_has_prefix(option, "rgb")) {
> >
> > option += strlen("rgb");
> > pf = PIXEL_RGB_RESERVED_8BIT_PER_COLOR;
> >
> > - } else if (strstarts(option, "bgr")) {
> > + } else if (str_has_prefix(option, "bgr")) {
> >
> > option += strlen("bgr");
> > pf = PIXEL_BGR_RESERVED_8BIT_PER_COLOR;
> >
> > } else if (isdigit(*option))
> >
> > @@ -90,7 +90,7 @@ static bool parse_res(char *option, char **next)
> >
> > static bool parse_auto(char *option, char **next)
> > {
> >
> > - if (!strstarts(option, "auto"))
> > + if (!str_has_prefix(option, "auto"))
> >
> > return false;
> >
> > option += strlen("auto");
> > if (*option && *option++ != ',')
> >
> > @@ -103,7 +103,7 @@ static bool parse_auto(char *option, char **next)
> >
> > static bool parse_list(char *option, char **next)
> > {
> >
> > - if (!strstarts(option, "list"))
> > + if (!str_has_prefix(option, "list"))
> >
> > return false;
> >
> > option += strlen("list");
> > if (*option && *option++ != ',')
> >
> > --
> > 2.20.1
On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> wrote:
> > From: Francis Laniel <[email protected]>
> >
> > 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.
James
Le vendredi 4 d?cembre 2020, 19:02:09 CET James Bottomley a ?crit :
> On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> >
> > wrote:
> > > From: Francis Laniel <[email protected]>
> > >
> > > 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
> ...
The proposed changes were a bit mechanical and I did not think about using the
returned value in the way you proposed.
This a good idea though, so I can modify my patches to include this and send a
v2!
> 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.
For the v2, should I only change cases where using str_has_prefix() brings a
benefit over strstarts() or all the cases?
> James
On Fri, 4 Dec 2020 at 19:02, James Bottomley
<[email protected]> wrote:
>
> On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> > wrote:
> > > From: Francis Laniel <[email protected]>
> > >
> > > 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.
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.
On Sat, 2020-12-05 at 20:36 +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <[email protected]> wrote:
> > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> > > wrote:
> > > > From: Francis Laniel <[email protected]>
> > > >
> > > > 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.
> 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.
James
On 05/12/2020 20.36, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <[email protected]> wrote:
>>
>> On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
>>> On Fri, 4 Dec 2020 at 18:06, <[email protected]>
>>> wrote:
>>>> From: Francis Laniel <[email protected]>
>>>>
>>>> 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.
Well, both functions are static inlines
static inline bool strstarts(const char *str, const char *prefix)
{
return strncmp(str, prefix, strlen(prefix)) == 0;
}
static __always_inline size_t str_has_prefix(const char *str, const char
*prefix)
{
size_t len = strlen(prefix);
return strncmp(str, prefix, len) == 0 ? len : 0;
}
So
len = str_has_prefix()
if (len) { use len }
is essentially
if (somecondition ? some-non-zero-constant : 0) { use
some-non-zero-constant }
which I'm fairly certain the compiler has no problem turning into
if (somecondition) { ... }
which is exactly the existing strstarts() code. So I wouldn't expect a
huge difference in generated code.
Rasmus
On Sat, 5 Dec 2020 at 21:24, James Bottomley
<[email protected]> wrote:
>
> On Sat, 2020-12-05 at 20:36 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 19:02, James Bottomley
> > <[email protected]> wrote:
> > > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > > On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> > > > wrote:
> > > > > From: Francis Laniel <[email protected]>
> > > > >
> > > > > 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.
[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
> <[email protected]> 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
On Sat, 5 Dec 2020 at 22:15, James Bottomley
<[email protected]> 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
> > <[email protected]> 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.
>
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.
On Sat, 2020-12-05 at 22:20 +0100, Ard Biesheuvel wrote:
> On Sat, 5 Dec 2020 at 22:15, James Bottomley
> <[email protected]> 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
> > > <[email protected]> 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
On Sat, 05 Dec 2020 15:04:31 -0800
James Bottomley <[email protected]> wrote:
> 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.
And one of the reasons we created str_has_prefix() is because we fixed that
exact bug, in a few places.
It was caused by a typo, where we had something like:
strstarts(option, "foo=") {
option += strlen("foo");
and forgot the "=" part, and broke the rest of the logic.
-- Steve
From: Steven Rostedt
> Sent: 07 December 2020 15:10
>
> On Sat, 05 Dec 2020 15:04:31 -0800
> James Bottomley <[email protected]> wrote:
>
> > 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.
>
> And one of the reasons we created str_has_prefix() is because we fixed that
> exact bug, in a few places.
>
> It was caused by a typo, where we had something like:
>
> strstarts(option, "foo=") {
> option += strlen("foo");
>
> and forgot the "=" part, and broke the rest of the logic.
And then someone else wonders whether the paint is the right colour.
Maybe the function should return the pointer to the character
after the prefix.
Suddenly you have a load of functions to pick from, none do
quite what you want - so you add yet another :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Sat, Dec 05, 2020 at 08:36:02PM +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <[email protected]> wrote:
> >
> > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > On Fri, 4 Dec 2020 at 18:06, <[email protected]>
> > > wrote:
> > > > From: Francis Laniel <[email protected]>
> > > >
> > > > 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.
The EFI stub is -ffreestanding, so you actually get multiple calls to
strlen() in any case. I could have used strncmp() directly with sizeof()
to avoid that, but the strstarts()/strlen() was slightly more readable
and the performance of this code doesn't really matter.
I wasn't aware of str_has_prefix() at the time. It does seem useful to
eliminate the duplication of the string literal, I like the
skip_prefix() API suggestion, maybe even
bool str_skip_prefix(const char **s, const char *pfx)
{
size_t len = str_has_prefix(*s, pfx);
*s += len;
return !!len;
}
...
if (str_skip_prefix(&option, prefix)) { ... }
to avoid the intermediate variable.
>
> 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.
On Fri, Dec 11, 2020 at 09:45:15AM +0000, David Laight wrote:
> From: Arvind Sankar
> > Sent: 10 December 2020 18:14
> ...
> > I wasn't aware of str_has_prefix() at the time. It does seem useful to
> > eliminate the duplication of the string literal, I like the
> > skip_prefix() API suggestion, maybe even
> >
> > bool str_skip_prefix(const char **s, const char *pfx)
> > {
> > size_t len = str_has_prefix(*s, pfx);
> > *s += len;
> > return !!len;
> > }
> > ...
> > if (str_skip_prefix(&option, prefix)) { ... }
> >
> > to avoid the intermediate variable.
>
> That'll generate horrid code - the 'option' variable has to be
> repeatedly reloaded from memory (unless it is all inlined).
>
> Perhaps the #define
>
> #define str_skip_prefix(str, prefix) \
> {( \
> size_t _pfx_len = strlen(prefix)); \
> memcmp(str, pfx, _pfx_len) ? 0 : ((str) += _pfx_len, 1); \
> )}
>
> There's probably something that'll let you use sizeof() instead
> of strlen() for quoted strings (if only sizeof pfx != sizeof (char *)).
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
Avoiding the intermediate varable is for readability at the call-site,
not performance. The performance of this function shouldn't matter --
you shouldn't be parsing strings in a hotpath anyway, and nobody cares
if the EFI stub takes a few nanoseconds longer if that makes the code
more robust and/or readable.
That said,
- I'd be surprised if the overhead of an L1D cache access was measurable
over the strncmp()/strlen() calls.
- You can't replace strncmp() with memcmp().
- Regarding sizeof() vs strlen(), you can simply use __builtin_strlen()
to optimize string literals, there's no need for hacks using the
preprocessor.
From: Arvind Sankar
> Sent: 10 December 2020 18:14
...
> I wasn't aware of str_has_prefix() at the time. It does seem useful to
> eliminate the duplication of the string literal, I like the
> skip_prefix() API suggestion, maybe even
>
> bool str_skip_prefix(const char **s, const char *pfx)
> {
> size_t len = str_has_prefix(*s, pfx);
> *s += len;
> return !!len;
> }
> ...
> if (str_skip_prefix(&option, prefix)) { ... }
>
> to avoid the intermediate variable.
That'll generate horrid code - the 'option' variable has to be
repeatedly reloaded from memory (unless it is all inlined).
Perhaps the #define
#define str_skip_prefix(str, prefix) \
{( \
size_t _pfx_len = strlen(prefix)); \
memcmp(str, pfx, _pfx_len) ? 0 : ((str) += _pfx_len, 1); \
)}
There's probably something that'll let you use sizeof() instead
of strlen() for quoted strings (if only sizeof pfx != sizeof (char *)).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)