[U-Boot] [PATCH v4 1/3] Add run_command_list() to run a list of commands

This new function runs a list of commands separated by semicolon or newline. We move this out of cmd_source so that it can be used by other code. The PXE code also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v3: - Added a few more comments on the need for malloc() - Change PXE's printf() to a debug() - Fix bug in built-in run_command() - Fix commit message to mention ; and \n - Move run_command_list() comment to header file
Changes in v4: - Remove length param from run_command_list() call in PXE
common/cmd_pxe.c | 20 ++---------- common/cmd_source.c | 49 +---------------------------- common/main.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 13 ++++++++ 4 files changed, 102 insertions(+), 65 deletions(-)
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index b3c1f67..55d1c05 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -515,33 +515,19 @@ static void label_print(void *data) */ static int label_localboot(struct pxe_label *label) { - char *localcmd, *dupcmd; - int ret; + char *localcmd;
localcmd = from_env("localcmd");
if (!localcmd) return -ENOENT;
- /* - * dup the command to avoid any issues with the version of it existing - * in the environment changing during the execution of the command. - */ - dupcmd = strdup(localcmd); - - if (!dupcmd) - return -ENOMEM; - if (label->append) setenv("bootargs", label->append);
- printf("running: %s\n", dupcmd); - - ret = run_command(dupcmd, 0); + debug("running: %s\n", localcmd);
- free(dupcmd); - - return ret; + return run_command_list(localcmd, -1, 0); }
/* diff --git a/common/cmd_source.c b/common/cmd_source.c index 32fff5c..c4cde98 100644 --- a/common/cmd_source.c +++ b/common/cmd_source.c @@ -39,9 +39,6 @@ #if defined(CONFIG_8xx) #include <mpc8xx.h> #endif -#ifdef CONFIG_SYS_HUSH_PARSER -#include <hush.h> -#endif
int source (ulong addr, const char *fit_uname) @@ -49,8 +46,6 @@ source (ulong addr, const char *fit_uname) ulong len; image_header_t *hdr; ulong *data; - char *cmd; - int rcode = 0; int verify; #if defined(CONFIG_FIT) const void* fit_hdr; @@ -151,49 +146,7 @@ source (ulong addr, const char *fit_uname) }
debug ("** Script length: %ld\n", len); - - if ((cmd = malloc (len + 1)) == NULL) { - return 1; - } - - /* make sure cmd is null terminated */ - memmove (cmd, (char *)data, len); - *(cmd + len) = 0; - -#ifdef CONFIG_SYS_HUSH_PARSER /*?? */ - rcode = parse_string_outer (cmd, FLAG_PARSE_SEMICOLON); -#else - { - char *line = cmd; - char *next = cmd; - - /* - * break into individual lines, - * and execute each line; - * terminate on error. - */ - while (*next) { - if (*next == '\n') { - *next = '\0'; - /* run only non-empty commands */ - if (*line) { - debug ("** exec: "%s"\n", - line); - if (run_command(line, 0) < 0) { - rcode = 1; - break; - } - } - line = next + 1; - } - ++next; - } - if (rcode == 0 && *line) - rcode = (run_command(line, 0) >= 0); - } -#endif - free (cmd); - return rcode; + return run_command_list((char *)data, len, 0); }
/**************************************************/ diff --git a/common/main.c b/common/main.c index a933357..d96ba0a 100644 --- a/common/main.c +++ b/common/main.c @@ -30,6 +30,7 @@ #include <common.h> #include <watchdog.h> #include <command.h> +#include <malloc.h> #include <version.h> #ifdef CONFIG_MODEM_SUPPORT #include <malloc.h> /* for free() prototype */ @@ -1373,6 +1374,90 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +/** + * Execute a list of command separated by ; or \n using the built-in parser. + * + * This function cannot take a const char * for the command, since if it + * finds newlines in the string, it replaces them with \0. + * + * @param cmd String containing list of commands + * @param flag Execution flags (CMD_FLAG_...) + * @return 0 on success, or != 0 on error. + */ +static int builtin_run_command_list(char *cmd, int flag) +{ + char *line, *next; + int rcode = 0; + + /* + * Break into individual lines, and execute each line; terminate on + * error. + */ + line = next = cmd; + while (*next) { + if (*next == '\n') { + *next = '\0'; + /* run only non-empty commands */ + if (*line) { + debug("** exec: "%s"\n", line); + if (builtin_run_command(line, 0) < 0) { + rcode = 1; + break; + } + } + line = next + 1; + } + ++next; + } + if (rcode == 0 && *line) + rcode = (builtin_run_command(line, 0) >= 0); + + return rcode; +} +#endif + +int run_command_list(const char *cmd, int len, int flag) +{ + int need_buff = 1; + char *buff = (char *)cmd; /* cast away const */ + int rcode = 0; + + if (len == -1) { + len = strlen(cmd); +#ifdef CONFIG_SYS_HUSH_PARSER + /* hush will never change our string */ + need_buff = 0; +#else + /* the built-in parser will change our string if it sees \n */ + need_buff = strchr(cmd, '\n') != NULL; +#endif + } + if (need_buff) { + buff = malloc(len + 1); + if (!buff) + return 1; + memcpy(buff, cmd, len); + buff[len] = '\0'; + } +#ifdef CONFIG_SYS_HUSH_PARSER + rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); +#else + /* + * This function will overwrite any \n it sees with a \0, which + * is why it can't work with a const char *. Here we are making + * using of internal knowledge of this function, to avoid always + * doing a malloc() which is actually required only in a case that + * is pretty rare. + */ + rcode = builtin_run_command_list(buff, flag); + if (need_buff) + free(buff); +#endif + + return rcode; +} + /****************************************************************************/
#if defined(CONFIG_CMD_RUN) diff --git a/include/common.h b/include/common.h index 8564a65..48c6abe 100644 --- a/include/common.h +++ b/include/common.h @@ -286,6 +286,19 @@ int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen); /* common/main.c */ void main_loop (void); int run_command(const char *cmd, int flag); + +/** + * Run a list of commands separated by ; or even \0 + * + * Note that if 'len' is not -1, then the command does not need to be nul + * terminated, Memory will be allocated for the command in that case. + * + * @param cmd List of commands to run, each separated bu semicolon + * @param len Length of commands excluding terminator if known (-1 if not) + * @param flag Execution flags (CMD_FLAG_...) + * @return 0 on success, or != 0 on error. + */ +int run_command_list(const char *cmd, int len, int flag); int readline (const char *const prompt); int readline_into_buffer(const char *const prompt, char *buffer, int timeout);

