[Toybox] Mutually Exclusive Arguments. Bug?

[briggers at ninthfloor.org]

On Sun, 3 Jun 2012, Rob Landley wrote:

> On 06/03/2012 08:07 AM, briggers at ninthfloor.org wrote:
>> Hi,
>>
>> I was trying to use the ~ control character to implement mutually
>> exclusive option arguments.
>
> Yeah, during why week off I came to the conclusion that a lot of the
> args stuff was limited, and specced out new infrastructure for it.
> Unfortunately, I haven't had time to finish implementing it:
>
> --- a/lib/args.c        Sun Jun 03 00:32:12 2012 -0500
> +++ b/lib/args.c        Sun Jun 03 21:08:41 2012 -0500
> @@ -29,11 +29,12 @@
> //    " " (space char) the "plus an  argument" must be separate
> //        I.E. "-j 3" not "-j3". So "kill -stop" != "kill -s top"
> //
> -//     These modify other option letters (previously seen in string):
> -//       +X enabling this enables X (switch on)
> -//       ~X enabling this disables X (switch off)
> -//       !X die with error if X already set (x!x die if x supplied twice)
> -//       [yz] needs at least one of y or z. TODO
> +//   at the end:
> +//     [#xyz] Groups. First char is type:
> +//       + enabling one enables all (switch on)
> +//       - enabling one disables the rest (switch off)
> +//       ! die with error if others already set
> +//
> //   at the beginning:
> //     ^ stop at first nonoption argument
> //     <0 die if less than # leftover arguments (default 0)
>
>
> Commit 583 was part of the cleanup I did adjacent to that:
>
>  http://landley.net/hg/toybox/rev/583
>
>> pwd takes two options -P and -L, which are mutually exclusive. Using the
>> ~ control character the optstring should be "L~PP~L". Using this
>> optstring gives a "Segmentation fault (core dumped)" error when running
>> the pwd command.
>
> There's a debug option that prints error messages if an option string
> tries to do something the infrastructure can't do. It defaults to off.
>
> The _new_ way of doing that would be "LP[!LP]"
>
> The problem with the current infrastructure is that you can only refer
> to entries it's already seen earlier in the string, so L~P can't find
> P because the structure describing P isn't initialized yet, and the
> logic to test for a null pointer and print an error is under a config
> optino (because it's a "should never happen" in normal use, so wasting
> the bytes on an error message makes the code bigger).
>
> It's this bit in lib/args.c by the way:
>
>    if (!*++options && CFG_TOYBOX_DEBUG)
>        error_exit("+~! no target");
>    // Find this option flag (in previously parsed struct opt)
>    for (i=0, opt = new; ; opt = opt->next) {
>        if (CFG_TOYBOX_DEBUG && !opt)
>            error_exit("+~! unknown target");
>        if (opt->c == *options) break;
>        i++;
>    }
>
>> For optstring "LP~L" the command runs. But since the command line
>> arguments are parsed from left to right "-L -P" results in an optflag
>> value of 1, and "-P -L" gives an optflag value of 3. Since the options
>> are mutually exclusive, the optflag value should be either 1 or 2 (or 0
>> incase of no arguments)
>
> Which is exactly why I need to define groups in a different way.
>
> The inability to refer to things which haven't been defined yet is
> a design limit in parse_optflaglist(), and the easy way to fix it
> is to define a new syntax that goes at the _end_ of the string to
> refer to relationships between groups of options. Which is what
> I'm doing.

I hope you don't mind my saying this, but do you think defining a new 
syntax is a good idea? It seems the problem is with the implementation 
rather that the current syntax itself. The current syntax basically 
already supports mutually exclusive arguments, the implementation doesnt. 
Defining a new syntax not only involes implementing the new syntax but 
might also entail going back and fixing a lot of the old code (And all the 
documentation). With the old syntax you could just redo the implementation 
to build the structures for all the options first before populating the 
additional options, without breaking any of the toys code.

Unless of course you have grander plans that would benefit from a rewrite.

>
>> optstring "L~PP" also gives "Segmentation fault (core dumped)" error.
>>
>> It seems disabling an option before it is even defined in the optstring
>> is the cause for the segmentation fault. I'm not sure if this is
>> intentional or not.
>
> It's a known limit in the design, which I have a plan to address.
> I can prioritize finishing this since there's a user waiting, but
> my week off just ran out and I go back to work in the morning. :(
>
>> I'm at a loss at trying to implement order independent argument
>> exclusion without using ~. Ideas, suggestions?
>
> So was I.  My idea is to rip out the ~ suffix and write better
> infrastructure.  (It does mean I can't create non-symmetric
> relationships anymore, but I can't think of a use case for that,
> and could aways just stick an if() in the command's main if
> I need something truly oddball.
>
> For right now, implement it with an if() in main to test for
> it, and I'll clean it up to have the option parsing do it
> automatically once the infrastructure's in.  Sound good?

My plan was to go about adding missing functionality to the commands 
already present. If you intend to make infrastructure changes I might just 
hold off till those are finished. If you already have a plan/design for 
the infrastructure changes and only need to implement them, let me know 
and I can help out with that instead.

>
> Thanks,
>
> Rob
> -- 
> GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code.
> Either it's "mere aggregation", or a license violation.  Pick one.
>


--
Ashish Bhate

 1338915455.0