linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()
@ 2020-12-04 17:03 laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 01/12] arm: " laniel_francis
                   ` (12 more replies)
  0 siblings, 13 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Russell King, Hauke Mehrtens, Rafał Miłecki,
	Thomas Bogendoerfer, Florian Fainelli, bcm-kernel-feedback-list,
	Herbert Xu, David S. Miller, Ard Biesheuvel, Tomi Valkeinen,
	David Airlie, Daniel Vetter, Laurent Pinchart, Kieran Bingham,
	Alasdair Kergon, Mike Snitzer, dm-devel, Bin Liu,
	Greg Kroah-Hartman, Jessica Yu
  Cc: Francis Laniel, linux-arm-kernel, linux-kernel, linux-mips,
	linux-crypto, linux-efi, dri-devel, linux-renesas-soc, linux-ide,
	linux-usb

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 01/12] arm: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 02/12] mips: " laniel_francis
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Russell King; +Cc: Francis Laniel, linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 02/12] mips: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 01/12] arm: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 03/12] crypto: " laniel_francis
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Hauke Mehrtens, Rafał Miłecki, Thomas Bogendoerfer
  Cc: Francis Laniel, linux-mips, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 03/12] crypto: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 01/12] arm: " laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 02/12] mips: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 04/12] device-mapper: " laniel_francis
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Herbert Xu, David S. Miller; +Cc: Francis Laniel, linux-crypto, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 04/12] device-mapper: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (2 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 03/12] crypto: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 05/12] renesas: " laniel_francis
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Alasdair Kergon, Mike Snitzer, dm-devel; +Cc: Francis Laniel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 05/12] renesas: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (3 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 04/12] device-mapper: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 06/12] omap: " laniel_francis
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, David Airlie, Daniel Vetter
  Cc: Francis Laniel, dri-devel, linux-renesas-soc, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 06/12] omap: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (4 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 05/12] renesas: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 07/12] efi: " laniel_francis
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Tomi Valkeinen, David Airlie, Daniel Vetter
  Cc: Francis Laniel, dri-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (5 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 06/12] omap: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:07   ` Ard Biesheuvel
  2020-12-04 17:03 ` [RFC PATCH v1 08/12] ide: " laniel_francis
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Francis Laniel, linux-efi, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 08/12] ide: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (6 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 07/12] efi: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 09/12] mips: " laniel_francis
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: David S. Miller; +Cc: Francis Laniel, linux-ide, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 09/12] mips: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (7 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 08/12] ide: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 10/12] module: " laniel_francis
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Florian Fainelli, bcm-kernel-feedback-list
  Cc: Francis Laniel, linux-mips, linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 10/12] module: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (8 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 09/12] mips: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 11/12] musb: " laniel_francis
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Francis Laniel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 11/12] musb: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (9 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 10/12] module: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:03 ` [RFC PATCH v1 12/12] string.h: Remove strstarts() laniel_francis
  2020-12-04 17:56 ` [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() James Bottomley
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  To: Bin Liu, Greg Kroah-Hartman; +Cc: Francis Laniel, linux-usb, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH v1 12/12] string.h: Remove strstarts().
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (10 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 11/12] musb: " laniel_francis
@ 2020-12-04 17:03 ` laniel_francis
  2020-12-04 17:56 ` [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() James Bottomley
  12 siblings, 0 replies; 30+ messages in thread
From: laniel_francis @ 2020-12-04 17:03 UTC (permalink / raw)
  Cc: Francis Laniel, linux-kernel

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 17:03 ` [RFC PATCH v1 07/12] efi: " laniel_francis
@ 2020-12-04 17:07   ` Ard Biesheuvel
  2020-12-04 17:19     ` Francis Laniel
  2020-12-04 18:02     ` James Bottomley
  0 siblings, 2 replies; 30+ messages in thread
From: Ard Biesheuvel @ 2020-12-04 17:07 UTC (permalink / raw)
  To: laniel_francis; +Cc: linux-efi, Linux Kernel Mailing List

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
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 17:07   ` Ard Biesheuvel
@ 2020-12-04 17:19     ` Francis Laniel
  2020-12-04 18:02     ` James Bottomley
  1 sibling, 0 replies; 30+ messages in thread
From: Francis Laniel @ 2020-12-04 17:19 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-efi, Linux Kernel Mailing List

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





^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix()
  2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
                   ` (11 preceding siblings ...)
  2020-12-04 17:03 ` [RFC PATCH v1 12/12] string.h: Remove strstarts() laniel_francis
@ 2020-12-04 17:56 ` James Bottomley
  12 siblings, 0 replies; 30+ messages in thread
From: James Bottomley @ 2020-12-04 17:56 UTC (permalink / raw)
  To: laniel_francis, Russell King, Hauke Mehrtens,
	Rafał Miłecki, Thomas Bogendoerfer, Florian Fainelli,
	bcm-kernel-feedback-list, Herbert Xu, David S. Miller,
	Ard Biesheuvel, Tomi Valkeinen, David Airlie, Daniel Vetter,
	Laurent Pinchart, Kieran Bingham, Alasdair Kergon, Mike Snitzer,
	dm-devel, Bin Liu, Greg Kroah-Hartman, Jessica Yu
  Cc: linux-arm-kernel, linux-kernel, linux-mips, linux-crypto,
	linux-efi, dri-devel, linux-renesas-soc, linux-ide, linux-usb

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 17:07   ` Ard Biesheuvel
  2020-12-04 17:19     ` Francis Laniel
@ 2020-12-04 18:02     ` James Bottomley
  2020-12-05 19:08       ` Francis Laniel
  2020-12-05 19:36       ` Ard Biesheuvel
  1 sibling, 2 replies; 30+ messages in thread