Any environment variable can hold commands to be executed by the 'run' command. The environment variables preboot, bootcmd and menucmd have special code for triggering execution in certain circumstances.
We adjust these calls to use run_command_list() instead of run_command(). This change permits these variables to have embedded newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Update commit message to be less confusing
Changes in v3: - Remove unneeded len parameter in calls to run_command_list
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/main.c b/common/main.c index d96ba0a..9405922 100644 --- a/common/main.c +++ b/common/main.c @@ -334,7 +334,7 @@ void main_loop (void) int prev = disable_ctrlc(1); /* disable Control C checking */ # endif
- run_command(p, 0); + run_command_list(p, -1, 0);
# ifdef CONFIG_AUTOBOOT_KEYED disable_ctrlc(prev); /* restore Control C checking */ @@ -382,7 +382,7 @@ void main_loop (void) int prev = disable_ctrlc(1); /* disable Control C checking */ # endif
- run_command(s, 0); + run_command_list(s, -1, 0);
# ifdef CONFIG_AUTOBOOT_KEYED disable_ctrlc(prev); /* restore Control C checking */ @@ -393,7 +393,7 @@ void main_loop (void) if (menukey == CONFIG_MENUKEY) { s = getenv("menucmd"); if (s) - run_command(s, 0); + run_command_list(s, -1, 0); } #endif /* CONFIG_MENUKEY */ #endif /* CONFIG_BOOTDELAY */

