[RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()

[RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

Hi.


First, I hope you are fine and the same for your relatives.

In this patch set, I replaced all calls to strstarts() by calls to
str_has_prefix().
Indeed, the kernel has two functions to test if a string begins with an other:
1. strstarts() which returns a bool, so 1 if the string begins with the prefix,
0 otherwise.
2. str_has_prefix() which returns the length of the prefix or 0.

str_has_prefix() was introduced later than strstarts(), in commit 495d714ad140
which also stated that str_has_prefix() should replace strstarts().
This is what this patch set does.

Concerning the patches, the modifications cover different areas of the kernel.
I compiled all of them and they compile smoothly.
Unfortunately, I did not test all of them, so here are the status of the patches
regarding test:
1. Tested with qemu-system-arm using insmod.
2. I do not have access to a bcm47xx MIPS CPU an qemu-system-mips does not
emulate this board.
3. Tested with qemu-system-x86_64 calling
crypto_alloc_skcipher("essiv(authenc(hmac(sha256),cbc(aes)),sha256)", 0, 0)
through LKDTM.
4. Tested with qemu-system-x86_64 using crypsetup.
5. I do not have access to a renesas board and I lack some context to test it
with qemu-system-arm.
6. I do not have access to an OMAP board and I lack some context to test it with
qemu-system-arm.
7. I did not find how to boot from the EFI_STUB with qemu. If you know how to
do, I would be happy to try running this code.
8. I ran qemu-system-x86_64 with a floppy disk attached but impossible to go
through this module code...
9. I do not have access to a bcm63xx MIPS CPU an qemu-system-mips does not
emulate this board.
10. Tested with qemu-system-x86_64 using insmod.
11. I do not have access to an AM335x or DA8xx platforms and I lack some context
to test it with qemu-system-arm.

If you see a way to improve the patches or if I did something wrong with the
mail do not hesitate to ask.


Best regards.

Francis Laniel (12):
  arm: Replace strstarts() by str_has_prefix().
  mips: Replace strstarts() by str_has_prefix().
  crypto: Replace strstarts() by str_has_prefix().
  device-mapper: Replace strstarts() by str_has_prefix().
  renesas: Replace strstarts() by str_has_prefix().
  omap: Replace strstarts() by str_has_prefix().
  efi: Replace strstarts() by str_has_prefix().
  ide: Replace strstarts() by str_has_prefix().
  mips: Replace strstarts() by str_has_prefix().
  module: Replace strstarts() by str_has_prefix().
  musb: Replace strstarts() by str_has_prefix().
  string.h: Remove strstarts().

 arch/arm/kernel/module.c                      | 12 +++++------
 arch/mips/bcm47xx/board.c                     |  4 ++--
 arch/mips/bcm63xx/boards/board_bcm963xx.c     |  2 +-
 crypto/essiv.c                                |  2 +-
 .../firmware/efi/libstub/efi-stub-helper.c    |  2 +-
 drivers/firmware/efi/libstub/gop.c            | 10 +++++-----
 drivers/gpu/drm/omapdrm/dss/base.c            |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  2 +-
 drivers/ide/ide-floppy.c                      |  4 ++--
 drivers/md/dm-crypt.c                         |  4 ++--
 drivers/usb/musb/musb_cppi41.c                |  4 ++--
 drivers/usb/musb/musb_debugfs.c               | 20 +++++++++----------
 include/linux/string.h                        | 10 ----------
 kernel/module.c                               |  6 +++---
 14 files changed, 37 insertions(+), 47 deletions(-)

-- 
2.20.1

[RFC PATCH v1 01/12] arm: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 arch/arm/kernel/module.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index e15444b25ca0..a8cbd040bcfc 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -56,16 +56,16 @@ void *module_alloc(unsigned long size)
 
 bool module_init_section(const char *name)
 {
-	return strstarts(name, ".init") ||
-		strstarts(name, ".ARM.extab.init") ||
-		strstarts(name, ".ARM.exidx.init");
+	return str_has_prefix(name, ".init") ||
+		str_has_prefix(name, ".ARM.extab.init") ||
+		str_has_prefix(name, ".ARM.exidx.init");
 }
 
 bool module_exit_section(const char *name)
 {
-	return strstarts(name, ".exit") ||
-		strstarts(name, ".ARM.extab.exit") ||
-		strstarts(name, ".ARM.exidx.exit");
+	return str_has_prefix(name, ".exit") ||
+		str_has_prefix(name, ".ARM.extab.exit") ||
+		str_has_prefix(name, ".ARM.exidx.exit");
 }
 
 int
-- 
2.20.1

[RFC PATCH v1 02/12] mips: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 arch/mips/bcm47xx/board.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/bcm47xx/board.c b/arch/mips/bcm47xx/board.c
index 35266a70e22a..c21a15b581f6 100644
--- a/arch/mips/bcm47xx/board.c
+++ b/arch/mips/bcm47xx/board.c
@@ -243,7 +243,7 @@ static __init const struct bcm47xx_board_type *bcm47xx_board_get_nvram(void)
 
 	if (bcm47xx_nvram_getenv("hardware_version", buf1, sizeof(buf1)) >= 0) {
 		for (e1 = bcm47xx_board_list_hardware_version; e1->value1; e1++) {
-			if (strstarts(buf1, e1->value1))
+			if (str_has_prefix(buf1, e1->value1))
 				return &e1->board;
 		}
 	}
@@ -251,7 +251,7 @@ static __init const struct bcm47xx_board_type *bcm47xx_board_get_nvram(void)
 	if (bcm47xx_nvram_getenv("hardware_version", buf1, sizeof(buf1)) >= 0 &&
 	    bcm47xx_nvram_getenv("boardnum", buf2, sizeof(buf2)) >= 0) {
 		for (e2 = bcm47xx_board_list_hw_version_num; e2->value1; e2++) {
-			if (!strstarts(buf1, e2->value1) &&
+			if (!str_has_prefix(buf1, e2->value1) &&
 			    !strcmp(buf2, e2->value2))
 				return &e2->board;
 		}
-- 
2.20.1

[RFC PATCH v1 03/12] crypto: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 crypto/essiv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/essiv.c b/crypto/essiv.c
index d012be23d496..f85d4416891f 100644
--- a/crypto/essiv.c
+++ b/crypto/essiv.c
@@ -504,7 +504,7 @@ static int essiv_create(struct crypto_template *tmpl, struct rtattr **tb)
 			goto out_free_inst;
 		aead_alg = crypto_spawn_aead_alg(&ictx->u.aead_spawn);
 		block_base = &aead_alg->base;
-		if (!strstarts(block_base->cra_name, "authenc(")) {
+		if (!str_has_prefix(block_base->cra_name, "authenc(")) {
 			pr_warn("Only authenc() type AEADs are supported by ESSIV\n");
 			err = -EINVAL;
 			goto out_drop_skcipher;
-- 
2.20.1

[RFC PATCH v1 04/12] device-mapper: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 drivers/md/dm-crypt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 392337f16ecf..b6f31b662d93 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2659,7 +2659,7 @@ static int crypt_ctr_auth_cipher(struct crypt_config *cc, char *cipher_api)
 	char *start, *end, *mac_alg = NULL;
 	struct crypto_ahash *mac;
 
-	if (!strstarts(cipher_api, "authenc("))
+	if (!str_has_prefix(cipher_api, "authenc("))
 		return 0;
 
 	start = strchr(cipher_api, '(');
@@ -2858,7 +2858,7 @@ static int crypt_ctr_cipher(struct dm_target *ti, char *cipher_in, char *key)
 		return -ENOMEM;
 	}
 
-	if (strstarts(cipher_in, "capi:"))
+	if (str_has_prefix(cipher_in, "capi:"))
 		ret = crypt_ctr_cipher_new(ti, cipher_in, key, &ivmode, &ivopts);
 	else
 		ret = crypt_ctr_cipher_old(ti, cipher_in, key, &ivmode, &ivopts);
-- 
2.20.1

[RFC PATCH v1 05/12] renesas: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index fe86a3e67757..3f9972165afa 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -1016,7 +1016,7 @@ static int rcar_du_crtc_parse_crc_source(struct rcar_du_crtc *rcrtc,
 	} else if (!strcmp(source_name, "auto")) {
 		*source = VSP1_DU_CRC_OUTPUT;
 		return 0;
-	} else if (strstarts(source_name, "plane")) {
+	} else if (str_has_prefix(source_name, "plane")) {
 		unsigned int i;
 
 		*source = VSP1_DU_CRC_PLANE;
-- 
2.20.1

[RFC PATCH v1 06/12] omap: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 drivers/gpu/drm/omapdrm/dss/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
index c7650a7c155d..dd3d466293d1 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -350,7 +350,7 @@ static bool omapdss_component_is_loaded(struct omapdss_comp_node *comp)
 {
 	if (comp->dss_core_component)
 		return true;
-	if (!strstarts(comp->compat, "omapdss,"))
+	if (!str_has_prefix(comp->compat, "omapdss,"))
 		return true;
 	if (omapdss_device_is_registered(comp->node))
 		return true;
-- 
2.20.1

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

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 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

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

[Ard Biesheuvel]

On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com> wrote:
>
> From: Francis Laniel <laniel_francis@privacyrequired.com>
>
> 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 <laniel_francis@privacyrequired.com>
> ---
>  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
>

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

[Francis Laniel]

Le vendredi 4 décembre 2020, 18:07:11 CET Ard Biesheuvel a écrit :
> On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com> wrote:
> > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > 
> > 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 <laniel_francis@privacyrequired.com>
> > ---
> > 
> >  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

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

[James Bottomley]

On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> wrote:
> > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > 
> > 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

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

[Francis Laniel]

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, <laniel_francis@privacyrequired.com>
> > 
> > wrote:
> > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > 
> > > 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

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

[Ard Biesheuvel]

On Fri, 4 Dec 2020 at 19:02, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> > wrote:
> > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > >
> > > 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.

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

[James Bottomley]

On Sat, 2020-12-05 at 20:36 +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> > > wrote:
> > > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > > 
> > > > 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

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@hansenpartnership.com> 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@hansenpartnership.com> wrote:
> > > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> > > > wrote:
> > > > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > > >
> > > > > 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.

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

[James Bottomley]

[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

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

[Ard Biesheuvel]

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

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

[James Bottomley]

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

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

[Steven Rostedt]

On Sat, 05 Dec 2020 15:04:31 -0800
James Bottomley <James.Bottomley@HansenPartnership.com> 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

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

[David Laight]

From: Steven Rostedt
> Sent: 07 December 2020 15:10

> 
> On Sat, 05 Dec 2020 15:04:31 -0800
> James Bottomley <James.Bottomley@HansenPartnership.com> 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)

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

[Rasmus Villemoes]

On 05/12/2020 20.36, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
>>
>> On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
>>> On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
>>> wrote:
>>>> From: Francis Laniel <laniel_francis@privacyrequired.com>
>>>>
>>>> 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

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

[Arvind Sankar]

On Sat, Dec 05, 2020 at 08:36:02PM +0100, Ard Biesheuvel wrote:
> On Fri, 4 Dec 2020 at 19:02, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> > > wrote:
> > > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > >
> > > > 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.

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

[David Laight]

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)

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

[Arvind Sankar]

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.

[RFC PATCH v1 08/12] ide: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 drivers/ide/ide-floppy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index f5a2870aaf54..c0b1080e458d 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -495,7 +495,7 @@ static void ide_floppy_setup(ide_drive_t *drive)
 	 * it. It should be fixed as of version 1.9, but to be on the safe side
 	 * we'll leave the limitation below for the 2.2.x tree.
 	 */
-	if (strstarts((char *)&id[ATA_ID_PROD], "IOMEGA ZIP 100 ATAPI")) {
+	if (str_has_prefix((char *)&id[ATA_ID_PROD], "IOMEGA ZIP 100 ATAPI")) {
 		drive->atapi_flags |= IDE_AFLAG_ZIP_DRIVE;
 		/* This value will be visible in the /proc/ide/hdx/settings */
 		drive->pc_delay = IDEFLOPPY_PC_DELAY;
@@ -506,7 +506,7 @@ static void ide_floppy_setup(ide_drive_t *drive)
 	 * Guess what? The IOMEGA Clik! drive also needs the above fix. It makes
 	 * nasty clicking noises without it, so please don't remove this.
 	 */
-	if (strstarts((char *)&id[ATA_ID_PROD], "IOMEGA Clik!")) {
+	if (str_has_prefix((char *)&id[ATA_ID_PROD], "IOMEGA Clik!")) {
 		blk_queue_max_hw_sectors(drive->queue, 64);
 		drive->atapi_flags |= IDE_AFLAG_CLIK_DRIVE;
 		/* IOMEGA Clik! drives do not support lock/unlock commands */
-- 
2.20.1

[RFC PATCH v1 09/12] mips: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 arch/mips/bcm63xx/boards/board_bcm963xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/bcm63xx/boards/board_bcm963xx.c b/arch/mips/bcm63xx/boards/board_bcm963xx.c
index 01aff80a5967..85ccb2b02621 100644
--- a/arch/mips/bcm63xx/boards/board_bcm963xx.c
+++ b/arch/mips/bcm63xx/boards/board_bcm963xx.c
@@ -747,7 +747,7 @@ void __init board_prom_init(void)
 
 	/* dump cfe version */
 	cfe = boot_addr + BCM963XX_CFE_VERSION_OFFSET;
-	if (strstarts(cfe, "cfe-")) {
+	if (str_has_prefix(cfe, "cfe-")) {
 		if(cfe[4] == 'v') {
 			if(cfe[5] == 'd')
 				snprintf(cfe_version, 11, "%s",
-- 
2.20.1

[RFC PATCH v1 10/12] module: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 kernel/module.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..d01466f1d2a6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2675,7 +2675,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
 		else
 			return 'b';
 	}
-	if (strstarts(info->secstrings + sechdrs[sym->st_shndx].sh_name,
+	if (str_has_prefix(info->secstrings + sechdrs[sym->st_shndx].sh_name,
 		      ".debug")) {
 		return 'n';
 	}
@@ -2842,12 +2842,12 @@ void * __weak module_alloc(unsigned long size)
 
 bool __weak module_init_section(const char *name)
 {
-	return strstarts(name, ".init");
+	return str_has_prefix(name, ".init");
 }
 
 bool __weak module_exit_section(const char *name)
 {
-	return strstarts(name, ".exit");
+	return str_has_prefix(name, ".exit");
 }
 
 #ifdef CONFIG_DEBUG_KMEMLEAK
-- 
2.20.1

[RFC PATCH v1 11/12] musb: Replace strstarts() by str_has_prefix().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

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 <laniel_francis@privacyrequired.com>
---
 drivers/usb/musb/musb_cppi41.c  |  4 ++--
 drivers/usb/musb/musb_debugfs.c | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 7fbb8a307145..a6d22c0957c5 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -686,9 +686,9 @@ static int cppi41_dma_controller_start(struct cppi41_dma_controller *controller)
 		ret = of_property_read_string_index(np, "dma-names", i, &str);
 		if (ret)
 			goto err;
-		if (strstarts(str, "tx"))
+		if (str_has_prefix(str, "tx"))
 			is_tx = 1;
-		else if (strstarts(str, "rx"))
+		else if (str_has_prefix(str, "rx"))
 			is_tx = 0;
 		else {
 			dev_err(dev, "Wrong dmatype %s\n", str);
diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
index 30a89aa8a3e7..47fc32bc6507 100644
--- a/drivers/usb/musb/musb_debugfs.c
+++ b/drivers/usb/musb/musb_debugfs.c
@@ -181,36 +181,36 @@ static ssize_t musb_test_mode_write(struct file *file,
 		goto ret;
 	}
 
-	if (strstarts(buf, "force host full-speed"))
+	if (str_has_prefix(buf, "force host full-speed"))
 		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
 
-	else if (strstarts(buf, "force host high-speed"))
+	else if (str_has_prefix(buf, "force host high-speed"))
 		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
 
-	else if (strstarts(buf, "force host"))
+	else if (str_has_prefix(buf, "force host"))
 		test = MUSB_TEST_FORCE_HOST;
 
-	else if (strstarts(buf, "fifo access"))
+	else if (str_has_prefix(buf, "fifo access"))
 		test = MUSB_TEST_FIFO_ACCESS;
 
-	else if (strstarts(buf, "force full-speed"))
+	else if (str_has_prefix(buf, "force full-speed"))
 		test = MUSB_TEST_FORCE_FS;
 
-	else if (strstarts(buf, "force high-speed"))
+	else if (str_has_prefix(buf, "force high-speed"))
 		test = MUSB_TEST_FORCE_HS;
 
-	else if (strstarts(buf, "test packet")) {
+	else if (str_has_prefix(buf, "test packet")) {
 		test = MUSB_TEST_PACKET;
 		musb_load_testpacket(musb);
 	}
 
-	else if (strstarts(buf, "test K"))
+	else if (str_has_prefix(buf, "test K"))
 		test = MUSB_TEST_K;
 
-	else if (strstarts(buf, "test J"))
+	else if (str_has_prefix(buf, "test J"))
 		test = MUSB_TEST_J;
 
-	else if (strstarts(buf, "test SE0 NAK"))
+	else if (str_has_prefix(buf, "test SE0 NAK"))
 		test = MUSB_TEST_SE0_NAK;
 
 	musb_writeb(musb->mregs, MUSB_TESTMODE, test);
-- 
2.20.1

[RFC PATCH v1 12/12] string.h: Remove strstarts().

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

When str_has_prefix() was introduced in commit 495d714ad140 ("Merge tag
'trace-v4.21' of
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace"), this commit
stated that the new function should replace the existing one.

Signed-off-by: Francis Laniel <laniel_francis@privacyrequired.com>
---
 include/linux/string.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b1f3894a0a3e..78bcae9bfa58 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -212,16 +212,6 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
 
 int ptr_to_hashval(const void *ptr, unsigned long *hashval_out);
 
-/**
- * strstarts - does @str start with @prefix?
- * @str: string to examine
- * @prefix: prefix to look for.
- */
-static inline bool strstarts(const char *str, const char *prefix)
-{
-	return strncmp(str, prefix, strlen(prefix)) == 0;
-}
-
 size_t memweight(const void *ptr, size_t bytes);
 
 /**
-- 
2.20.1

Re: [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()

[James Bottomley]

On Fri, 2020-12-04 at 18:03 +0100, laniel_francis@privacyrequired.com
wrote:
> In this patch set, I replaced all calls to strstarts() by calls to
> str_has_prefix(). Indeed, the kernel has two functions to test if a
> string begins with an other:
> 1. strstarts() which returns a bool, so 1 if the string begins with
> the prefix,0 otherwise.
> 2. str_has_prefix() which returns the length of the prefix or 0.
> 
> str_has_prefix() was introduced later than strstarts(), in commit
> 495d714ad140 which also stated that str_has_prefix() should replace
> strstarts(). This is what this patch set does.

What's the reason why?  If you look at the use cases for the
replacement of strstart()  they're all cases where we need to know the
length we're skipping and this is hard coded, leading to potential
errors later.  This is a classic example:  3d739c1f6156 ("tracing: Use
the return of str_has_prefix() to remove open coded numbers").  However
you're not doing this transformation in the conversion, so the
conversion is pretty useless.  I also see no case for replacing
strstart() where we're using it simply as a boolean without needing to
know the length of the prefix.

James