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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120226/e5810669/attachment.pgp>