[PATCH] option-parser: Handle short double-arg options

[Bryce Harrington]

On Thu, Apr 14, 2016 at 03:16:07PM +0300, Pekka Paalanen wrote:
> On Sat, 13 Feb 2016 23:56:38 +0100
> Benoit Gschwind <gschwind at gnu-log.net> wrote:
> 
> > Hello Bryce,
> > 
> > It seems the corner case '-f42xxx 2938475' doesn't work as expected with 
> > 'f' short option as integer:
> > 
> > 1. one dash then call short_option
> > 2. in short_option will check arg[2] and call handle_option
> > 3. in handle_option will call strtol, where *value is '4' and *p is 'x' 
> > thus *value && !*p is 0
> > 4. return from handle_option with 0
> > 5. return from short_option with 0
> > 6. call short_option_with_arg with arg = "-f42xxx" and param = "2938475"
> > 7. pass through all test and call handle_option
> > 8. give 2938475 to the option
> > 
> > The expected behavior is an error.
> > 
> > Should I use Reviewed-by ?
> 
> No, you only give Reviewed-by when you think everything is good. You
> are pointing out a problem instead.
> 
> Looks like this patch landed. Isn't there something to fix?

The option handling code is relatively concise, which is good but the
consequence is that there's a heap of corner cases like the above.

Personally I don't think it's worth going overboard in trying to solve
every potential case, just ones that are likely to come up in use,
because if we do want a really robust option parsing system we probably
should be looking at one of the several widely used option handling
libraries like popt or whatnot.

> I think we might have tests for the option parser. Would be good to add
> this particular corner-case as a test there. If we don't have tests,
> would be nice to have some.

Of course I'm a strong +1 for more tests, but again I think if we care
that much about making the option handling solid, we really ought first
do some due diligence looking at existing solutions.  This isn't an area
where we're value-adding much...

Either way we go, I'd be happy to integrate a replacement lib or improve
our existing solution, although I have some other matters that would be
higher priority in the near term.

Bryce

> Thanks,
> pq
> 
> > Le 12/02/2016 00:25, Bryce Harrington a écrit :
> > > weston allows both short and long style options to take arguments.  In
> > > the case of short options, allow an optional space between the option
> > > name and value.  E.g., previously you could launch weston this way:
> > >
> > >    weston -i2 -cmyconfig.ini
> > >
> > > now you can also launch it like this:
> > >
> > >    weston -i 2 -c myconfig.ini
> > >
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > > ---
> > >   shared/option-parser.c | 41 ++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 38 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/shared/option-parser.c b/shared/option-parser.c
> > > index f1cc529..d5fee8e 100644
> > > --- a/shared/option-parser.c
> > > +++ b/shared/option-parser.c
> > > @@ -98,14 +98,37 @@ short_option(const struct weston_option *options, int count, char *arg)
> > >
> > >   				return 1;
> > >   			}
> > > -		} else {
> > > +		} else if (arg[2]) {
> > >   			return handle_option(options + k, arg + 2);
> > > +		} else {
> > > +			return 0;
> > >   		}
> > >   	}
> > >
> > >   	return 0;
> > >   }
> > >
> > > +static int
> > > +short_option_with_arg(const struct weston_option *options, int count, char *arg, char *param)
> > > +{
> > > +	int k;
> > > +
> > > +	if (!arg[1])
> > > +		return 0;
> > > +
> > > +	for (k = 0; k < count; k++) {
> > > +		if (options[k].short_name != arg[1])
> > > +			continue;
> > > +
> > > +		if (options[k].type == WESTON_OPTION_BOOLEAN)
> > > +			continue;
> > > +
> > > +		return handle_option(options + k, param);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   int
> > >   parse_options(const struct weston_option *options,
> > >   	      int count, int *argc, char *argv[])
> > > @@ -115,10 +138,22 @@ parse_options(const struct weston_option *options,
> > >   	for (i = 1, j = 1; i < *argc; i++) {
> > >   		if (argv[i][0] == '-') {
> > >   			if (argv[i][1] == '-') {
> > > +				/* Long option, e.g. --foo or --foo=bar */
> > >   				if (long_option(options, count, argv[i]))
> > >   					continue;
> > > -			} else if (short_option(options, count, argv[i]))
> > > -				continue;
> > > +
> > > +			} else {
> > > +				/* Short option, e.g -f or -f42 */
> > > +				if (short_option(options, count, argv[i]))
> > > +					continue;
> > > +
> > > +				/* ...also handle -f 42 */
> > > +				if (i+1 < *argc &&
> > > +				    short_option_with_arg(options, count, argv[i], argv[i+1])) {
> > > +					i++;
> > > +					continue;
> > > +				}
> > > +			}
> > >   		}
> > >   		argv[j++] = argv[i];
> > >   	}
> > >  
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBVw+KByNf5bQRqqqnAQgZzQ/5AVjf4VlKmzMXKn54foAVIL2uAVWn/v1z
> ZjQHHvHdmo3TbJE3c03MA/y/Unv6ok3w8RJcuxUElchysN3UKOFqifWqLmdGszPA
> T1j4Pv3Bz6ZK7ueDhdEdzrlcW/0l1ZIb2Q6+kXRjcuic04tp7A3dRT+w9EEwulBB
> fCx8eLb69ah2jvIhu4EAJD3mZSUjsqP6mWeLyQgCKWm8BBmzVwg1WLLy8v3gXXDY
> JM9ksQtKuEqyehVu/mkj5gLBMUICQZxZUWDHWWdlH1rji6r1GjW7OqdpGESxwODK
> g87yGx8gnZZMpSPcv+fAcbK+9l71p+8m3q2K6aP7vlCH/q1scye6PruV/cjdbD1c
> UkrJr5EEVOE9HTinXzYkeyQa1vmWzBoqT5551XqjaGmLJCPEVlGTrqJ5rwQ3CjPX
> 8qcaRZJ8ThkmLJBt8X+InqIMBrLSKwCpwaeAynIIPP6j1pXMyUZGR1d1C5B7/D/H
> N9PzggzXI0GkxkYVjj3Ogq3y2ao7Oc11Zem2rEiD9TIwFMEq3gQSeK1wd9zhznwx
> UWnWQ7bMm4c4+/QNmwitCEzCHK4AbVl+R2leo2zWv8bCIcc23eBiqSS708ZvV/sA
> ib2yGcB68EmW7a24NRa/gn1LFVs2HzqWJkBjHDkjkBC2CuyNVB+mITCJlQ2+2fwW
> m+mN+s0pWao=
> =A7eg
> -----END PGP SIGNATURE-----