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

[Simon Glass]

Hi Michael,

On Wed, Feb 15, 2012 at 3:55 PM, Michael Walle <michael at 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 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

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