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

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger <vapier at 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