Since run_command() and run_command_list() are important and a little confusing, add some basic tests to check that the behaviour is correct.
Note: I am not sure that this should be committed, nor where it should go in the source tree. Comments welcome.
To run the unit tests use the ut_cmd command available in sandbox:
make sandbox_config make ./u-boot -c ut_cmd
(To test both hush and built-in parsers, you need to manually change CONFIG_SYS_HUSH_PARSER in include/configs/sandbox.h and build/run again)
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v3: - Add some unit tests for run_command() and run_command_list()
Changes in v4: - Add some unit tests for run_command() and run_command_list() - Fix code nits in unit test and test/Makefile
Makefile | 1 + test/Makefile | 45 +++++++++++++++++++++++++++ test/command_ut.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 0 deletions(-) create mode 100644 test/Makefile create mode 100644 test/command_ut.c
diff --git a/Makefile b/Makefile index 351a8f0..d2bd97a 100644 --- a/Makefile +++ b/Makefile @@ -301,6 +301,7 @@ LIBS += common/libcommon.o LIBS += lib/libfdt/libfdt.o LIBS += api/libapi.o LIBS += post/libpost.o +LIBS += test/libtest.o
ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP34XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX),) LIBS += $(CPUDIR)/omap-common/libomap-common.o diff --git a/test/Makefile b/test/Makefile new file mode 100644 index 0000000..871c484 --- /dev/null +++ b/test/Makefile @@ -0,0 +1,45 @@ +# +# (C) Copyright 2012 The Chromium Authors +# +# See file CREDITS for list of people who contributed to this +# project. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, +# MA 02111-1307 USA +# + +include $(TOPDIR)/config.mk + +LIB = $(obj)libtest.o + +COBJS-$(CONFIG_SANDBOX) += command_ut.o + +COBJS := $(COBJS-y) +SRCS := $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) + +all: $(LIB) + +$(LIB): $(obj).depend $(OBJS) + $(call cmd_link_o_target, $(OBJS)) + +######################################################################### + +# defines $(obj).depend target +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +######################################################################### diff --git a/test/command_ut.c b/test/command_ut.c new file mode 100644 index 0000000..50da638 --- /dev/null +++ b/test/command_ut.c @@ -0,0 +1,87 @@ +/* + * Copyright (c) 2012, The Chromium Authors + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* Define this to make sure that our assert()s will activate */ +#define DEBUG + +#include <common.h> + +static const char test_cmd[] = + "setenv list 1\n setenv list ${list}2; " + "setenv list ${list}3\0" + "setenv list ${list}4"; + +static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + printf("%s: Testing commands\n", __func__); + run_command("env default -f", 0); + + /* run a single command */ + run_command("setenv single 1", 0); + assert(!strcmp("1", getenv("single"))); + + /* make sure that compound statements work */ +#ifdef CONFIG_SYS_HUSH_PARSER + run_command("if test -n ${single} ; then setenv check 1; fi", 0); + assert(!strcmp("1", getenv("check"))); + run_command("setenv check", 0); +#endif + + /* commands separated by ; */ + run_command_list("setenv list 1; setenv list ${list}1", -1, 0); + assert(!strcmp("11", getenv("list"))); + + /* commands separated by \n */ + run_command_list("setenv list 1\n setenv list ${list}1", -1, 0); + assert(!strcmp("11", getenv("list"))); + + /* command followed by \n and nothing else */ + run_command_list("setenv list 1${list}\n", -1, 0); + assert(!strcmp("111", getenv("list"))); + + /* three commands in a row */ + run_command_list("setenv list 1\n setenv list ${list}2; " + "setenv list ${list}3", -1, 0); + assert(!strcmp("123", getenv("list"))); + + /* a command string with \0 in it. Stuff after \0 should be ignored */ + run_command("setenv list", 0); + run_command_list(test_cmd, sizeof(test_cmd), 0); + assert(!strcmp("123", getenv("list"))); + + /* + * a command list where we limit execution to only the first command + * using the length parameter. + */ + run_command_list("setenv list 1\n setenv list ${list}2; " + "setenv list ${list}3", strlen("setenv list 1"), 0); + assert(!strcmp("1", getenv("list"))); + + printf("%s: Everything went swimmingly\n", __func__); + return 0; +} + +U_BOOT_CMD( + ut_cmd, 5, 1, do_ut_cmd, + "Very basic unit test of command parsers", + "" +);
participants (1)
-
Simon Glass