[U-Boot] [PATCH 3/3] sandbox: add getopt support

Simon Glass sjg at chromium.org
Mon Feb 27 03:46:23 CET 2012


Hi Mike,

On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> Signed-off-by: Mike Frysinger <vapier at 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


More information about the U-Boot mailing list