On Thu, 14 Apr 2016 11:10:57 -0700 Bryce Harrington <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > --- > > > > 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 > > > [email protected] > > > 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----- >
pgpn9NYxgw4r1.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
