[libvirt] [PATCH] virsh: fix regression in argv parsing

[Eric Blake]

On 09/21/2011 08:54 AM, Eric Blake wrote:
> Commit 85d2810 broke commands with mandatory argv (send-key,
> qemu-monitor-command).  This fixes things, and enhances the
> test to cover it.

To make review easier, I'll enhance the commit message:

Prior to commit 85d2810, we had an issue where:

snapshot-create-as dom name --diskspec spec --diskspec spec

failed to parse the second spec, because the first spec had marked that 
option as no longer requiring an argument.

In commit 85d2810, I fixed it by making argv options no longer mark the 
option as seen.  But this in turn breaks mandatory argv options, which 
now complain that the argv option is missing.

This patch reverts that part of 85d2810, and instead replaces it with 
fixes to no longer clear opts_need_arg of an argv argument.

>
> * tools/virsh.c (vshCmddefGetOption, vshCmddefGetData)
> (vshCommandParse): Fix option parsing for required argv option.
> (vshCmddefOptParse): Check that argv option is last.
> * tests/virsh-optparse: Enhance test.
> ---
>   tests/virsh-optparse |    9 +++++++++
>   tools/virsh.c        |   24 +++++++++++++++---------
>   2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/tests/virsh-optparse b/tests/virsh-optparse
> index 18252d2..d0d4329 100755
> --- a/tests/virsh-optparse
> +++ b/tests/virsh-optparse
> @@ -118,6 +118,7 @@ for args in \
>       'test name desc vda vdb' \
>       'test --diskspec vda name --diskspec vdb desc' \
>       '--description desc --name name --domain test vda vdb' \
> +    '--description desc --diskspec vda --name name --domain test vdb' \
>   ; do
>     virsh -q -c $test_url snapshot-create-as --print-xml $args \
>       >out 2>>err || fail=1
> @@ -126,4 +127,12 @@ done
>
>   test -s err&&  fail=1
>
> +# Test a required argv
> +cat<<\EOF>  exp-err || framework_failure
> +error: this function is not supported by the connection driver: virDomainQemuMonitorCommand
> +EOF
> +virsh -q -c $test_url qemu-monitor-command test a>out 2>err&&  fail=1
> +test -s out&&  fail=1
> +compare err exp-err || fail=1
> +
>   (exit $fail); exit $fail
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 371346a..0b187bc 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -13834,6 +13834,7 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name)
>       return NULL;
>   }
>
> +/* Validate that the options associated with cmd can be parsed.  */
>   static int
>   vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg,
>                     uint32_t *opts_required)
> @@ -13871,13 +13872,16 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg,
>           } else {
>               optional = true;
>           }
> +
> +        if (opt->type == VSH_OT_ARGV&&  cmd->opts[i + 1].name)
> +            return -1; /* argv option must be listed last */
>       }
>       return 0;
>   }
>
>   static const vshCmdOptDef *
>   vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name,
> -                   uint32_t *opts_seen)
> +                   uint32_t *opts_seen, int *opt_index)
>   {
>       int i;
>
> @@ -13885,12 +13889,12 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name,
>           const vshCmdOptDef *opt =&cmd->opts[i];
>
>           if (STREQ(opt->name, name)) {
> -            if (*opts_seen&  (1<<  i)) {
> +            if ((*opts_seen&  (1<<  i))&&  opt->type != VSH_OT_ARGV) {
>                   vshError(ctl, _("option --%s already seen"), name);
>                   return NULL;
>               }
> -            if (opt->type != VSH_OT_ARGV)
> -                *opts_seen |= 1<<  i;
> +            *opts_seen |= 1<<  i;
> +            *opt_index = i;
>               return opt;
>           }
>       }
> @@ -13913,10 +13917,9 @@ vshCmddefGetData(const vshCmdDef *cmd, uint32_t *opts_need_arg,
>       /* Grab least-significant set bit */
>       i = ffs(*opts_need_arg) - 1;
>       opt =&cmd->opts[i];
> -    if (opt->type != VSH_OT_ARGV) {
> +    if (opt->type != VSH_OT_ARGV)
>           *opts_need_arg&= ~(1<<  i);
> -        *opts_seen |= 1<<  i;
> -    }
> +    *opts_seen |= 1<<  i;
>       return opt;
>   }
>
> @@ -14878,12 +14881,14 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
>               } else if (tkdata[0] == '-'&&  tkdata[1] == '-'&&
>                          c_isalnum(tkdata[2])) {
>                   char *optstr = strchr(tkdata + 2, '=');
> +                int opt_index;
> +
>                   if (optstr) {
>                       *optstr = '\0'; /* convert the '=' to '\0' */
>                       optstr = vshStrdup(ctl, optstr + 1);
>                   }
>                   if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2,
> -&opts_seen))) {
> +&opts_seen,&opt_index))) {
>                       VIR_FREE(optstr);
>                       goto syntaxError;
>                   }
> @@ -14905,7 +14910,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
>                                    VSH_OT_INT ? _("number") : _("string"));
>                           goto syntaxError;
>                       }
> -                    opts_need_arg&= ~opts_seen;
> +                    if (opt->type != VSH_OT_ARGV)
> +                        opts_need_arg&= ~(1<<  opt_index);
>                   } else {
>                       tkdata = NULL;
>                       if (optstr) {

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