| Message ID | 20201101211544.3579850-11-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 2c02152a8e2d640fe1d93177fbf523d25cd3e8f9 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | setexpr: Correct various bugs and add tests plus string support | expand |
What is the purpose of + operator on strings?
Can't we use setenv "${a}${b}" ?
Hi Marek, On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote: > > What is the purpose of + operator on strings? > Can't we use setenv "${a}${b}" ? Yes, that does the same thing, although it is a bit clumsy. setenv a *10 setenv b *100 setenv c "${a}${b}" instead of setexpr c *10 + *100 Regards, Simon
On Tue, 3 Nov 2020 08:12:17 -0700 Simon Glass <sjg@chromium.org> wrote: > Hi Marek, > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote: > > > > What is the purpose of + operator on strings? > > Can't we use setenv "${a}${b}" ? > > Yes, that does the same thing, although it is a bit clumsy. > > setenv a *10 > setenv b *100 > setenv c "${a}${b}" > > instead of > > setexpr c *10 + *100 Hi Simon, I don't know. It provides the same functionality that exists, but only adds code. Is someone really going to use this? Marek PS: What I think would be more useful is to add substringing functionality into hush, so e.g. ${a:3:5}, and pattern substitions: ${parameter/pattern/string} ...
Dear Simon, In message <CAPnjgZ3SXdKaKmYCw=Q0w-JGhghAPVhwHdtG5q2dNEMNiY60Xg@mail.gmail.com> you wrote: > > > What is the purpose of + operator on strings? > > Can't we use setenv "${a}${b}" ? > > Yes, that does the same thing, although it is a bit clumsy. > > setenv a *10 > setenv b *100 > setenv c "${a}${b}" > > instead of > > setexpr c *10 + *100 I don't get it. The equivalent to "${a}${b}" would be setexpr c "*10*100" which is even simpler? Best regards, Wolfgang Denk
Dear Marek, In message <20201103173011.08e22c11@nic.cz> you wrote: > > PS: What I think would be more useful is to add substringing > functionality into hush, so e.g. ${a:3:5}, and pattern substitions: > ${parameter/pattern/string} ... No, NAK, don't. At least not to the current imple,entation of hush. First, upgrade to the recent version from BusyBox, and then we might discuss extensions. Actually we should eventually discuss these in BusyBox to have such stuff in "hush mainline" ... Best regards, Wolfgang Denk
Hi Marek, On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote: > > On Tue, 3 Nov 2020 08:12:17 -0700 > Simon Glass <sjg@chromium.org> wrote: > > > Hi Marek, > > > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > What is the purpose of + operator on strings? > > > Can't we use setenv "${a}${b}" ? > > > > Yes, that does the same thing, although it is a bit clumsy. > > > > setenv a *10 > > setenv b *100 > > setenv c "${a}${b}" > > > > instead of > > > > setexpr c *10 + *100 > > Hi Simon, > > I don't know. It provides the same functionality that exists, but only > adds code. > Is someone really going to use this? I don't know. Perhaps we can wait and see if anyone cares? > > Marek > > PS: What I think would be more useful is to add substringing > functionality into hush, so e.g. ${a:3:5}, and pattern substitions: > ${parameter/pattern/string} ... Yes we need to upgrade hush. Regards, Simon
Hi Wolfgang, On Thu, 5 Nov 2020 at 09:47, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon, > > In message <CAPnjgZ3SXdKaKmYCw=Q0w-JGhghAPVhwHdtG5q2dNEMNiY60Xg@mail.gmail.com> you wrote: > > > > > What is the purpose of + operator on strings? > > > Can't we use setenv "${a}${b}" ? > > > > Yes, that does the same thing, although it is a bit clumsy. > > > > setenv a *10 > > setenv b *100 > > setenv c "${a}${b}" > > > > instead of > > > > setexpr c *10 + *100 > > I don't get it. The equivalent to "${a}${b}" would be > > setexpr c "*10*100" > > which is even simpler? I don't see how that works. The *10 thing in my example reads a string out of address 10. Regards, Simon
On Thu, 5 Nov 2020 10:27:28 -0700 Simon Glass <sjg@chromium.org> wrote: > I don't see how that works. The *10 thing in my example reads a string > out of address 10. /o\ ahaaaa, OK, that makes sense. So setexpr can dereference strings. I didn't know about that, I thouth the resulting string would be "*10*100". In this case Acked-by: Marek Behún <marek.behun@nic.cz>
Dear Simon. In message <CAPnjgZ3DmeaincEcYkjeTuRijqtMZhJiDnyx_o4eSRE4gJaVFw@mail.gmail.com> you wrote: > > > > setexpr c *10 + *100 > > > > I don't get it. The equivalent to "${a}${b}" would be > > > > setexpr c "*10*100" > > > > which is even simpler? > > I don't see how that works. The *10 thing in my example reads a string > out of address 10. Ah, got it. This requires your "[PATCH 10/10] setexpr: Add support for strings" first... But then... should there not be some '.s' size specification somewhere? Best regards, Wolfgang Denk
Hi Wolfgang, On Thu, 5 Nov 2020 at 12:10, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon. > > In message <CAPnjgZ3DmeaincEcYkjeTuRijqtMZhJiDnyx_o4eSRE4gJaVFw@mail.gmail.com> you wrote: > > > > > > setexpr c *10 + *100 > > > > > > I don't get it. The equivalent to "${a}${b}" would be > > > > > > setexpr c "*10*100" > > > > > > which is even simpler? > > > > I don't see how that works. The *10 thing in my example reads a string > > out of address 10. > > Ah, got it. This requires your "[PATCH 10/10] setexpr: Add support > for strings" first... > > But then... should there not be some '.s' size specification > somewhere? Ooops yes: setexpr.s c *10 + *100 Regards, Simon
On Thu, Nov 05, 2020 at 10:27:25AM -0700, Simon Glass wrote: > Hi Marek, > > On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote: > > > > On Tue, 3 Nov 2020 08:12:17 -0700 > > Simon Glass <sjg@chromium.org> wrote: > > > > > Hi Marek, > > > > > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > What is the purpose of + operator on strings? > > > > Can't we use setenv "${a}${b}" ? > > > > > > Yes, that does the same thing, although it is a bit clumsy. > > > > > > setenv a *10 > > > setenv b *100 > > > setenv c "${a}${b}" > > > > > > instead of > > > > > > setexpr c *10 + *100 > > > > Hi Simon, > > > > I don't know. It provides the same functionality that exists, but only > > adds code. > > Is someone really going to use this? > > I don't know. Perhaps we can wait and see if anyone cares? Sorry, does that mean this support is just speculative? Or did I misread this part of the thread? Thanks.
Hi Tom, On Fri, 6 Nov 2020 at 13:58, Tom Rini <trini@konsulko.com> wrote: > > On Thu, Nov 05, 2020 at 10:27:25AM -0700, Simon Glass wrote: > > Hi Marek, > > > > On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Tue, 3 Nov 2020 08:12:17 -0700 > > > Simon Glass <sjg@chromium.org> wrote: > > > > > > > Hi Marek, > > > > > > > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > What is the purpose of + operator on strings? > > > > > Can't we use setenv "${a}${b}" ? > > > > > > > > Yes, that does the same thing, although it is a bit clumsy. > > > > > > > > setenv a *10 > > > > setenv b *100 > > > > setenv c "${a}${b}" > > > > > > > > instead of > > > > > > > > setexpr c *10 + *100 > > > > > > Hi Simon, > > > > > > I don't know. It provides the same functionality that exists, but only > > > adds code. > > > Is someone really going to use this? > > > > I don't know. Perhaps we can wait and see if anyone cares? > > Sorry, does that mean this support is just speculative? Or did I > misread this part of the thread? Thanks. I think it was a misunderstanding of what it does. Marek acked the patch and Wolfgang seems happy. Regards, SImon
On Sun, Nov 01, 2020 at 02:15:44PM -0700, Simon Glass wrote: > Add support for dealing with string operands, including reading a string > from memory into an environment variable and concatenating two strings. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Acked-by: Marek Behún <marek.behun@nic.cz> Applied to u-boot/next, thanks!
diff --git a/cmd/setexpr.c b/cmd/setexpr.c index 8a3654505da..e828be39700 100644 --- a/cmd/setexpr.c +++ b/cmd/setexpr.c @@ -13,15 +13,21 @@ #include <command.h> #include <env.h> #include <log.h> +#include <malloc.h> #include <mapmem.h> +#include <linux/sizes.h> /** * struct expr_arg: Holds an argument to an expression * * @ival: Integer value (if width is not CMD_DATA_SIZE_STR) + * @sval: String value (if width is CMD_DATA_SIZE_STR) */ struct expr_arg { - ulong ival; + union { + ulong ival; + char *sval; + }; }; static int get_arg(char *s, int w, struct expr_arg *argp) @@ -36,6 +42,8 @@ static int get_arg(char *s, int w, struct expr_arg *argp) ulong *p; ulong addr; ulong val; + int len; + char *str; addr = simple_strtoul(&s[1], NULL, 16); switch (w) { @@ -51,6 +59,21 @@ static int get_arg(char *s, int w, struct expr_arg *argp) unmap_sysmem(p); arg.ival = val; 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'; + unmap_sysmem(p); + arg.sval = str; + break; case 4: p = map_sysmem(addr, sizeof(u32)); val = *(u32 *)p; @@ -65,6 +88,8 @@ static int get_arg(char *s, int w, struct expr_arg *argp) break; } } else { + if (w == CMD_DATA_SIZE_STR) + return -EINVAL; arg.ival = simple_strtoul(s, NULL, 16); } *argp = arg; @@ -341,6 +366,7 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, { struct expr_arg aval, bval; ulong value; + int ret = 0; int w; /* @@ -361,8 +387,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_FAILURE; /* plain assignment: "setexpr name value" */ - if (argc == 3) - return env_set_hex(argv[1], aval.ival); + if (argc == 3) { + if (w == CMD_DATA_SIZE_STR) { + ret = env_set(argv[1], aval.sval); + free(aval.sval); + } else { + ret = env_set_hex(argv[1], aval.ival); + } + + return ret; + } /* 5 or 6 args (6 args only with [g]sub) */ #ifdef CONFIG_REGEX @@ -384,10 +418,38 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, if (strlen(argv[3]) != 1) return CMD_RET_USAGE; - if (get_arg(argv[4], w, &bval)) + if (get_arg(argv[4], w, &bval)) { + if (w == CMD_DATA_SIZE_STR) + free(aval.sval); return CMD_RET_FAILURE; + } + + if (w == CMD_DATA_SIZE_STR) { + int len; + char *str; - if (w != CMD_DATA_SIZE_STR) { + switch (argv[3][0]) { + case '+': + len = strlen(aval.sval) + strlen(bval.sval) + 1; + str = malloc(len); + if (!str) { + printf("Out of memory\n"); + ret = CMD_RET_FAILURE; + } else { + /* These were copied out and checked earlier */ + strcpy(str, aval.sval); + strcat(str, bval.sval); + ret = env_set(argv[1], str); + if (ret) + printf("Could not set var\n"); + free(str); + } + break; + default: + printf("invalid op\n"); + ret = 1; + } + } else { ulong a = aval.ival; ulong b = bval.ival; @@ -424,15 +486,21 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, env_set_hex(argv[1], value); } - return 0; + if (w == CMD_DATA_SIZE_STR) { + free(aval.sval); + free(bval.sval); + } + + return ret; } U_BOOT_CMD( setexpr, 6, 0, do_setexpr, "set environment variable as the result of eval expression", - "[.b, .w, .l] name [*]value1 <op> [*]value2\n" + "[.b, .w, .l, .s] name [*]value1 <op> [*]value2\n" " - set environment variable 'name' to the result of the evaluated\n" " expression specified by <op>. <op> can be &, |, ^, +, -, *, /, %\n" + " (for strings only + is supported)\n" " size argument is only meaningful if value1 and/or value2 are\n" " memory addresses (*)\n" "setexpr[.b, .w, .l] name [*]value\n" diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index 2a897efd9bd..fd6d869c0ed 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -287,6 +287,92 @@ static int setexpr_test_backref(struct unit_test_state *uts) } SETEXPR_TEST(setexpr_test_backref, UT_TESTF_CONSOLE_REC); +/* 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)); + + start_mem = ut_check_free(); + ut_assertok(env_set("fred", "12345")); + ut_assertok(run_command("setexpr.s fred *0", 0)); + ut_asserteq_str("hello", env_get("fred")); + ut_assertok(ut_check_delta(start_mem)); + + unmap_sysmem(buf); + + return 0; +} +SETEXPR_TEST(setexpr_test_str, UT_TESTF_CONSOLE_REC); + + +/* 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); + memset(buf, '\xff', BUF_SIZE); + strcpy(buf, "hello"); + strcpy(buf + 0x10, " there"); + + ut_assertok(console_record_reset_enable()); + 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")); + ut_assertok(ut_check_delta(start_mem)); + + unmap_sysmem(buf); + + return 0; +} +SETEXPR_TEST(setexpr_test_str_oper, UT_TESTF_CONSOLE_REC); + +/* Test 'setexpr' command with a string that is too long */ +static int setexpr_test_str_long(struct unit_test_state *uts) +{ + const int size = 128 << 10; /* setexpr strings are a max of 64KB */ + char *buf, *val; + + buf = map_sysmem(0, size); + memset(buf, 'a', size); + + /* String should be truncated to 64KB */ + ut_assertok(run_command("setexpr.s fred *0", 0)); + val = env_get("fred"); + ut_asserteq(64 << 10, strlen(val)); + + unmap_sysmem(buf); + + return 0; +} +SETEXPR_TEST(setexpr_test_str_long, UT_TESTF_CONSOLE_REC); + int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct unit_test *tests = ll_entry_start(struct unit_test,
Add support for dealing with string operands, including reading a string from memory into an environment variable and concatenating two strings. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/setexpr.c | 82 +++++++++++++++++++++++++++++++++++++++---- test/cmd/setexpr.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 7 deletions(-)