From 0395d75d6b867a3c109e5a71c3773f8415bf5121 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 3 Feb 2025 16:10:26 +0100 Subject: [PATCH 1/4] test: clean up setexpr_test_str() Assign variable buf in the sub-test where it is used. Signed-off-by: Heinrich Schuchardt --- test/cmd/setexpr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index 7f318a42ead..cc2003faab5 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -309,10 +309,10 @@ static int setexpr_test_str(struct unit_test_state *uts) */ 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)); + strcpy(buf, "hello"); ut_assertok(env_set("fred", "12345")); start_mem = ut_check_free(); ut_assertok(run_command("setexpr.s fred *0", 0)); From b0716426367d992db8fb9a2235c0eb9c9fafe324 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 3 Feb 2025 16:10:27 +0100 Subject: [PATCH 2/4] test: remove available memory check in setexpr_test_str() env_set() frees the previous value after allocating the new value. As the free() may merge memory chunks the available memory is not expected to stay constant. Signed-off-by: Heinrich Schuchardt --- test/cmd/setexpr.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index cc2003faab5..519912a8e22 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -297,31 +297,18 @@ 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(); ut_asserteq(1, run_command("setexpr.s fred 0", 0)); - ut_assertok(ut_check_delta(start_mem)); 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); From d4265cdcd57c1975d4e756ab1778c837acc9776c Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 3 Feb 2025 16:10:28 +0100 Subject: [PATCH 3/4] test: remove available memory check in setexpr_test_str_oper() env_set() frees the previous value after allocating the new value. As the free() may merge memory chunks the available memory is not expected to stay constant. Signed-off-by: Heinrich Schuchardt --- test/cmd/setexpr.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index 519912a8e22..650fbc8ebee 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -319,7 +319,6 @@ 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; buf = map_sysmem(0, BUF_SIZE); @@ -327,37 +326,14 @@ static int setexpr_test_str_oper(struct unit_test_state *uts) 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; From af7eca24a78b1419656d618463f1eabe297fdb2f Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 3 Feb 2025 16:10:29 +0100 Subject: [PATCH 4/4] cmd/setexpr: support concatenation of direct strings 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 + ` 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'. Signed-off-by: Heinrich Schuchardt --- cmd/setexpr.c | 54 ++++++++++++++++++++++++++++++++-------------- test/cmd/setexpr.c | 7 +++++- 2 files changed, 44 insertions(+), 17 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 650fbc8ebee..5e9b577fe36 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -303,7 +303,8 @@ static int setexpr_test_str(struct unit_test_state *uts) memset(buf, '\xff', BUF_SIZE); ut_assertok(env_set("fred", "x")); - ut_asserteq(1, run_command("setexpr.s fred 0", 0)); + 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")); @@ -321,6 +322,10 @@ static int setexpr_test_str_oper(struct unit_test_state *uts) { 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");