[PATCH 1/3] fastboot: print all variables only on getvar:all and not its prefixes

[PATCH 1/3] fastboot: print all variables only on getvar:all and not its prefixes

[Ahmad Fatoum]

strcmp_l1 compares up to the length of the first arguments, i.e. it does
a prefix check. For this, the prefix, which is usually a string literal,
needs to be the first argument.

The check for getvar:all doesn't follow this with the result that all of

  fastboot getvar 'all'
  fastboot getvar 'al'
  fastboot getvar 'a'
  fastboot getvar ''

behave the same. This undocumented quirk is most likely unintended, so
let's replace this with an actual equality check.

Note that strcmp_l1 also does a NULL-ness check. This is safe to remove,
as explained in the follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 common/fastboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index 532286703089..e85cc6d8aaf8 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -312,7 +312,7 @@ static void cb_getvar(struct fastboot *fb, const char *cmd)
 
 	pr_debug("getvar: \"%s\"\n", cmd);
 
-	if (!strcmp_l1(cmd, "all")) {
+	if (!strcmp(cmd, "all")) {
 		list_for_each_entry(var, &fb->variables, list)
 			fastboot_tx_print(fb, FASTBOOT_MSG_INFO, "%s: %s",
 					  var->name, var->value);
-- 
2.39.2

[PATCH 2/3] fastboot: retire strcmp_l1 in favor of str_has_prefix

[Ahmad Fatoum]

strcmp_l1 is basically str_has_prefix with inverted arguments and a
NULL check.

We don't need the NULL check as cmdbuf and cmd->cmd should always be
non-NULL: The former is the address of an array populated by fastboot
network or USB code and the latter is a pointer to a string literal.

So let's codify the assumption that cmdbuf is not NULL into the
prototype and replace strcmp_l1 with str_has_prefix. This has the added
benefit that str_has_prefix returns the length of the prefix, which
saves us a repeated call to strlen(cmd->cmd);

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 common/fastboot.c  | 14 +++++---------
 include/fastboot.h |  3 ++-
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index e85cc6d8aaf8..dc66d7123b02 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -287,13 +287,6 @@ static void cb_reboot(struct fastboot *fb, const char *cmd)
 	restart_machine();
 }
 
-static int strcmp_l1(const char *s1, const char *s2)
-{
-	if (!s1 || !s2)
-		return -1;
-	return strncmp(s1, s2, strlen(s1));
-}
-
 static void cb_getvar(struct fastboot *fb, const char *cmd)
 {
 	struct fb_variable *var;
@@ -815,10 +808,13 @@ static void fb_run_command(struct fastboot *fb, const char *cmdbuf,
 	console_countdown_abort("fastboot");
 
 	for (i = 0; i < num_commands; i++) {
+		size_t cmdlen;
+
 		cmd = &cmds[i];
 
-		if (!strcmp_l1(cmd->cmd, cmdbuf)) {
-			cmd->cb(fb, cmdbuf + strlen(cmd->cmd));
+		cmdlen = str_has_prefix(cmdbuf, cmd->cmd);
+		if (cmdlen) {
+			cmd->cb(fb, cmdbuf + cmdlen);
 
 			return;
 		}
diff --git a/include/fastboot.h b/include/fastboot.h
index cd415847e348..4b2fdf3190b2 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -87,7 +87,8 @@ int fastboot_tx_print(struct fastboot *fb, enum fastboot_msg_type type,
 		      const char *fmt, ...);
 void fastboot_start_download_generic(struct fastboot *fb);
 void fastboot_download_finished(struct fastboot *fb);
-void fastboot_exec_cmd(struct fastboot *fb, const char *cmdbuf);
+void fastboot_exec_cmd(struct fastboot *fb, const char *cmdbuf)
+        __attribute__((nonnull));
 void fastboot_abort(struct fastboot *fb);
 
 #endif
-- 
2.39.2

[PATCH 3/3] fastboot: avoid console_countdown_abort for getvar request

[Ahmad Fatoum]

We currently abort boot countdown on any fastboot communication at all
as we have the expectation that this is what the user wants.

This doesn't hold true on systems with fwupd: The fastboot plugin probes
connected devices:

  getvar:product
  getvar:version
  getvar:version-bootloader
  getvar:serialno
  getvar:secure

to determine whether an update is in-order. The first getvar will
automatically abort barebox boot up, which is likely not what the user
intended.

Therefore, let's abort console countdown only for non-getvar: requests.

Reported-by: Jonas Martin <j.martin@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Cc: lgo@pengutronix.de
Cc: jlu@pengutronix.de
v1 -> v2:
  - add rationale for the change (fwupd)
  - use strstarts instead of strcmp_l1 removed in earlier patch
---
 common/fastboot.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index dc66d7123b02..66b59ab9b0d7 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -794,6 +794,11 @@ static void cb_erase(struct fastboot *fb, const char *cmd)
 		fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, "");
 }
 
+static bool fastboot_cmd_should_abort(const char *cmdbuf)
+{
+	return !strstarts(cmdbuf, "getvar:");
+}
+
 struct cmd_dispatch_info {
 	char *cmd;
 	void (*cb)(struct fastboot *fb, const char *opt);
@@ -805,7 +810,8 @@ static void fb_run_command(struct fastboot *fb, const char *cmdbuf,
 	const struct cmd_dispatch_info *cmd;
 	int i;
 
-	console_countdown_abort("fastboot");
+	if (fastboot_cmd_should_abort(cmdbuf))
+		console_countdown_abort("fastboot");
 
 	for (i = 0; i < num_commands; i++) {
 		size_t cmdlen;
-- 
2.39.2

Re: [PATCH 1/3] fastboot: print all variables only on getvar:all and not its prefixes

[Sascha Hauer]

On Fri, 09 Aug 2024 16:19:57 +0200, Ahmad Fatoum wrote:
> strcmp_l1 compares up to the length of the first arguments, i.e. it does
> a prefix check. For this, the prefix, which is usually a string literal,
> needs to be the first argument.
> 
> The check for getvar:all doesn't follow this with the result that all of
> 
>   fastboot getvar 'all'
>   fastboot getvar 'al'
>   fastboot getvar 'a'
>   fastboot getvar ''
> 
> [...]

Applied, thanks!

[1/3] fastboot: print all variables only on getvar:all and not its prefixes
      https://git.pengutronix.de/cgit/barebox/commit/?id=aff71a944811 (link may not be stable)
[2/3] fastboot: retire strcmp_l1 in favor of str_has_prefix
      https://git.pengutronix.de/cgit/barebox/commit/?id=bec2074b771d (link may not be stable)
[3/3] fastboot: avoid console_countdown_abort for getvar request
      https://git.pengutronix.de/cgit/barebox/commit/?id=23f330f1c8f3 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>