[libvirt] [PATCH 2/2] virsh: fix regression in parsing optional integer

[Eric Blake]

On 04/15/2011 03:35 PM, Jiri Denemark wrote:
> On Tue, Apr 12, 2011 at 15:35:07 -0600, Eric Blake wrote:
>> Regression introduced in 0.8.5, commit c1564268.  The command
>> 'virsh freecell 0' quit working when it changed from an optional
>> string to an optional integer.
>>
>> This patch introduces a slight change that specifying an option
>> twice is now detected as an error.
>>
>> * tools/virsh.c (vshCmddefGetData, vshCmddefGetOption)
>> (vshCommandCheckOpts): Alter parameters to use bitmaps.
>> (vshCmddefOptParse): New function.
>> (vshCommandParse): Update for better handling of positional
>> arguments.
>> (vshCmddefHelp): Allow unit tests to validate options.
>> ---
>>  tools/virsh.c |  149 +++++++++++++++++++++++++++++++++++++++-----------------
>>  1 files changed, 104 insertions(+), 45 deletions(-)
> 
> 100iI hate command line parsing in virsh.
> ^[

Me too.  But it should be a lot better now.

> 
> The code looks like it does what it's supposed to do and I guess we should be
> fine with the limit for 32 arguments for a single virsh command :-) If not,
> there's clearly something wrong about the command which would need more.
> 
> ACK

I tweaked the commit message, then pushed:

commit b9973f526cb12b8e3d751ed19fb071b4a54ea1c0
Author: Eric Blake <eblake at redhat.com>
Date:   Tue Apr 12 14:42:59 2011 -0600

    virsh: fix regression in parsing optional integer

    Regression introduced in 0.8.5, commit c1564268.  The command
    'virsh freecell 0' quit working when it changed from an optional
    string to an optional integer.

    This patch introduces a slight change that specifying an option
    twice is now detected as an error.  It also changes things so
    that a command that has more than 1 required option will not
    complain about missing options if one but not all of the options
    were given in long format, as in 'virsh vol-create --pool p file',
    as well as making positional parsing work for all optional
    options (each positional argument is associated with the earliest
    option that has not yet been seen by name).

    Optional boolean options can appear before required argument
    options, because they don't affect positional argument parsing,
    and obviously a required boolean option makes no sense.

    Technically, this patch renders VSH_OT_STRING and VSH_OT_DATA
    redundant; but cleaning that up can be a separate patch.

    No command should ever need more than 32 options, right? :)

    * tools/virsh.c (vshCmddefGetData, vshCmddefGetOption)
    (vshCommandCheckOpts): Alter parameters to use bitmaps.
    (vshCmddefOptParse): New function.
    (vshCommandParse): Update for better handling of positional
    arguments.
    (vshCmddefHelp): Allow unit tests to validate options.


-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110415/3290357a/attachment-0001.sig>