[PATCH] option-parser: Handle short double-arg options
Pekka Paalanen
ppaalanen at gmail.com
Fri Apr 15 08:55:57 UTC 2016
On Thu, 14 Apr 2016 11:10:57 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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.
>
Hi Bryce,
in that case, shouldn't we just revert this patch?
> > 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.
Personally I've been ok with what it was. I have forgotten why it
didn't originally just wrap getopt_long (a GNUism? too much effort?).
But you're right, it's a really unimportant thing, so I won't push it
in any direction myself. It just looked like you ignored a review
comment pointing out a new problem.
Changing the command line argument syntax might even be too disruptive
nowadays.
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-----
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160415/886c4e66/attachment-0001.sig>
More information about the wayland-devel
mailing list