Hi Mike,
On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger <vap...@gentoo.org> wrote:
> Signed-off-by: Mike Frysinger <vap...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
