| Message ID | 1326523557-6818-5-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Simon, Am Samstag 14 Januar 2012, 07:45:52 schrieb Simon Glass: > diff --git a/common/cmd_source.c b/common/cmd_source.c > index 481241c..32fff5c 100644 > --- a/common/cmd_source.c > +++ b/common/cmd_source.c > @@ -179,7 +179,7 @@ source (ulong addr, const char *fit_uname) > if (*line) { > debug ("** exec: \"%s\"\n", > line); > - if (builtin_run_command(line, 0) < 0) { > + if (run_command(line, 0) < 0) { > rcode = 1; > break; > } > @@ -189,7 +189,7 @@ source (ulong addr, const char *fit_uname) > ++next; > } > if (rcode == 0 && *line) > - rcode = (builtin_run_command(line, 0) >= 0); > + rcode = (run_command(line, 0) >= 0); > } > #endif > free (cmd); Please have a look at this file. There is already a conditional on CONFIG_SYS_HUSH_PARSER, which can be removed, since it is now handled in run_command(). Do not forget to remove the hush.h include, too. Could you move this script handling into an own function into common/main.c? This way others can easily execute whole scripts without duplicating this code. I'm planning to submit support for a board which needs exactly that, so there will be at least one board which make use of this. Mike proposed source_commands() as a function name. My original patch named it run_script(). Dunno whats a better name. Feel free to reuse/incorporate my patch at http://patchwork.ozlabs.org/patch/137122/ into your patchset ;)
On Saturday 14 January 2012 01:45:52 Simon Glass wrote: > This function is only one of the parser options, so use run_command() > instead. It knows how to use hush if selected. i can't grok this either :/ -mike
Hi Mike, On Sat, Feb 4, 2012 at 7:23 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Saturday 14 January 2012 01:45:52 Simon Glass wrote: > > This function is only one of the parser options, so use run_command() > > instead. It knows how to use hush if selected. > > i can't grok this either :/ > -mike > Reworded... Regards, Simon
Hi Michael, On Sat, Jan 21, 2012 at 8:07 AM, Michael Walle <michael@walle.cc> wrote: > > Hi Simon, > > Am Samstag 14 Januar 2012, 07:45:52 schrieb Simon Glass: > > diff --git a/common/cmd_source.c b/common/cmd_source.c > > index 481241c..32fff5c 100644 > > --- a/common/cmd_source.c > > +++ b/common/cmd_source.c > > @@ -179,7 +179,7 @@ source (ulong addr, const char *fit_uname) > > if (*line) { > > debug ("** exec: \"%s\"\n", > > line); > > - if (builtin_run_command(line, 0) < > 0) > { > > + if (run_command(line, 0) < 0) { > > rcode = 1; > > break; > > } > > @@ -189,7 +189,7 @@ source (ulong addr, const char *fit_uname) > > ++next; > > } > > if (rcode == 0 && *line) > > - rcode = (builtin_run_command(line, 0) >= 0); > > + rcode = (run_command(line, 0) >= 0); > > } > > #endif > > free (cmd); > > Please have a look at this file. There is already a conditional on > CONFIG_SYS_HUSH_PARSER, which can be removed, since it is now handled in > run_command(). Do not forget to remove the hush.h include, too. > OK > > Could you move this script handling into an own function into > common/main.c? > This way others can easily execute whole scripts without duplicating this > code. I'm planning to submit support for a board which needs exactly that, > so > there will be at least one board which make use of this. > > Mike proposed source_commands() as a function name. My original patch > named it > run_script(). Dunno whats a better name. > I will go with run_command_list() so it matches run_command(). Will send an updated series with two new commits which hopefully does what you want. > > Feel free to reuse/incorporate my patch at > http://patchwork.ozlabs.org/patch/137122/ > into your patchset ;) > Thanks, have done so. > > -- > michael > Regards, Simon
Hi Michael, On Tue, Feb 14, 2012 at 2:42 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Michael, > > On Sat, Jan 21, 2012 at 8:07 AM, Michael Walle <michael@walle.cc> wrote: > >> >> Hi Simon, >> >> Am Samstag 14 Januar 2012, 07:45:52 schrieb Simon Glass: >> > diff --git a/common/cmd_source.c b/common/cmd_source.c >> > index 481241c..32fff5c 100644 >> > --- a/common/cmd_source.c >> > +++ b/common/cmd_source.c >> > @@ -179,7 +179,7 @@ source (ulong addr, const char *fit_uname) >> > if (*line) { >> > debug ("** exec: \"%s\"\n", >> > line); >> > - if (builtin_run_command(line, 0) >> < 0) >> { >> > + if (run_command(line, 0) < 0) { >> > rcode = 1; >> > break; >> > } >> > @@ -189,7 +189,7 @@ source (ulong addr, const char *fit_uname) >> > ++next; >> > } >> > if (rcode == 0 && *line) >> > - rcode = (builtin_run_command(line, 0) >= 0); >> > + rcode = (run_command(line, 0) >= 0); >> > } >> > #endif >> > free (cmd); >> >> Please have a look at this file. There is already a conditional on >> CONFIG_SYS_HUSH_PARSER, which can be removed, since it is now handled in >> run_command(). Do not forget to remove the hush.h include, too. >> > > OK > >> >> Could you move this script handling into an own function into >> common/main.c? >> This way others can easily execute whole scripts without duplicating this >> code. I'm planning to submit support for a board which needs exactly >> that, so >> there will be at least one board which make use of this. >> >> Mike proposed source_commands() as a function name. My original patch >> named it >> run_script(). Dunno whats a better name. >> > > I will go with run_command_list() so it matches run_command(). Will send > an updated series with two new commits which hopefully does what you want. > > >> >> Feel free to reuse/incorporate my patch at >> http://patchwork.ozlabs.org/patch/137122/ >> into your patchset ;) >> > > Thanks, have done so. > Unfortunately I think that this approach is a little broken. We are running a command sequence from getenv(), but while processing it, we might update the environment variable. Worse, we actually overwrite the newlines in the variable as we process it. Since this seems like a bigger change than I thought, and I think it missed the merge window, I have created a two separate patches for this so people can review it properly. Will send through shortly. Regards, Simon > > >> >> -- >> michael >> > > Regards, > Simon > >
Am Mittwoch 15 Februar 2012, 07:05:24 schrieb Simon Glass: > Unfortunately I think that this approach is a little broken. We are running > a command sequence from getenv(), but while processing it, we might update > the environment variable. Worse, we actually overwrite the newlines in the > variable as we process it. Isn't it as easy as copying the current content and processing that instead?
Hi Michael, On Wed, Feb 15, 2012 at 11:44 AM, Michael Walle <michael@walle.cc> wrote: > Am Mittwoch 15 Februar 2012, 07:05:24 schrieb Simon Glass: >> Unfortunately I think that this approach is a little broken. We are running >> a command sequence from getenv(), but while processing it, we might update >> the environment variable. Worse, we actually overwrite the newlines in the >> variable as we process it. > Isn't it as easy as copying the current content and processing that instead? Yes, that's what I have done - but it is not trivial so did not try to bring it into the old series. Regards, Simon > > > -- > Michael
diff --git a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c index 54d15ea..fba5e01 100644 --- a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c +++ b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c @@ -226,12 +226,7 @@ static void kw_sysrst_action(void) } debug("Starting %s process...\n", __FUNCTION__); -#if !defined(CONFIG_SYS_HUSH_PARSER) - ret = builtin_run_command(s, 0); -#else - ret = parse_string_outer(s, FLAG_PARSE_SEMICOLON - | FLAG_EXIT_FROM_LOOP); -#endif + ret = run_command(s, 0); if (ret < 0) debug("Error.. %s failed\n", __FUNCTION__); else diff --git a/board/esd/common/auto_update.c b/board/esd/common/auto_update.c index 4cc15fa..fc60545 100644 --- a/board/esd/common/auto_update.c +++ b/board/esd/common/auto_update.c @@ -169,7 +169,7 @@ int au_do_update(int i, long sz) k++; } - builtin_run_command(addr, 0); + run_command(addr, 0); return 0; } diff --git a/board/esd/common/cmd_loadpci.c b/board/esd/common/cmd_loadpci.c index c2bf279..8fcae63 100644 --- a/board/esd/common/cmd_loadpci.c +++ b/board/esd/common/cmd_loadpci.c @@ -110,7 +110,7 @@ int do_loadpci(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * Call run_cmd */ printf("running command at addr 0x%s ...\n", addr); - builtin_run_command((char *)la, 0); + run_command((char *)la, 0); break; default: diff --git a/board/esd/du440/du440.c b/board/esd/du440/du440.c index 75fb200..1ada1bc 100644 --- a/board/esd/du440/du440.c +++ b/board/esd/du440/du440.c @@ -831,7 +831,7 @@ int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) *d = '\0'; start = get_ticks(); - ret = builtin_run_command(cmd, 0); + ret = run_command(cmd, 0); end = get_ticks(); printf("ticks=%ld\n", (ulong)(end - start)); diff --git a/common/cmd_bedbug.c b/common/cmd_bedbug.c index 0228ee8..5b08123 100644 --- a/common/cmd_bedbug.c +++ b/common/cmd_bedbug.c @@ -237,7 +237,7 @@ void bedbug_main_loop (unsigned long addr, struct pt_regs *regs) if (len == -1) printf ("<INTERRUPT>\n"); else - rc = builtin_run_command(lastcommand, flag); + rc = run_command(lastcommand, flag); if (rc <= 0) { /* invalid command or not repeatable, forget it */ diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2e3e159..6bfef62 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1045,14 +1045,8 @@ int do_bootd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int rcode = 0; -#ifndef CONFIG_SYS_HUSH_PARSER - if (builtin_run_command(getenv("bootcmd"), flag) < 0) + if (run_command(getenv("bootcmd"), flag) < 0) rcode = 1; -#else - if (parse_string_outer(getenv("bootcmd"), - FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0) - rcode = 1; -#endif return rcode; } diff --git a/common/cmd_source.c b/common/cmd_source.c index 481241c..32fff5c 100644 --- a/common/cmd_source.c +++ b/common/cmd_source.c @@ -179,7 +179,7 @@ source (ulong addr, const char *fit_uname) if (*line) { debug ("** exec: \"%s\"\n", line); - if (builtin_run_command(line, 0) < 0) { + if (run_command(line, 0) < 0) { rcode = 1; break; } @@ -189,7 +189,7 @@ source (ulong addr, const char *fit_uname) ++next; } if (rcode == 0 && *line) - rcode = (builtin_run_command(line, 0) >= 0); + rcode = (run_command(line, 0) >= 0); } #endif free (cmd); diff --git a/common/main.c b/common/main.c index 8eec02a..62c2072 100644 --- a/common/main.c +++ b/common/main.c @@ -266,26 +266,6 @@ int abortboot(int bootdelay) # endif /* CONFIG_AUTOBOOT_KEYED */ #endif /* CONFIG_BOOTDELAY >= 0 */ -/* - * Return 0 on success, or != 0 on error. - */ -int run_command(const char *cmd, int flag) -{ -#ifndef CONFIG_SYS_HUSH_PARSER - /* - * builtin_run_command can return 0 or 1 for success, so clean up - * its result. - */ - if (builtin_run_command(cmd, flag) == -1) - return 1; - - return 0; -#else - return parse_string_outer(cmd, - FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP); -#endif -} - /****************************************************************************/ void main_loop (void) @@ -454,7 +434,7 @@ void main_loop (void) if (len == -1) puts ("<INTERRUPT>\n"); else - rc = builtin_run_command(lastcommand, flag); + rc = run_command(lastcommand, flag); if (rc <= 0) { /* invalid command or not repeatable, forget it */ @@ -1262,8 +1242,7 @@ static void process_macros (const char *input, char *output) * the environment data, which may change magicly when the command we run * creates or modifies environment variables (like "bootp" does). */ - -int builtin_run_command(const char *cmd, int flag) +static int builtin_run_command(const char *cmd, int flag) { cmd_tbl_t *cmdtp; char cmdbuf[CONFIG_SYS_CBSIZE]; /* working copy of cmd */ @@ -1388,6 +1367,26 @@ int builtin_run_command(const char *cmd, int flag) return rc ? rc : repeatable; } +/* + * Return 0 on success, or != 0 on error. + */ +int run_command(const char *cmd, int flag) +{ +#ifndef CONFIG_SYS_HUSH_PARSER + /* + * builtin_run_command can return 0 or 1 for success, so clean up + * its result. + */ + if (builtin_run_command(cmd, flag) == -1) + return 1; + + return 0; +#else + return parse_string_outer(cmd, + FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP); +#endif +} + /****************************************************************************/ #if defined(CONFIG_CMD_RUN) diff --git a/include/common.h b/include/common.h index 69818a5..1893983 100644 --- a/include/common.h +++ b/include/common.h @@ -260,7 +260,6 @@ int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen); /* common/main.c */ void main_loop (void); -int builtin_run_command(const char *cmd, int flag); int run_command(const char *cmd, int flag); int readline (const char *const prompt); int readline_into_buffer (const char *const prompt, char * buffer);
This function is only one of the parser options, so use run_command() instead. It knows how to use hush if selected. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/arm/cpu/arm926ejs/kirkwood/cpu.c | 7 +---- board/esd/common/auto_update.c | 2 +- board/esd/common/cmd_loadpci.c | 2 +- board/esd/du440/du440.c | 2 +- common/cmd_bedbug.c | 2 +- common/cmd_bootm.c | 8 +----- common/cmd_source.c | 4 +- common/main.c | 45 ++++++++++++++++----------------- include/common.h | 1 - 9 files changed, 30 insertions(+), 43 deletions(-)