Merge patch series "cmd/setexpr: support concatenation of direct strings"

Heinrich Schuchardt <heinrich.schuchardt@canonical.com> says:

The setexpr.s command allows to concatenate two strings.

According to the description in doc/usage/cmd/setexpr.rst the parameters
value1 and value2 can be either direct values or pointers to a
memory location holding the values.

Unfortunately `setexpr.s <value1> + <value2>` fails if any of the values
is a direct value. $? is set to false.

* Add support for direct values in setexpr.s.
* Correct the unit test for "setexpr.s fred 0".
* Add a new unit test for "setexpr.s fred '1' + '3'" giving '13'.
* Remove invalid memory leak tests

Link: https://lore.kernel.org/r/20250203151029.60265-1-heinrich.schuchardt@canonical.com

2 files changed, 45 insertions, 55 deletions

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index e111b8ba98a..c45fa85c887 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -35,9 +35,37 @@ struct expr_arg {
 	};
 };
 
+/**
+ * arg_set_str() - copy string to expression argument
+ *
+ * The string is truncated to 64 KiB plus NUL terminator.
+ *
+ * @p:		pointer to string
+ * @argp:	pointer to expression argument
+ * Return:	0 on success, -ENOMEM if out of memory
+ */
+static int arg_set_str(void *p, struct expr_arg *argp)
+{
+	int len;
+	char *str;
+
+	/* Maximum string length of 64 KiB plus NUL terminator */
+	len = strnlen((char *)p, SZ_64K) + 1;
+	str = malloc(len);
+	if (!str) {
+		printf("Out of memory\n");
+		return -ENOMEM;
+	}
+	memcpy(str, p, len);
+	str[len - 1] = '\0';
+	argp->sval = str;
+	return 0;
+}
+
 static int get_arg(char *s, int w, struct expr_arg *argp)
 {
 	struct expr_arg arg;
+	int ret;
 
 	/*
 	 * If the parameter starts with a '*' then assume it is a pointer to
@@ -47,8 +75,6 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 		ulong *p;
 		ulong addr;
 		ulong val;
-		int len;
-		char *str;
 
 		addr = hextoul(&s[1], NULL);
 		switch (w) {
@@ -66,18 +92,10 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 			break;
 		case CMD_DATA_SIZE_STR:
 			p = map_sysmem(addr, SZ_64K);
-
-			/* Maximum string length of 64KB plus terminator */
-			len = strnlen((char *)p, SZ_64K) + 1;
-			str = malloc(len);
-			if (!str) {
-				printf("Out of memory\n");
-				return -ENOMEM;
-			}
-			memcpy(str, p, len);
-			str[len - 1] = '\0';
+			ret = arg_set_str(p, &arg);
 			unmap_sysmem(p);
-			arg.sval = str;
+			if (ret)
+				return ret;
 			break;
 		case 4:
 			p = map_sysmem(addr, sizeof(u32));
@@ -93,9 +111,13 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 			break;
 		}
 	} else {
-		if (w == CMD_DATA_SIZE_STR)
-			return -EINVAL;
-		arg.ival = hextoul(s, NULL);
+		if (w == CMD_DATA_SIZE_STR) {
+			ret = arg_set_str(s, &arg);
+			if (ret)
+				return ret;
+		} else {
+			arg.ival = hextoul(s, NULL);
+		}
 	}
 	*argp = arg;
 
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index 7f318a42ead..5e9b577fe36 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -297,31 +297,19 @@ SETEXPR_TEST(setexpr_test_backref, UTF_CONSOLE);
 /* Test 'setexpr' command with setting strings */
 static int setexpr_test_str(struct unit_test_state *uts)
 {
-	ulong start_mem;
 	char *buf;
 
 	buf = map_sysmem(0, BUF_SIZE);
 	memset(buf, '\xff', BUF_SIZE);
 
-	/*
-	 * Set 'fred' to the same length as we expect to get below, to avoid a
-	 * new allocation in 'setexpr'. That way we can check for memory leaks.
-	 */
 	ut_assertok(env_set("fred", "x"));
-	start_mem = ut_check_free();
-	strcpy(buf, "hello");
-	ut_asserteq(1, run_command("setexpr.s fred 0", 0));
-	ut_assertok(ut_check_delta(start_mem));
+	ut_asserteq(0, run_command("setexpr.s fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
 
+	strcpy(buf, "hello");
 	ut_assertok(env_set("fred", "12345"));
-	start_mem = ut_check_free();
 	ut_assertok(run_command("setexpr.s fred *0", 0));
 	ut_asserteq_str("hello", env_get("fred"));
-	/*
-	 * This fails in CI at present.
-	 *
-	 * ut_assertok(ut_check_delta(start_mem));
-	 */
 
 	unmap_sysmem(buf);
 
@@ -332,45 +320,25 @@ SETEXPR_TEST(setexpr_test_str, UTF_CONSOLE);
 /* Test 'setexpr' command with concatenating strings */
 static int setexpr_test_str_oper(struct unit_test_state *uts)
 {
-	ulong start_mem;
 	char *buf;
 
+	/* Test concatenation of strings */
+	ut_assertok(run_command("setexpr.s fred '1' + '3'", 0));
+	ut_asserteq_str("13", env_get("fred"));
+
 	buf = map_sysmem(0, BUF_SIZE);
 	memset(buf, '\xff', BUF_SIZE);
 	strcpy(buf, "hello");
 	strcpy(buf + 0x10, " there");
 
-	start_mem = ut_check_free();
 	ut_asserteq(1, run_command("setexpr.s fred *0 * *10", 0));
-	ut_assertok(ut_check_delta(start_mem));
 	ut_assert_nextline("invalid op");
 	ut_assert_console_end();
 
-	/*
-	 * Set 'fred' to the same length as we expect to get below, to avoid a
-	 * new allocation in 'setexpr'. That way we can check for memory leaks.
-	 */
 	ut_assertok(env_set("fred", "12345012345"));
-	start_mem = ut_check_free();
 	ut_assertok(run_command("setexpr.s fred *0 + *10", 0));
 	ut_asserteq_str("hello there", env_get("fred"));
 
-	/*
-	 * This check does not work with sandbox_flattree, apparently due to
-	 * memory allocations in env_set().
-	 *
-	 * The truetype console produces lots of memory allocations even though
-	 * the LCD display is not visible. But even without these, it does not
-	 * work.
-	 *
-	 * A better test would be for dlmalloc to record the allocs and frees
-	 * for a particular caller, but that is not supported.
-	 *
-	 * For now, drop this test.
-	 *
-	 * ut_assertok(ut_check_delta(start_mem));
-	 */
-
 	unmap_sysmem(buf);
 
 	return 0;