| Message ID | 1330295931-18425-3-git-send-email-vapier@gentoo.org |
|---|---|
| State | Accepted |
| Delegated to: | Mike Frysinger |
| Headers | show |
Hi Mike, On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger <vapier@gentoo.org> wrote: > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > --- > arch/sandbox/cpu/os.c | 64 ++++++++++++++++++++++ > arch/sandbox/cpu/start.c | 83 +++++++++++++++++++++++++++++ > arch/sandbox/cpu/u-boot.lds | 4 ++ > arch/sandbox/include/asm/state.h | 5 ++ > arch/sandbox/include/asm/u-boot-sandbox.h | 1 + > arch/sandbox/lib/board.c | 1 + > include/os.h | 35 ++++++++++++ > 7 files changed, 193 insertions(+), 0 deletions(-) > > diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c > index cb469e0..4b1c987 100644 > --- a/arch/sandbox/cpu/os.c > +++ b/arch/sandbox/cpu/os.c > @@ -21,6 +21,7 @@ > > #include <errno.h> > #include <fcntl.h> > +#include <getopt.h> > #include <stdlib.h> > #include <termios.h> > #include <time.h> > @@ -32,6 +33,7 @@ > #include <linux/types.h> > > #include <os.h> > +#include <asm/state.h> > > /* Operating System Interface */ > > @@ -155,3 +157,65 @@ u64 os_get_nsec(void) > return tv.tv_sec * 1000000000ULL + tv.tv_usec * 1000; > #endif > } > + > +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], > + *__u_boot_sb_getopt_end[]; I wonder if we can put this in asm-generic/sections.h - or perhaps that doesn't exist yet? Also how about __u_boot_sandbox_option_start/end? I'm really not keen on 'sb'. > +static char *short_opts; > +static struct option *long_opts; > + > +int os_parse_args(struct sandbox_state *state, int argc, char *argv[]) > +{ > + struct sb_cmdline_option **sb_opt = __u_boot_sb_getopt_start; > + size_t num_flags = __u_boot_sb_getopt_end - __u_boot_sb_getopt_start; num_options? > + size_t i; > + > + int hidden_short_opt; > + size_t si; si? > + > + int c; > + > + if (short_opts || long_opts) > + os_exit(1); > + > + state->argc = argc; > + state->argv = argv; > + > + short_opts = os_malloc(sizeof(*short_opts) * (num_flags + 1)); Comment on why +1? is it for \0 terminator? > + long_opts = os_malloc(sizeof(*long_opts) * num_flags); > + if (!short_opts || !long_opts) > + os_exit(1); > + > + si = 0; > + hidden_short_opt = 0x80; > + for (i = 0; i < num_flags; ++i) { > + long_opts[i].name = sb_opt[i]->flag; > + long_opts[i].has_arg = sb_opt[i]->has_arg ? > + required_argument : no_argument; > + long_opts[i].flag = NULL; > + > + if (sb_opt[i]->flag_short) > + short_opts[si++] = long_opts[i].val = sb_opt[i]->flag_short; > + else > + long_opts[i].val = sb_opt[i]->flag_short = hidden_short_opt++; What is this hidden_short_opt for? Suggest a comment. > + } > + short_opts[si] = '\0'; > + > + /* We need to handle output ourselves since u-boot provides printf */ > + opterr = 0; > + > + while ((c = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) { > + for (i = 0; i < num_flags; ++i) { > + if (sb_opt[i]->flag_short == c) { > + sb_opt[i]->callback(state, optarg); > + break; > + } > + } > + if (i == num_flags) { > + /* store the faulting flag index for later */ > + state->parse_err = -optind; > + break; > + } > + } > + > + return 0; > +} > diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c > index dc020ee..895ec49 100644 > --- a/arch/sandbox/cpu/start.c > +++ b/arch/sandbox/cpu/start.c > @@ -22,19 +22,102 @@ > #include <common.h> > #include <asm/state.h> > > +#include <os.h> > + > +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], > + *__u_boot_sb_getopt_end[]; As above > + > +int sb_early_getopt_check(void) > +{ > + struct sandbox_state *state = state_get_current(); > + struct sb_cmdline_option **sb_opt = __u_boot_sb_getopt_start; > + size_t num_flags = __u_boot_sb_getopt_end - __u_boot_sb_getopt_start; num_options again > + size_t i; > + int max_arg_len, max_noarg_len; > + > + if (state->parse_err == 0) > + return 0; > + Comment: parse_error stores -n where n is the index of the option that we faulted. > + if (state->parse_err < 0) > + printf("error: unknown option: %s\n\n", > + state->argv[-state->parse_err - 1]); > + > + printf( do we need this extra newline? > + "u-boot, a command line test interface to U-Boot\n\n" > + "Usage: u-boot [options]\n" > + "Options:\n"); > + > + max_arg_len = 0; > + for (i = 0; i < num_flags; ++i) > + max_arg_len = max(strlen(sb_opt[i]->flag), max_arg_len); > + max_noarg_len = max_arg_len + 7; > + > + for (i = 0; i < num_flags; ++i) { > + /* first output the short flag if it has one */ > + if (sb_opt[i]->flag_short >= 0x80) Can we declare a pointer to sb_opt[i] and the top here and use it below? > + printf(" "); > + else > + printf(" -%c, ", sb_opt[i]->flag_short); > + > + /* then the long flag */ > + if (sb_opt[i]->has_arg) > + printf("--%-*s", max_noarg_len, sb_opt[i]->flag); > + else > + printf("--%-*s <arg> ", max_arg_len, sb_opt[i]->flag); > + > + /* finally the help text */ > + printf(" %s\n", sb_opt[i]->help); puts() might be safer, but then again I think we have vsnprintf() turned on now. > + } > + > + os_exit(state->parse_err < 0 ? 1 : 0); > +} > + > +static int sb_cmdline_cb_help(struct sandbox_state *state, const char *arg) > +{ > + /* just flag to sb_early_getopt_check to show usage */ > + state->parse_err = 1; > + return 0; > +} > +SB_CMDLINE_OPT_SHORT(help, 'h', 0, "Display help"); > + > int sb_main_loop_init(void) > { > + struct sandbox_state *state = state_get_current(); > + > + /* Execute command if required */ > + if (state->cmd) { > + /* TODO: redo this when cmd tidy-up series lands */ > +#ifdef CONFIG_SYS_HUSH_PARSER > + run_command(state->cmd, 0); > +#else > + parse_string_outer(state->cmd, FLAG_PARSE_SEMICOLON | > + FLAG_EXIT_FROM_LOOP); > +#endif > + os_exit(state->exit_type); > + } > + > + return 0; > +} > + > +static int sb_cmdline_cb_command(struct sandbox_state *state, const char *arg) > +{ > + state->cmd = arg; > return 0; > } > +SB_CMDLINE_OPT_SHORT(command, 'c', 1, "Execute U-Boot command"); > > int main(int argc, char *argv[]) > { > + struct sandbox_state *state; > int err; > > err = state_init(); > if (err) > return err; > > + state = state_get_current(); > + os_parse_args(state, argc, argv); We don't check the return value. Perhaps add a comment as to why. > + > /* > * Do pre- and post-relocation init, then start up U-Boot. This will > * never return. > diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds > index 0c56aa7..2ca6b8c 100644 > --- a/arch/sandbox/cpu/u-boot.lds > +++ b/arch/sandbox/cpu/u-boot.lds > @@ -28,6 +28,10 @@ SECTIONS > _u_boot_cmd : { *(.u_boot_cmd) } > __u_boot_cmd_end = .; > > + __u_boot_sb_getopt_start = .; > + _u_boot_sb_getopt : { *(.u_boot_sb_getopt) } > + __u_boot_sb_getopt_end = .; > + > __bss_start = .; > } > > diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h > index 5b34e94..edeef08 100644 > --- a/arch/sandbox/include/asm/state.h > +++ b/arch/sandbox/include/asm/state.h > @@ -22,6 +22,8 @@ > #ifndef __SANDBOX_STATE_H > #define __SANDBOX_STATE_H > > +#include <config.h> > + > /* How we exited U-Boot */ > enum exit_type_id { > STATE_EXIT_NORMAL, > @@ -33,6 +35,9 @@ enum exit_type_id { > struct sandbox_state { > const char *cmd; /* Command to execute */ > enum exit_type_id exit_type; /* How we exited U-Boot */ > + int parse_err; /* Error to report from parsing */ > + int argc; /* Program arguments */ > + char **argv; > }; > > /** > diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h > index f0e8b3c..8255e9d 100644 > --- a/arch/sandbox/include/asm/u-boot-sandbox.h > +++ b/arch/sandbox/include/asm/u-boot-sandbox.h > @@ -36,6 +36,7 @@ int board_init(void); > int dram_init(void); > > /* start.c */ > +int sb_early_getopt_check(void); > int sb_main_loop_init(void); > > #endif /* _U_BOOT_SANDBOX_H_ */ > diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c > index 1082e7d..5c6da5b 100644 > --- a/arch/sandbox/lib/board.c > +++ b/arch/sandbox/lib/board.c > @@ -134,6 +134,7 @@ init_fnc_t *init_sequence[] = { > env_init, /* initialize environment */ > serial_init, /* serial communications setup */ > console_init_f, /* stage 1 init of console */ > + sb_early_getopt_check, comment > display_banner, /* say that we are here */ > #if defined(CONFIG_DISPLAY_CPUINFO) > print_cpuinfo, /* display cpu info (and speed) */ > diff --git a/include/os.h b/include/os.h > index 6b7ee47..aea4503 100644 > --- a/include/os.h > +++ b/include/os.h > @@ -27,6 +27,8 @@ > #ifndef __OS_H__ > #define __OS_H__ > > +struct sandbox_state; > + > /** > * Access to the OS read() system call > * > @@ -122,4 +124,37 @@ void os_usleep(unsigned long usec); > */ > u64 os_get_nsec(void); > > +/** > + * Parse arguments and update sandbox state. > + * > + * @param state Sandbox state to update > + * @param argc Argument count > + * @param argv Argument vector > + * @return 0 if ok, and program should continue; > + * 1 if ok, but program should stop; > + * -1 on error: program should terminate. > + */ > +int os_parse_args(struct sandbox_state *state, int argc, char *argv[]); > + > +struct sb_cmdline_option { > + const char *flag; comment each of these members > + char flag_short; > + const char *help; > + int has_arg; > + int (*callback)(struct sandbox_state *state, const char *opt); comment this callback > +}; > +#define _SB_CMDLINE_OPT(f, s, ha, h) \ comment: declare an option to be understood by sandbox... > + static struct sb_cmdline_option sb_cmdline_option_##f = { \ > + .flag = #f, \ > + .flag_short = s, \ > + .help = h, \ > + .has_arg = ha, \ > + .callback = sb_cmdline_cb_##f, \ > + }; \ > + static __attribute__((section(".u_boot_sb_getopt"), used)) \ > + struct sb_cmdline_option *sb_cmdline_option_##f##_ptr = \ > + &sb_cmdline_option_##f > +#define SB_CMDLINE_OPT(f, ha, h) _SB_CMDLINE_OPT(f, 0, ha, h) > +#define SB_CMDLINE_OPT_SHORT(f, s, ha, h) _SB_CMDLINE_OPT(f, s, ha, h) SANDBOX: also please comment these, perhaps with args also. > + > #endif > -- > 1.7.8.4 > Regards, Simon
On Sunday 26 February 2012 21:46:23 Simon Glass wrote: > On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote: > > --- a/arch/sandbox/cpu/os.c > > +++ b/arch/sandbox/cpu/os.c > > > > +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], > > + *__u_boot_sb_getopt_end[]; > > I wonder if we can put this in asm-generic/sections.h - or perhaps > that doesn't exist yet? sorry, should have labeled this patch more as a PoC as i know it requires a little more polish. these would go into sandbox's asm/sections.h since these are specific to the sandbox port. > Also how about __u_boot_sandbox_option_start/end? I'm really not keen on > 'sb'. for these two vars, that's fine. i prefer "sb" for internal static stuff since "sandbox" can get wordy real fast. > > + int hidden_short_opt; > > + size_t si; > > si? short_opt_index is the self-commenting name > > + short_opts = os_malloc(sizeof(*short_opts) * (num_flags + 1)); > > Comment on why +1? is it for \0 terminator? yes, the getopt_long() api requires a NUL terminated string > > + si = 0; > > + hidden_short_opt = 0x80; > > + for (i = 0; i < num_flags; ++i) { > > + long_opts[i].name = sb_opt[i]->flag; > > + long_opts[i].has_arg = sb_opt[i]->has_arg ? > > + required_argument : no_argument; > > + long_opts[i].flag = NULL; > > + > > + if (sb_opt[i]->flag_short) > > + short_opts[si++] = long_opts[i].val = > > sb_opt[i]->flag_short; + else > > + long_opts[i].val = sb_opt[i]->flag_short = > > hidden_short_opt++; > > What is this hidden_short_opt for? Suggest a comment. i think most options we have will be long only. much harder to make sure you don't have collisions in the entire base when the flag definition is in the subfiles. but getopt_long() needs a unique int for each long flag, so "hidden" just means "not an ascii char". > > + if (state->parse_err < 0) > > + printf("error: unknown option: %s\n\n", > > + state->argv[-state->parse_err - 1]); > > + > > + printf( > > do we need this extra newline? i find the extra newline helps to differentiate from the noise when we turn around and dump the --help output. alternative would be to not automatically show --help. > > + for (i = 0; i < num_flags; ++i) { > > + /* first output the short flag if it has one */ > > + if (sb_opt[i]->flag_short >= 0x80) > > Can we declare a pointer to sb_opt[i] and the top here and use it below? yes > > + printf(" "); > > + else > > + printf(" -%c, ", sb_opt[i]->flag_short); > > + > > + /* then the long flag */ > > + if (sb_opt[i]->has_arg) > > + printf("--%-*s", max_noarg_len, sb_opt[i]->flag); > > + else > > + printf("--%-*s <arg> ", max_arg_len, > > sb_opt[i]->flag); + > > + /* finally the help text */ > > + printf(" %s\n", sb_opt[i]->help); > > puts() might be safer, but then again I think we have vsnprintf() turned on > now. not sure what you mean by "safer" ... if you mean the implicit stack overflows, i don't think that'll be an issue here. the help strings aren't very long. > > int main(int argc, char *argv[]) > > { > > ... > > + state = state_get_current(); > > + os_parse_args(state, argc, argv); > > We don't check the return value. Perhaps add a comment as to why. since the code takes care of setting parse_err itself now, i'm not sure what to do with the return value -mike
Hi Mike, On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Sunday 26 February 2012 21:46:23 Simon Glass wrote: >> On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote: >> > --- a/arch/sandbox/cpu/os.c >> > +++ b/arch/sandbox/cpu/os.c >> > >> > +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], >> > + *__u_boot_sb_getopt_end[]; >> >> I wonder if we can put this in asm-generic/sections.h - or perhaps >> that doesn't exist yet? > > sorry, should have labeled this patch more as a PoC as i know it requires a > little more polish. these would go into sandbox's asm/sections.h since these > are specific to the sandbox port. > >> Also how about __u_boot_sandbox_option_start/end? I'm really not keen on >> 'sb'. > > for these two vars, that's fine. i prefer "sb" for internal static stuff since > "sandbox" can get wordy real fast. > >> > + int hidden_short_opt; >> > + size_t si; >> >> si? > > short_opt_index is the self-commenting name > >> > + short_opts = os_malloc(sizeof(*short_opts) * (num_flags + 1)); >> >> Comment on why +1? is it for \0 terminator? > > yes, the getopt_long() api requires a NUL terminated string > >> > + si = 0; >> > + hidden_short_opt = 0x80; >> > + for (i = 0; i < num_flags; ++i) { >> > + long_opts[i].name = sb_opt[i]->flag; >> > + long_opts[i].has_arg = sb_opt[i]->has_arg ? >> > + required_argument : no_argument; >> > + long_opts[i].flag = NULL; >> > + >> > + if (sb_opt[i]->flag_short) >> > + short_opts[si++] = long_opts[i].val = >> > sb_opt[i]->flag_short; + else >> > + long_opts[i].val = sb_opt[i]->flag_short = >> > hidden_short_opt++; >> >> What is this hidden_short_opt for? Suggest a comment. > > i think most options we have will be long only. much harder to make sure you > don't have collisions in the entire base when the flag definition is in the > subfiles. but getopt_long() needs a unique int for each long flag, so "hidden" > just means "not an ascii char". > >> > + if (state->parse_err < 0) >> > + printf("error: unknown option: %s\n\n", >> > + state->argv[-state->parse_err - 1]); >> > + >> > + printf( >> >> do we need this extra newline? > > i find the extra newline helps to differentiate from the noise when we turn > around and dump the --help output. alternative would be to not automatically > show --help. > >> > + for (i = 0; i < num_flags; ++i) { >> > + /* first output the short flag if it has one */ >> > + if (sb_opt[i]->flag_short >= 0x80) >> >> Can we declare a pointer to sb_opt[i] and the top here and use it below? > > yes > >> > + printf(" "); >> > + else >> > + printf(" -%c, ", sb_opt[i]->flag_short); >> > + >> > + /* then the long flag */ >> > + if (sb_opt[i]->has_arg) >> > + printf("--%-*s", max_noarg_len, sb_opt[i]->flag); >> > + else >> > + printf("--%-*s <arg> ", max_arg_len, >> > sb_opt[i]->flag); + >> > + /* finally the help text */ >> > + printf(" %s\n", sb_opt[i]->help); >> >> puts() might be safer, but then again I think we have vsnprintf() turned on >> now. > > not sure what you mean by "safer" ... if you mean the implicit stack overflows, > i don't think that'll be an issue here. the help strings aren't very long. > >> > int main(int argc, char *argv[]) >> > { >> > ... >> > + state = state_get_current(); >> > + os_parse_args(state, argc, argv); >> >> We don't check the return value. Perhaps add a comment as to why. > > since the code takes care of setting parse_err itself now, i'm not sure what > to do with the return value I agree it is right, just asking for a comment. Same with most of my other things - more comments as I suggested is nice for people that come into the code fresh. Regards, Simon > -mike
On Sunday 26 February 2012 23:26:52 Simon Glass wrote: > On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger wrote: > > On Sunday 26 February 2012 21:46:23 Simon Glass wrote: > >> On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote: > >> > int main(int argc, char *argv[]) > >> > { > >> > ... > >> > + state = state_get_current(); > >> > + os_parse_args(state, argc, argv); > >> > >> We don't check the return value. Perhaps add a comment as to why. > > > > since the code takes care of setting parse_err itself now, i'm not sure > > what to do with the return value > > I agree it is right, just asking for a comment. Same with most of my > other things - more comments as I suggested is nice for people that > come into the code fresh. my plan was to clean this up a bit more before submitting (such as adding comments). i was looking more for feedback on the general approach here and any fundamental sticking points since this is a semi-radical departure from what either of us posted earlier. in this case, i'm asking: should we just toss the return value and have it be void ? -mike
Hi Mike, On Sun, Feb 26, 2012 at 8:35 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Sunday 26 February 2012 23:26:52 Simon Glass wrote: >> On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger wrote: >> > On Sunday 26 February 2012 21:46:23 Simon Glass wrote: >> >> On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote: >> >> > int main(int argc, char *argv[]) >> >> > { >> >> > ... >> >> > + state = state_get_current(); >> >> > + os_parse_args(state, argc, argv); >> >> >> >> We don't check the return value. Perhaps add a comment as to why. >> > >> > since the code takes care of setting parse_err itself now, i'm not sure >> > what to do with the return value >> >> I agree it is right, just asking for a comment. Same with most of my >> other things - more comments as I suggested is nice for people that >> come into the code fresh. > > my plan was to clean this up a bit more before submitting (such as adding > comments). i was looking more for feedback on the general approach here and > any fundamental sticking points since this is a semi-radical departure from > what either of us posted earlier. OK I see. Well actually I was expecting that we would need something along these lines eventually, since arg parsing belongs with the module that provides the argument I think. So as well to have it now, even if it doesn't have a lot of uses yet. It will. > > in this case, i'm asking: should we just toss the return value and have it be > void ? I suggest not - even if we ignore it, it seems like information that should be returned. Perhaps we should make it more explicit by returning state->return_code with a comment that callers can use it now or later. But I think the way you have done it is fine. Regards, Simon > -mike
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index cb469e0..4b1c987 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -21,6 +21,7 @@ #include <errno.h> #include <fcntl.h> +#include <getopt.h> #include <stdlib.h> #include <termios.h> #include <time.h> @@ -32,6 +33,7 @@ #include <linux/types.h> #include <os.h> +#include <asm/state.h> /* Operating System Interface */ @@ -155,3 +157,65 @@ u64 os_get_nsec(void) return tv.tv_sec * 1000000000ULL + tv.tv_usec * 1000; #endif } + +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], + *__u_boot_sb_getopt_end[]; +static char *short_opts; +static struct option *long_opts; + +int os_parse_args(struct sandbox_state *state, int argc, char *argv[]) +{ + struct sb_cmdline_option **sb_opt = __u_boot_sb_getopt_start; + size_t num_flags = __u_boot_sb_getopt_end - __u_boot_sb_getopt_start; + size_t i; + + int hidden_short_opt; + size_t si; + + int c; + + if (short_opts || long_opts) + os_exit(1); + + state->argc = argc; + state->argv = argv; + + short_opts = os_malloc(sizeof(*short_opts) * (num_flags + 1)); + long_opts = os_malloc(sizeof(*long_opts) * num_flags); + if (!short_opts || !long_opts) + os_exit(1); + + si = 0; + hidden_short_opt = 0x80; + for (i = 0; i < num_flags; ++i) { + long_opts[i].name = sb_opt[i]->flag; + long_opts[i].has_arg = sb_opt[i]->has_arg ? + required_argument : no_argument; + long_opts[i].flag = NULL; + + if (sb_opt[i]->flag_short) + short_opts[si++] = long_opts[i].val = sb_opt[i]->flag_short; + else + long_opts[i].val = sb_opt[i]->flag_short = hidden_short_opt++; + } + short_opts[si] = '\0'; + + /* We need to handle output ourselves since u-boot provides printf */ + opterr = 0; + + while ((c = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) { + for (i = 0; i < num_flags; ++i) { + if (sb_opt[i]->flag_short == c) { + sb_opt[i]->callback(state, optarg); + break; + } + } + if (i == num_flags) { + /* store the faulting flag index for later */ + state->parse_err = -optind; + break; + } + } + + return 0; +} diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index dc020ee..895ec49 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -22,19 +22,102 @@ #include <common.h> #include <asm/state.h> +#include <os.h> + +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[], + *__u_boot_sb_getopt_end[]; + +int sb_early_getopt_check(void) +{ + struct sandbox_state *state = state_get_current(); + struct sb_cmdline_option **sb_opt = __u_boot_sb_getopt_start; + size_t num_flags = __u_boot_sb_getopt_end - __u_boot_sb_getopt_start; + size_t i; + int max_arg_len, max_noarg_len; + + if (state->parse_err == 0) + return 0; + + if (state->parse_err < 0) + printf("error: unknown option: %s\n\n", + state->argv[-state->parse_err - 1]); + + printf( + "u-boot, a command line test interface to U-Boot\n\n" + "Usage: u-boot [options]\n" + "Options:\n"); + + max_arg_len = 0; + for (i = 0; i < num_flags; ++i) + max_arg_len = max(strlen(sb_opt[i]->flag), max_arg_len); + max_noarg_len = max_arg_len + 7; + + for (i = 0; i < num_flags; ++i) { + /* first output the short flag if it has one */ + if (sb_opt[i]->flag_short >= 0x80) + printf(" "); + else + printf(" -%c, ", sb_opt[i]->flag_short); + + /* then the long flag */ + if (sb_opt[i]->has_arg) + printf("--%-*s", max_noarg_len, sb_opt[i]->flag); + else + printf("--%-*s <arg> ", max_arg_len, sb_opt[i]->flag); + + /* finally the help text */ + printf(" %s\n", sb_opt[i]->help); + } + + os_exit(state->parse_err < 0 ? 1 : 0); +} + +static int sb_cmdline_cb_help(struct sandbox_state *state, const char *arg) +{ + /* just flag to sb_early_getopt_check to show usage */ + state->parse_err = 1; + return 0; +} +SB_CMDLINE_OPT_SHORT(help, 'h', 0, "Display help"); + int sb_main_loop_init(void) { + struct sandbox_state *state = state_get_current(); + + /* Execute command if required */ + if (state->cmd) { + /* TODO: redo this when cmd tidy-up series lands */ +#ifdef CONFIG_SYS_HUSH_PARSER + run_command(state->cmd, 0); +#else + parse_string_outer(state->cmd, FLAG_PARSE_SEMICOLON | + FLAG_EXIT_FROM_LOOP); +#endif + os_exit(state->exit_type); + } + + return 0; +} + +static int sb_cmdline_cb_command(struct sandbox_state *state, const char *arg) +{ + state->cmd = arg; return 0; } +SB_CMDLINE_OPT_SHORT(command, 'c', 1, "Execute U-Boot command"); int main(int argc, char *argv[]) { + struct sandbox_state *state; int err; err = state_init(); if (err) return err; + state = state_get_current(); + os_parse_args(state, argc, argv); + /* * Do pre- and post-relocation init, then start up U-Boot. This will * never return. diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds index 0c56aa7..2ca6b8c 100644 --- a/arch/sandbox/cpu/u-boot.lds +++ b/arch/sandbox/cpu/u-boot.lds @@ -28,6 +28,10 @@ SECTIONS _u_boot_cmd : { *(.u_boot_cmd) } __u_boot_cmd_end = .; + __u_boot_sb_getopt_start = .; + _u_boot_sb_getopt : { *(.u_boot_sb_getopt) } + __u_boot_sb_getopt_end = .; + __bss_start = .; } diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index 5b34e94..edeef08 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -22,6 +22,8 @@ #ifndef __SANDBOX_STATE_H #define __SANDBOX_STATE_H +#include <config.h> + /* How we exited U-Boot */ enum exit_type_id { STATE_EXIT_NORMAL, @@ -33,6 +35,9 @@ enum exit_type_id { struct sandbox_state { const char *cmd; /* Command to execute */ enum exit_type_id exit_type; /* How we exited U-Boot */ + int parse_err; /* Error to report from parsing */ + int argc; /* Program arguments */ + char **argv; }; /** diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h index f0e8b3c..8255e9d 100644 --- a/arch/sandbox/include/asm/u-boot-sandbox.h +++ b/arch/sandbox/include/asm/u-boot-sandbox.h @@ -36,6 +36,7 @@ int board_init(void); int dram_init(void); /* start.c */ +int sb_early_getopt_check(void); int sb_main_loop_init(void); #endif /* _U_BOOT_SANDBOX_H_ */ diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c index 1082e7d..5c6da5b 100644 --- a/arch/sandbox/lib/board.c +++ b/arch/sandbox/lib/board.c @@ -134,6 +134,7 @@ init_fnc_t *init_sequence[] = { env_init, /* initialize environment */ serial_init, /* serial communications setup */ console_init_f, /* stage 1 init of console */ + sb_early_getopt_check, display_banner, /* say that we are here */ #if defined(CONFIG_DISPLAY_CPUINFO) print_cpuinfo, /* display cpu info (and speed) */ diff --git a/include/os.h b/include/os.h index 6b7ee47..aea4503 100644 --- a/include/os.h +++ b/include/os.h @@ -27,6 +27,8 @@ #ifndef __OS_H__ #define __OS_H__ +struct sandbox_state; + /** * Access to the OS read() system call * @@ -122,4 +124,37 @@ void os_usleep(unsigned long usec); */ u64 os_get_nsec(void); +/** + * Parse arguments and update sandbox state. + * + * @param state Sandbox state to update + * @param argc Argument count + * @param argv Argument vector + * @return 0 if ok, and program should continue; + * 1 if ok, but program should stop; + * -1 on error: program should terminate. + */ +int os_parse_args(struct sandbox_state *state, int argc, char *argv[]); + +struct sb_cmdline_option { + const char *flag; + char flag_short; + const char *help; + int has_arg; + int (*callback)(struct sandbox_state *state, const char *opt); +}; +#define _SB_CMDLINE_OPT(f, s, ha, h) \ + static struct sb_cmdline_option sb_cmdline_option_##f = { \ + .flag = #f, \ + .flag_short = s, \ + .help = h, \ + .has_arg = ha, \ + .callback = sb_cmdline_cb_##f, \ + }; \ + static __attribute__((section(".u_boot_sb_getopt"), used)) \ + struct sb_cmdline_option *sb_cmdline_option_##f##_ptr = \ + &sb_cmdline_option_##f +#define SB_CMDLINE_OPT(f, ha, h) _SB_CMDLINE_OPT(f, 0, ha, h) +#define SB_CMDLINE_OPT_SHORT(f, s, ha, h) _SB_CMDLINE_OPT(f, s, ha, h) + #endif
Signed-off-by: Mike Frysinger <vapier@gentoo.org> --- arch/sandbox/cpu/os.c | 64 ++++++++++++++++++++++ arch/sandbox/cpu/start.c | 83 +++++++++++++++++++++++++++++ arch/sandbox/cpu/u-boot.lds | 4 ++ arch/sandbox/include/asm/state.h | 5 ++ arch/sandbox/include/asm/u-boot-sandbox.h | 1 + arch/sandbox/lib/board.c | 1 + include/os.h | 35 ++++++++++++ 7 files changed, 193 insertions(+), 0 deletions(-)