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

[Michael Walle]

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 at 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 at walle.cc>
> >> Signed-off-by: Simon Glass <sjg at 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
 - 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().


-- 
Michael