[U-Boot] [PATCH 1/2] Add run_command_list() to run a list of commands
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org --- common/cmd_pxe.c | 20 ++------------ common/cmd_source.c | 49 +---------------------------------- common/main.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 1 + 4 files changed, 77 insertions(+), 65 deletions(-)
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index 8a68fa1..4b18d0b 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); + printf("running: %s\n", localcmd);
- free(dupcmd); - - return ret; + return run_command_list(localcmd, strlen(localcmd), 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 db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +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 && *cmd) + rcode = (builtin_run_command(cmd, 0) >= 0); + + return rcode; +} +#endif + +/* + * Run a list of commands separated by ; + * + * Note that if 'len' is not -1, then the command may not be \0 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 command 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 need_buff = 1; + char *buff = (char *)cmd; + int rcode = 0; + + if (len == -1) { + len = strlen(cmd); + need_buff = strchr(cmd, '\n') != NULL; + } + if (need_buff) { + buff = malloc(len + 1); + if (!buff) + return 1; + memcpy(buff, cmd, len + 1); + } +#ifdef CONFIG_SYS_HUSH_PARSER + rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); +#else + rcode = builtin_run_command_list(buff, flag); +#endif + if (need_buff) + free(buff); + + return rcode; +} + /****************************************************************************/
#if defined(CONFIG_CMD_RUN) diff --git a/include/common.h b/include/common.h index 0bda049..cc5d61c 100644 --- a/include/common.h +++ b/include/common.h @@ -261,6 +261,7 @@ 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); +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);
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org --- common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/main.c b/common/main.c index 87f2855..48d6b12 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, strlen(p), 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, strlen(s), 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, strlen(s), 0); } #endif /* CONFIG_MENUKEY */ #endif /* CONFIG_BOOTDELAY */
Dear Simon Glass,
In message 1329286030-32560-2-git-send-email-sjg@chromium.org you wrote:
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Please squash this into the patch that introduces run_command_list() [assuming this is the patch which breaks behaviour?]
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Tue, Feb 14, 2012 at 11:31 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1329286030-32560-2-git-send-email-sjg@chromium.org you wrote:
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Please squash this into the patch that introduces run_command_list() [assuming this is the patch which breaks behaviour?]
The first patch is just a clean-up and should not change any behaviour. This second patch changes the behaviour of the named env variables.
I will combine them.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de The word "fit", as I understand it, means "appropriate to a purpose", and I would say the body of the Dean is supremely appropriate to the purpose of sitting around all day and eating big heavy meals. - Terry Pratchett, _Moving Pictures_
Dear Simon Glass,
In message CAPnjgZ0XueZukodaNSdQSYvuevyiH5Z-=3-2pkcC7OekddG-JA@mail.gmail.com you wrote:
In message 1329286030-32560-2-git-send-email-sjg@chromium.org you wrote:
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Please squash this into the patch that introduces run_command_list() [assuming this is the patch which breaks behaviour?]
The first patch is just a clean-up and should not change any behaviour. This second patch changes the behaviour of the named env variables.
So which commit introduced the breakage, then?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Wed, Feb 15, 2012 at 2:49 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ0XueZukodaNSdQSYvuevyiH5Z-=3-2pkcC7OekddG-JA@mail.gmail.com you wrote:
In message 1329286030-32560-2-git-send-email-sjg@chromium.org you wrote:
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
Signed-off-by: Simon Glass sjg@chromium.org
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Please squash this into the patch that introduces run_command_list() [assuming this is the patch which breaks behaviour?]
The first patch is just a clean-up and should not change any behaviour. This second patch changes the behaviour of the named env variables.
So which commit introduced the breakage, then?
Which breakage are you referring to? The intent of these two patches is to add a new feature (proposed by Michael Walle michael@walle.cc who also provided a patch) on top of the command refactor series.
Sorry, there is some confusion here but I'm not sure what it is.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de A stone was placed at a ford in a river with the inscription: "When this stone is covered it is dangerous to ford here."
Dear Simon Glass,
In message CAPnjgZ1BMMrN-+bHFr9GcH-5wF4WSz_7Qa53C_cHPLEbL2ycqA@mail.gmail.com you wrote: ...
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
...
The first patch is just a clean-up and should not change any behaviour. This second patch changes the behaviour of the named env variables.
So which commit introduced the breakage, then?
Which breakage are you referring to? The intent of these two patches is to add a new feature (proposed by Michael Walle michael@walle.cc who also provided a patch) on top of the command refactor series.
Sorry, there is some confusion here but I'm not sure what it is.
Guess I'm confused.
I don't understand in which way the variables listed here (preboot, bootcmd and menucmd) are special - actually all variables can hold commands, that then can be executed using the "run" command.
If newline is a valid command separator (and I think it is), this should work for _all_ variable.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Tue, Mar 6, 2012 at 7:26 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message CAPnjgZ1BMMrN-+bHFr9GcH-5wF4WSz_7Qa53C_cHPLEbL2ycqA@mail.gmail.com you wrote: ...
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
...
The first patch is just a clean-up and should not change any behaviour. This second patch changes the behaviour of the named env variables.
So which commit introduced the breakage, then?
Which breakage are you referring to? The intent of these two patches is to add a new feature (proposed by Michael Walle michael@walle.cc who also provided a patch) on top of the command refactor series.
Sorry, there is some confusion here but I'm not sure what it is.
Guess I'm confused.
I don't understand in which way the variables listed here (preboot, bootcmd and menucmd) are special - actually all variables can hold commands, that then can be executed using the "run" command.
If newline is a valid command separator (and I think it is), this should work for _all_ variable.
OK I see. It is my commit message that is confusing, sorry. I didn't mean that only those env variables can hold a command, just that this patch affects the code which makes those environment variables work.
I will update the commit message and resend this patch.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Backed up the system lately?
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
common/main.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/main.c b/common/main.c index 87f2855..48d6b12 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, strlen(p), 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, strlen(s), 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, strlen(s), 0); } #endif /* CONFIG_MENUKEY */ #endif /* CONFIG_BOOTDELAY */
Dear Simon Glass,
In message 1329286030-32560-2-git-send-email-sjg@chromium.org you wrote:
The environment variables preboot, bootcmd and menucmd can hold a command to execute. This change permits these variables to have newlines so that they work the same as the 'source' command.
This comment needs to be fixed. Not only these variables are effected, and from the patch it is not visible what it has to do with newlines.
Best regards,
Wolfgang Denk
Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org
common/cmd_pxe.c | 20 ++------------ common/cmd_source.c | 49 +---------------------------------- common/main.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 1 + 4 files changed, 77 insertions(+), 65 deletions(-)
[..snip..]
diff --git a/common/main.c b/common/main.c index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +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 && *cmd)
rcode = (builtin_run_command(cmd, 0) >= 0);- return rcode;
+} +#endif
+/*
- Run a list of commands separated by ;
mh, where is the command string seperated by ';' if no hush is used?
- Note that if 'len' is not -1, then the command may not be \0
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 command 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 need_buff = 1;
- char *buff = (char *)cmd;
mhhh, builtin_run_command_list will modify buff. what do you think about always copying the buffer if no hush parser is used?
- int rcode = 0;
- if (len == -1) {
len = strlen(cmd);need_buff = strchr(cmd, '\n') != NULL;- }
- if (need_buff) {
buff = malloc(len + 1);if (!buff)return 1;memcpy(buff, cmd, len + 1);- }
+#ifdef CONFIG_SYS_HUSH_PARSER
- rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
+#else
- rcode = builtin_run_command_list(buff, flag);
+#endif
- if (need_buff)
free(buff);- return rcode;
+}
/************************************************************************* ***/
#if defined(CONFIG_CMD_RUN) diff --git a/include/common.h b/include/common.h index 0bda049..cc5d61c 100644 --- a/include/common.h +++ b/include/common.h @@ -261,6 +261,7 @@ 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); +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);
Hi Michael,
On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle michael@walle.cc wrote:
Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org
common/cmd_pxe.c | 20 ++------------ common/cmd_source.c | 49 +---------------------------------- common/main.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 1 + 4 files changed, 77 insertions(+), 65 deletions(-)
[..snip..]
diff --git a/common/main.c b/common/main.c index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +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 && *cmd)
- rcode = (builtin_run_command(cmd, 0) >= 0);
- return rcode;
+} +#endif
+/*
- Run a list of commands separated by ;
mh, where is the command string seperated by ';' if no hush is used?
In that case it is handled by builtin_run_command() already.
- Note that if 'len' is not -1, then the command may not be \0
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 command 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 need_buff = 1;
- char *buff = (char *)cmd;
mhhh, builtin_run_command_list will modify buff. what do you think about always copying the buffer if no hush parser is used?
Yes we could do - how would that help?
- int rcode = 0;
- if (len == -1) {
- len = strlen(cmd);
- need_buff = strchr(cmd, '\n') != NULL;
- }
- if (need_buff) {
- buff = malloc(len + 1);
- if (!buff)
- return 1;
- memcpy(buff, cmd, len + 1);
- }
+#ifdef CONFIG_SYS_HUSH_PARSER
- rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
+#else
- rcode = builtin_run_command_list(buff, flag);
+#endif
- if (need_buff)
- free(buff);
- return rcode;
+}
/************************************************************************* ***/
#if defined(CONFIG_CMD_RUN) diff --git a/include/common.h b/include/common.h index 0bda049..cc5d61c 100644 --- a/include/common.h +++ b/include/common.h @@ -261,6 +261,7 @@ 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); +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);
-- Michael
Regards, Simon
Am Mittwoch 15 Februar 2012, 23:23:19 schrieb Simon Glass:
Hi Michael,
On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle michael@walle.cc wrote:
Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org
common/cmd_pxe.c | 20 ++------------ common/cmd_source.c | 49 +---------------------------------- common/main.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 1 + 4 files changed, 77 insertions(+), 65 deletions(-)
[..snip..]
diff --git a/common/main.c b/common/main.c index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +static int builtin_run_command_list(char *cmd, int flag) +{
char *line, *next;int rcode = 0;/** Break into individual lines, and execute each line; terminateon + * 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 && *cmd)rcode = (builtin_run_command(cmd, 0) >= 0);return rcode;+} +#endif
+/*
- Run a list of commands separated by ;
mh, where is the command string seperated by ';' if no hush is used?
In that case it is handled by builtin_run_command() already.
- Note that if 'len' is not -1, then the command may not be \0
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 command 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 need_buff = 1;char *buff = (char *)cmd;mhhh, builtin_run_command_list will modify buff. what do you think about always copying the buffer if no hush parser is used?
Yes we could do - how would that help?
ah sorry, i hadn't understood your code. too late here ;)
copying the buffer everytime (in case no hush is available): - removes the cast, which seems very odd on the first look - makes run_command_list() more robust against furture changes in builtin_run_command_list(), eg. if it modifies the string not only if '\n' is found in the cmd.
at least you should add a comment to keep the check for need_buff in sync with builtin_run_command_list().
Hi Michael,
On Wed, Feb 15, 2012 at 3:55 PM, Michael Walle michael@walle.cc wrote:
Am Mittwoch 15 Februar 2012, 23:23:19 schrieb Simon Glass:
Hi Michael,
On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle michael@walle.cc wrote:
Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Simon Glass sjg@chromium.org
common/cmd_pxe.c | 20 ++------------ common/cmd_source.c | 49 +---------------------------------- common/main.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h | 1 + 4 files changed, 77 insertions(+), 65 deletions(-)
[..snip..]
diff --git a/common/main.c b/common/main.c index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag) #endif }
+#ifndef CONFIG_SYS_HUSH_PARSER +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 && *cmd)
- rcode = (builtin_run_command(cmd, 0) >= 0);
- return rcode;
+} +#endif
+/*
- Run a list of commands separated by ;
mh, where is the command string seperated by ';' if no hush is used?
In that case it is handled by builtin_run_command() already.
- Note that if 'len' is not -1, then the command may not be \0
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 command 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 need_buff = 1;
- char *buff = (char *)cmd;
mhhh, builtin_run_command_list will modify buff. what do you think about always copying the buffer if no hush parser is used?
Yes we could do - how would that help?
ah sorry, i hadn't understood your code. too late here ;)
copying the buffer everytime (in case no hush is available): - removes the cast, which seems very odd on the first look
I can add a comment.
- makes run_command_list() more robust against furture changes in builtin_run_command_list(), eg. if it modifies the string not only if '\n' is found in the cmd.
Yes that's true, and I wasn't able to pass 'cmd' which would solve the problem at compile time.
at least you should add a comment to keep the check for need_buff in sync with builtin_run_command_list().
Yes, will do that. I am a little reluctant to always malloc() since it introduces a malloc() even in the trivial case where we want to execute the contents of a simple env variable with no newline.
-- Michael
Regards, Simon
Dear Simon Glass,
In message 1329286030-32560-1-git-send-email-sjg@chromium.org you wrote:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
Separated by semicolon? What about newline here?
- printf("running: %s\n", localcmd);
Should this be a debug() ?
- 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 && *cmd)
rcode = (builtin_run_command(cmd, 0) >= 0);
This looks wrong to me. There you are re-executing the original command. Shoudl this not be
if (rcode == 0 && *line) rcode = (run_command(line, 0) >= 0);
??
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Wed, Mar 7, 2012 at 3:34 AM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1329286030-32560-1-git-send-email-sjg@chromium.org you wrote:
This new function runs a list of commands separated by semicolon. We move this out of cmd_source so that it can be used by other code. The PXE also uses the new function.
This took a bit longer to get back to than I expected.
Separated by semicolon? What about newline here?
- printf("running: %s\n", localcmd);
Should this be a debug() ?
I'm not sure - the old code has printf() but I don't know why. I will change it.
- 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 && *cmd)
- rcode = (builtin_run_command(cmd, 0) >= 0);
This looks wrong to me. There you are re-executing the original command. Shoudl this not be
if (rcode == 0 && *line) rcode = (run_command(line, 0) >= 0);
??
Yes you are right of course. I created a few tests and found a few other nits as well, so will send a new series that tidies all this up.
Regards, Simon
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Computers are not intelligent. They only think they are.
participants (3)
-
Michael Walle -
Simon Glass -
Wolfgang Denk