[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 8:42 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Sunday 26 February 2012 23:33:25 Simon Glass wrote:
>> On Sun, Feb 26, 2012 at 8:08 PM, Mike Frysinger wrote:
>> > On Sunday 26 February 2012 21:50:32 Simon Glass wrote:
>> >> Given your efforts on the cmdline parsing I'm beginning to think we
>> >> should perhaps add os_printf() and os_printf_stderr() and provide an
>> >> explicit interface. It might only be useful prior to main(), then
>> >> again I'm not so sure.
>> >
>> > i've been pondering this.  on one hand, we want to parse flags as soon as
>> > possible, but on the other, we want to be able to not have to worry about
>> > what state the system is in when parsing things.  maybe we specially
>> > mark the few flags that we need very early on and parse those, and then
>> > parse the rest once the system is mostly functional ?  i can really only
>> > think of one or two flags that we *might* need very early -- namely,
>> > ones that'd select a config or fdt. on the other hand, are there things
>> > that we'd want to change that'd affect everything from console_init_f()
>> > and earlier ?
>>
>> IMO flags should purely update the initial state and therefore we
>> don't need the system to be function to parse them. Once the system is
>> functional, the various bits that are interested can go and look at
>> the state to get the info they want. IOW I don't think we need to
>> delay parsing until the system is functional - we just need to take
>> 'action' then.
>
> ok, so for example, i updated my spi flash patch to do:
>
> drivers/mtd/spi/sandbox.c:
> static int sb_cmdline_cb_spi_sf(struct sandbox_state *state, const char *arg)
> {
>    unsigned long bus, cs;
>    const char *spec = sb_spi_parse_spec(arg, &bus, &cs);
>
>    if (!spec)
>        return 1;
>
>    state->spi[bus][cs][0] = &sb_sf_ops;
>    state->spi[bus][cs][1] = spec;
>    return 0;
> }
> SB_CMDLINE_OPT(spi_sf, 1, "connect a SPI flash: <bus>:<cs>:<id>:<file>");

sandbox...give me your address and I'll send you a cheque to cover the
bytes so used :-)

>
> and people could do:
>        ./u-boot --spi_sf 0:3:m25p16:./some-file.bin
> this would connect the spi flash simulation up to the simulated spi bus 0 on cs
> 3 and have it simulate a m25p16 flash with "some-file.bin" backing it.
>
> if someone were to enter the wrong value for "0:3:m25p16:./some-file.bin", how
> do you propose we communicate that ?  the existing code can easily signal the
> higher parsing logic via return values, but then we'd just get:
>        failed to parse option: 0:3:m25p16:./some-file.bin
>
> but what exactly did the code not like ?  was it the bus # ?  the cs # ?  the
> spi flash id ?  the backing file ?  if the getopt code has access to printf(),
> we can clearly communicate to the user what is going wrong.

Yes I think printf() is useful in getopt, I just would prefer to avoid
using U-Boot's printf. It goes through all the console code, and we
might be running a test that deliberately breaks that, perhaps.
Actually this could be a pretty important thing to sort out - we need
to keep a clear boundary between test code and U-Boot code (as we
discussed over GPIO). Having the test code use U-Boot's printf() is
blurring that.

To your example, if the syntax is correct perhaps, then it got through
initial parsing. If the filename is wrong, or the bus number, then
perhaps there will be a message and failure much later. Perhaps the
initial parsing just does what it can to avoid running past an obvious
syntax error. But we can't really know success until we open the file,
or try the bus.

Regards,
Simon

> -mike