| Message ID | 1307386599-4256-2-git-send-email-jason.hobbs@calxeda.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear "Jason Hobbs", In message <1307386599-4256-2-git-send-email-jason.hobbs@calxeda.com> you wrote: > Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com> > --- > common/hush.c | 2 +- > common/main.c | 46 +++++++++++++++++++--------------------------- > include/common.h | 1 + > include/hush.h | 2 +- > 4 files changed, 22 insertions(+), 29 deletions(-) ... > index dcbacc9..7da6604 100644 > --- a/common/main.c > +++ b/common/main.c > @@ -342,12 +342,7 @@ void main_loop (void) > int prev = disable_ctrlc(1); /* disable Control C checking */ > # endif > > -# ifndef CONFIG_SYS_HUSH_PARSER > - run_command (p, 0); > -# else > - parse_string_outer(p, FLAG_PARSE_SEMICOLON | > - FLAG_EXIT_FROM_LOOP); > -# endif > + run_command2(p, 0); Indentation looks incorrect here? > -# endif > - } > + if (s) > + run_command2(s, 0); Indentation always by TABs please. > +int run_command2(const char *cmd, int flag) > +{ > +#ifndef CONFIG_SYS_HUSH_PARSER > + if (run_command(cmd, flag) == -1) > + return 1; > +#else > + if (parse_string_outer(cmd, > + FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0) > + return 1; > +#endif > + return 0; Why do you make this function return int when there is no tesing of the return code anywhere? And why do you make this not an inline function (or macro) so the compiler could avoid the function call overhead? Best regards, Wolfgang Denk
Dear Wolfgang, On Mon, Jun 06, 2011 at 09:16:00PM +0200, Wolfgang Denk wrote: > > -# ifndef CONFIG_SYS_HUSH_PARSER > > - run_command (p, 0); > > -# else > > - parse_string_outer(p, FLAG_PARSE_SEMICOLON | > > - FLAG_EXIT_FROM_LOOP); > > -# endif > > + run_command2(p, 0); > > Indentation looks incorrect here? Yes, it is incorrect. I will correct it in the next version of the patch. > > > -# endif > > - } > > + if (s) > > + run_command2(s, 0); > > Indentation always by TABs please. The if (s) line and the rest of this block were indented with spaces before I changed it. Should I make the cosmetic change of correct the indentation there to use tabs as part of this patch? > > +int run_command2(const char *cmd, int flag) > > +{ > > +#ifndef CONFIG_SYS_HUSH_PARSER > > + if (run_command(cmd, flag) == -1) > > + return 1; > > +#else > > + if (parse_string_outer(cmd, > > + FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0) > > + return 1; > > +#endif > > + return 0; > > Why do you make this function return int when there is no tesing of > the return code anywhere? Where run_command2 replaced run_command/parse_string_outer blocks in main.c, there was no checking of the return codes of those functions already. I do use the return code of it in cmd_pxecfg.c, which is introduced later in this patch series. > And why do you make this not an inline function (or macro) so the > compiler could avoid the function call overhead? No particular reason - I just didn't consider optimization. Is common.h the best place to place this as an inline function? Thanks, Jason
Dear "Jason Hobbs", In message <20110606195832.GA4833@jhobbs-laptop> you wrote: > > The if (s) line and the rest of this block were indented with spaces > before I changed it. Should I make the cosmetic change of correct the > indentation there to use tabs as part of this patch? Yes, please. But as a separate pacth, earlier in this series. > > Why do you make this function return int when there is no tesing of > > the return code anywhere? > > Where run_command2 replaced run_command/parse_string_outer blocks in > main.c, there was no checking of the return codes of those functions > already. I do use the return code of it in cmd_pxecfg.c, which is > introduced later in this patch series. I see. OK. > > And why do you make this not an inline function (or macro) so the > > compiler could avoid the function call overhead? > > No particular reason - I just didn't consider optimization. Is common.h > the best place to place this as an inline function? No. Sorry, I have no good recommendation at the moment either. Best regards, Wolfgang Denk
diff --git a/common/hush.c b/common/hush.c index 85a6030..940889b 100644 --- a/common/hush.c +++ b/common/hush.c @@ -3217,7 +3217,7 @@ int parse_stream_outer(struct in_str *inp, int flag) #ifndef __U_BOOT__ static int parse_string_outer(const char *s, int flag) #else -int parse_string_outer(char *s, int flag) +int parse_string_outer(const char *s, int flag) #endif /* __U_BOOT__ */ { struct in_str input; diff --git a/common/main.c b/common/main.c index dcbacc9..7da6604 100644 --- a/common/main.c +++ b/common/main.c @@ -342,12 +342,7 @@ void main_loop (void) int prev = disable_ctrlc(1); /* disable Control C checking */ # endif -# ifndef CONFIG_SYS_HUSH_PARSER - run_command (p, 0); -# else - parse_string_outer(p, FLAG_PARSE_SEMICOLON | - FLAG_EXIT_FROM_LOOP); -# endif + run_command2(p, 0); # ifdef CONFIG_AUTOBOOT_KEYED disable_ctrlc(prev); /* restore Control C checking */ @@ -392,12 +387,7 @@ void main_loop (void) int prev = disable_ctrlc(1); /* disable Control C checking */ # endif -# ifndef CONFIG_SYS_HUSH_PARSER - run_command (s, 0); -# else - parse_string_outer(s, FLAG_PARSE_SEMICOLON | - FLAG_EXIT_FROM_LOOP); -# endif + run_command2(s, 0); # ifdef CONFIG_AUTOBOOT_KEYED disable_ctrlc(prev); /* restore Control C checking */ @@ -407,14 +397,8 @@ void main_loop (void) # ifdef CONFIG_MENUKEY if (menukey == CONFIG_MENUKEY) { s = getenv("menucmd"); - if (s) { -# ifndef CONFIG_SYS_HUSH_PARSER - run_command (s, 0); -# else - parse_string_outer(s, FLAG_PARSE_SEMICOLON | - FLAG_EXIT_FROM_LOOP); -# endif - } + if (s) + run_command2(s, 0); } #endif /* CONFIG_MENUKEY */ #endif /* CONFIG_BOOTDELAY */ @@ -1396,6 +1380,19 @@ int run_command (const char *cmd, int flag) return rc ? rc : repeatable; } +int run_command2(const char *cmd, int flag) +{ +#ifndef CONFIG_SYS_HUSH_PARSER + if (run_command(cmd, flag) == -1) + return 1; +#else + if (parse_string_outer(cmd, + FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0) + return 1; +#endif + return 0; +} + /****************************************************************************/ #if defined(CONFIG_CMD_RUN) @@ -1413,14 +1410,9 @@ int do_run (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) printf ("## Error: \"%s\" not defined\n", argv[i]); return 1; } -#ifndef CONFIG_SYS_HUSH_PARSER - if (run_command (arg, flag) == -1) - return 1; -#else - if (parse_string_outer(arg, - FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0) + + if (run_command2(arg, flag) != 0) return 1; -#endif } return 0; } diff --git a/include/common.h b/include/common.h index 1e4a6a5..e659630 100644 --- a/include/common.h +++ b/include/common.h @@ -228,6 +228,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_command2 (const char *cmd, int flag); int readline (const char *const prompt); int readline_into_buffer (const char *const prompt, char * buffer); int parse_line (char *, char *[]); diff --git a/include/hush.h b/include/hush.h index 5c566cc..ecf9222 100644 --- a/include/hush.h +++ b/include/hush.h @@ -29,7 +29,7 @@ #define FLAG_REPARSING (1 << 2) /* >=2nd pass */ extern int u_boot_hush_start(void); -extern int parse_string_outer(char *, int); +extern int parse_string_outer(const char *, int); extern int parse_file_outer(void); int set_local_var(const char *s, int flg_export);
Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com> --- common/hush.c | 2 +- common/main.c | 46 +++++++++++++++++++--------------------------- include/common.h | 1 + include/hush.h | 2 +- 4 files changed, 22 insertions(+), 29 deletions(-)