From: James Bottomley @ 2020-12-04 18:02 UTC (permalink / raw)
  To: Ard Biesheuvel, laniel_francis; +Cc: linux-efi, Linux Kernel Mailing List

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 18:02     ` James Bottomley
@ 2020-12-05 19:08       ` Francis Laniel
  2020-12-05 19:36       ` Ard Biesheuvel
  1 sibling, 0 replies; 30+ messages in thread
From: Francis Laniel @ 2020-12-05 19:08 UTC (permalink / raw)
  To: James Bottomley; +Cc: Ard Biesheuvel, linux-efi, Linux Kernel Mailing List

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-04 18:02     ` James Bottomley
  2020-12-05 19:08       ` Francis Laniel
@ 2020-12-05 19:36       ` Ard Biesheuvel
  2020-12-05 20:24         ` James Bottomley
                           ` (2 more replies)
  1 sibling, 3 replies; 30+ messages in thread
From: Ard Biesheuvel @ 2020-12-05 19:36 UTC (permalink / raw)
  To: James Bottomley; +Cc: laniel_francis, linux-efi, Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 19:36       ` Ard Biesheuvel
@ 2020-12-05 20:24         ` James Bottomley
  2020-12-05 20:57           ` Ard Biesheuvel
  2020-12-05 20:28         ` Rasmus Villemoes
  2020-12-10 18:14         ` Arvind Sankar
  2 siblings, 1 reply; 30+ messages in thread
From: James Bottomley @ 2020-12-05 20:24 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: laniel_francis, linux-efi, Linux Kernel Mailing List

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 19:36       ` Ard Biesheuvel
  2020-12-05 20:24         ` James Bottomley
@ 2020-12-05 20:28         ` Rasmus Villemoes
  2020-12-10 18:14         ` Arvind Sankar
  2 siblings, 0 replies; 30+ messages in thread
From: Rasmus Villemoes @ 2020-12-05 20:28 UTC (permalink / raw)
  To: Ard Biesheuvel, James Bottomley
  Cc: laniel_francis, linux-efi, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 20:24         ` James Bottomley
@ 2020-12-05 20:57           ` Ard Biesheuvel
  2020-12-05 21:15             ` James Bottomley
  0 siblings, 1 reply; 30+ messages in thread
From: Ard Biesheuvel @ 2020-12-05 20:57 UTC (permalink / raw)
  To: James Bottomley; +Cc: laniel_francis, linux-efi, Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 20:57           ` Ard Biesheuvel
@ 2020-12-05 21:15             ` James Bottomley
  2020-12-05 21:20               ` Ard Biesheuvel
  0 siblings, 1 reply; 30+ messages in thread
From: James Bottomley @ 2020-12-05 21:15 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: laniel_francis, linux-efi, Linux Kernel Mailing List, Steven Rostedt

[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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 21:15             ` James Bottomley
@ 2020-12-05 21:20               ` Ard Biesheuvel
  2020-12-05 23:04                 ` James Bottomley
  0 siblings, 1 reply; 30+ messages in thread
From: Ard Biesheuvel @ 2020-12-05 21:20 UTC (permalink / raw)
  To: James Bottomley
  Cc: laniel_francis, linux-efi, Linux Kernel Mailing List, Steven Rostedt

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 21:20               ` Ard Biesheuvel
@ 2020-12-05 23:04                 ` James Bottomley
  2020-12-07 15:10                   ` Steven Rostedt
  0 siblings, 1 reply; 30+ messages in thread
From: James Bottomley @ 2020-12-05 23:04 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: laniel_francis, linux-efi, Linux Kernel Mailing List, Steven Rostedt

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 23:04                 ` James Bottomley
@ 2020-12-07 15:10                   ` Steven Rostedt
  2020-12-07 16:25                     ` David Laight
  0 siblings, 1 reply; 30+ messages in thread
From: Steven Rostedt @ 2020-12-07 15:10 UTC (permalink / raw)
  To: James Bottomley
  Cc: Ard Biesheuvel, laniel_francis, linux-efi, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-07 15:10                   ` Steven Rostedt
@ 2020-12-07 16:25                     ` David Laight
  0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2020-12-07 16:25 UTC (permalink / raw)
  To: 'Steven Rostedt', James Bottomley
  Cc: Ard Biesheuvel, laniel_francis, linux-efi, Linux Kernel Mailing List

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)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-05 19:36       ` Ard Biesheuvel
  2020-12-05 20:24         ` James Bottomley
  2020-12-05 20:28         ` Rasmus Villemoes
@ 2020-12-10 18:14         ` Arvind Sankar
  2020-12-11  9:45           ` David Laight
  2 siblings, 1 reply; 30+ messages in thread
From: Arvind Sankar @ 2020-12-10 18:14 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: James Bottomley, laniel_francis, linux-efi, Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-10 18:14         ` Arvind Sankar
@ 2020-12-11  9:45           ` David Laight
  2020-12-11 16:10             ` Arvind Sankar
  0 siblings, 1 reply; 30+ messages in thread
From: David Laight @ 2020-12-11  9:45 UTC (permalink / raw)
  To: 'Arvind Sankar', Ard Biesheuvel
  Cc: James Bottomley, laniel_francis, linux-efi, Linux Kernel Mailing List

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)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
  2020-12-11  9:45           ` David Laight
@ 2020-12-11 16:10             ` Arvind Sankar
  0 siblings, 0 replies; 30+ messages in thread
From: Arvind Sankar @ 2020-12-11 16:10 UTC (permalink / raw)
  To: David Laight
  Cc: 'Arvind Sankar',
	Ard Biesheuvel, James Bottomley, laniel_francis, linux-efi,
	Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-12-11 17:20 UTC | newest]

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

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