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