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

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