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

[Pekka Paalanen]

On Fri, 15 Apr 2016 10:26:26 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Fri, Apr 15, 2016 at 11:55:57AM +0300, Pekka Paalanen wrote:
> > 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?  
> 
> This solved a problem that came up in actual use, while I was testing
> the idle inhibition functionality.  I did try to keep the patch as
> minimal as possible though.
>  
> > > > 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?).  
> 
> Or portability maybe?
> 
> getopt_long has a few quirks/limitations itself (for Inkscape we
> switched to popt).
> 
> > 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.  
> 
> Sorry.  We did actually exchange emails about it off-list.
> 
> > Changing the command line argument syntax might even be too disruptive
> > nowadays.  
> 
> Possibly, yeah, although some packages are pretty flexible and I would
> expect could replicate our syntax style adequately enough.  I agree that
> avoiding disruption would have to be a requirement in making any choice.
> 
> I suppose one thing to consider is that with the libweston backend
> configuration support going in, there might be more pressure for more
> featureful command line option functionality.  But that pressure will be
> pretty obvious if it comes.  

Except that all command line parsing and config file parsing will be
outside the scope of libweston. None of it will be in libweston, so it
will all stay specific to weston itself.


Thanks,
pq
-------------- 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/20160418/90756c90/attachment.sig>