[libvirt] [PATCH] virsh: Fix a problem of argv parsing
Problem example: # virsh -d 5 vol-create --pool default col.xml vol-create: pool(optdata): default vol-create: pool(optdata): col.xml error: command 'vol-create' requires <file> option It gets same "vshCmdOptDef" for both "--pool default" and "col.xml". This patch fixes it by increase "data_ct" when things like "--pool default" is successfully parsed, so that could get right "vshCmdOptDef" for the other arguments which are not with option name together. --- tools/virsh.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 19e3449..4d52bfb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11750,6 +11750,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) VSH_OT_INT ? _("number") : _("string")); goto syntaxError; } + + data_ct++; } else { tkdata = NULL; if (optstr) { -- 1.7.4
On 04/05/2011 06:38 AM, Osier Yang wrote:
Problem example: # virsh -d 5 vol-create --pool default col.xml vol-create: pool(optdata): default vol-create: pool(optdata): col.xml error: command 'vol-create' requires <file> option
It gets same "vshCmdOptDef" for both "--pool default" and "col.xml".
This patch fixes it by increase "data_ct" when things like "--pool default" is successfully parsed, so that could get right "vshCmdOptDef" for the other arguments which are not with option name together.
While I agree that this patch appears to fix the problem, I'd feel much better if we _also_ added a test case to prove we don't regress in the future (especially since we might be making future changes to argument parsing to improve tab-completion or unambiguous prefix support). It looks like the following are impacted (at least these are the commands with more than one VSH_OT_DATA/VSH_OFLAG_REQ option): domblkstat domifstat domblkinfo save dump vcpupin setvcpus setmem setmaxmem domxml-from-native domxml-to-native migrate migrate-setmaxdowntime pool-define-as pool-create-as vol-create-as vol-create vol-create-from vol-clone vol-upload vol-download secret-set-value attach-device detach-device update-device attach-interface detach-interface attach-disk detach-disk snapshot-dumpxml snapshot-revert snapshot-delete qemu-monitor-command And since test:///default supports setvcpus, a valid test might be to copy the layout of virsh-schedinfo as framework, and test that all of these are equivalent: virsh -c test:///default setvcpus test 2 virsh -c test:///default setvcpus --domain test 2 virsh -c test:///default setvcpus --domain=test 2 virsh -c test:///default setvcpus test --count 2 virsh -c test:///default setvcpus test --count=2 virsh -c test:///default setvcpus --domain test --count 2 virsh -c test:///default setvcpus --domain=test --count 2 virsh -c test:///default setvcpus --domain test --count=2 virsh -c test:///default setvcpus --domain=test --count=2 virsh -c test:///default setvcpus --count 2 --domain test virsh -c test:///default setvcpus --count 2 --domain=test virsh -c test:///default setvcpus --count=2 --domain test virsh -c test:///default setvcpus --count=2 --domain=test -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/05/2011 06:38 AM, Osier Yang wrote:
Problem example: # virsh -d 5 vol-create --pool default col.xml vol-create: pool(optdata): default vol-create: pool(optdata): col.xml error: command 'vol-create' requires <file> option
It gets same "vshCmdOptDef" for both "--pool default" and "col.xml".
This patch fixes it by increase "data_ct" when things like "--pool default" is successfully parsed, so that could get right "vshCmdOptDef" for the other arguments which are not with option name together. --- tools/virsh.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
NAK to this patch. It still doesn't detect other issues with argument parsing, such as 'virsh vol-create --pool default --pool other col.xml'. Typically, option parsing should favor the last instance of a given option, rather than the first, or error out that the option is given too many times, but your patch doesn't change the status quo of going with the first instance of the option. Instead, I've prepared a better patch that not only fixes this issue, but also addresses a regression I introduced in 0.8.5, where 'virsh freecell 0' no longer parses correctly. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
I wish I could have come up with something with fewer lines of code changed; but I'm at least happy that the end result only touched vshCommandParse and its helper functions rather than having to touch lots of existing commands. This fixes multiple bugs: a regression in 'virsh freecell 0' parsing, as well as an infelicity in 'virsh vol-create --pool default vol.xml' handling. It also makes it so that 'virsh vol-info vol-name pool-name' works without having to specify an explicit --pool argument. Eric Blake (2): virsh: list required options first virsh: fix regression in parsing optional integer tools/virsh.c | 169 ++++++++++++++++++++++++++++++++++++++------------------- 1 files changed, 114 insertions(+), 55 deletions(-)
* tests/virsh-optparse: New file. * tests/Makefile.am (test_scripts): Use it. --- Hmm, I'd better take my own advice and test this stuff :) tests/Makefile.am | 1 + tests/virsh-optparse | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 0 deletions(-) create mode 100755 tests/virsh-optparse diff --git a/tests/Makefile.am b/tests/Makefile.am index 5896442..3fa708c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ test_scripts += \ undefine \ vcpupin \ virsh-all \ + virsh-optparse \ virsh-schedinfo \ virsh-synopsis endif diff --git a/tests/virsh-optparse b/tests/virsh-optparse new file mode 100755 index 0000000..5fe5097 --- /dev/null +++ b/tests/virsh-optparse @@ -0,0 +1,70 @@ +#!/bin/sh +# Ensure that virsh option parsing doesn't regress + +# Copyright (C) 2011 Red Hat, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 2 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +: ${srcdir=$(pwd)} +: ${abs_top_srcdir=$(pwd)/..} +: ${abs_top_builddir=$(pwd)/..} + +# If $abs_top_builddir/tools is not early in $PATH, put it there, +# so that we can safely invoke "virsh" simply with its name. +case $PATH in + $abs_top_builddir/tools/src:$abs_top_builddir/tools:*) ;; + $abs_top_builddir/tools:*) ;; + *) PATH=$abs_top_builddir/tools:$PATH; export PATH ;; +esac + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +. "$srcdir/test-lib.sh" + +cat <<\EOF > exp-out || framework_failure + +setvcpus: <domain> trying as domain NAME +setvcpus: count(optdata): 2 +setvcpus: domain(optdata): test +setvcpus: found option <domain>: test +EOF + +fail=0 + +test_url=test:///default + +for args in \ + 'test 2' \ + '--domain test 2' \ + '--domain=test 2' \ + 'test --count 2' \ + 'test --count=2' \ + '--domain test --count 2' \ + '--domain=test --count 2' \ + '--domain test --count=2' \ + '--domain=test --count=2' \ + '--count 2 --domain test' \ + '--count 2 --domain=test' \ + '--count=2 --domain test' \ + '--count=2 --domain=test' \ +; do + virsh -d5 -c $test_url setvcpus $args >out 2>>err || fail=1 + LC_ALL=C sort out | compare - exp-out || fail=1 +done +test -s err && fail=1 + +(exit $fail); exit $fail -- 1.7.1
On Tue, Apr 12, 2011 at 16:01:00 -0600, Eric Blake wrote:
* tests/virsh-optparse: New file. * tests/Makefile.am (test_scripts): Use it. ---
Hmm, I'd better take my own advice and test this stuff :)
tests/Makefile.am | 1 + tests/virsh-optparse | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 0 deletions(-) create mode 100755 tests/virsh-optparse
And you also introduce this test for option parsing, nice. ACK Jirka
On 04/15/2011 03:37 PM, Jiri Denemark wrote:
On Tue, Apr 12, 2011 at 16:01:00 -0600, Eric Blake wrote:
* tests/virsh-optparse: New file. * tests/Makefile.am (test_scripts): Use it. ---
Hmm, I'd better take my own advice and test this stuff :)
tests/Makefile.am | 1 + tests/virsh-optparse | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 0 deletions(-) create mode 100755 tests/virsh-optparse
And you also introduce this test for option parsing, nice.
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The current state of virsh parsing is that: all lookup the volume by path (technically, the last two also attempt a name lookup within a pool, whereas the first skips that step, but the end result is the same); meanwhile: complains about unexpected data. Why? Because the --pool option is optional, so default was parsed as the --vol argument, and /path/to/image.img doesn't match up with any remaining options that require an argument. Therefore, the only way to specify pool is with an explicit "--pool" argument (you can't specify it positionally). However, named arguments can appear in any order, so: have also always worked. Therefore, this patch has no functional change on vol-info option parsing, but only on 'virsh help vol-info' synopsis layout. However, it also allows the next patch to 1) enforce that required options are always first (without this patch, the next patch would fail the testsuite), and 2) allow the user to omit the "--pool" argument. That is, the next patch makes it possible to do: instead of the longer * tools/virsh.c (opts_vol_create_from, opts_vol_clone) (opts_vol_upload, opts_vol_download, opts_vol_delete) (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) (opts_vol_path): List optional pool parameter after required arguments. --- tools/virsh.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2e35021..cd1d246 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6963,8 +6963,8 @@ static const vshCmdInfo info_vol_create_from[] = { static const vshCmdOptDef opts_vol_create_from[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML vol description")}, - {"inputpool", VSH_OT_STRING, 0, N_("pool name or uuid of the input volume's pool")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("input vol name or key")}, + {"inputpool", VSH_OT_STRING, 0, N_("pool name or uuid of the input volume's pool")}, {NULL, 0, 0, NULL} }; @@ -7059,9 +7059,9 @@ static const vshCmdInfo info_vol_clone[] = { }; static const vshCmdOptDef opts_vol_clone[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("orig vol name or key")}, {"newname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("clone name")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7136,9 +7136,9 @@ static const vshCmdInfo info_vol_upload[] = { }; static const vshCmdOptDef opts_vol_upload[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, {"length", VSH_OT_INT, 0, N_("amount of data to upload") }, {NULL, 0, 0, NULL} @@ -7236,9 +7236,9 @@ static const vshCmdInfo info_vol_download[] = { }; static const vshCmdOptDef opts_vol_download[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"offset", VSH_OT_INT, 0, N_("volume offset to download from") }, {"length", VSH_OT_INT, 0, N_("amount of data to download") }, {NULL, 0, 0, NULL} @@ -7342,8 +7342,8 @@ static const vshCmdInfo info_vol_delete[] = { }; static const vshCmdOptDef opts_vol_delete[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7383,8 +7383,8 @@ static const vshCmdInfo info_vol_wipe[] = { }; static const vshCmdOptDef opts_vol_wipe[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7424,8 +7424,8 @@ static const vshCmdInfo info_vol_info[] = { }; static const vshCmdOptDef opts_vol_info[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7475,8 +7475,8 @@ static const vshCmdInfo info_vol_dumpxml[] = { }; static const vshCmdOptDef opts_vol_dumpxml[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7888,8 +7888,8 @@ static const vshCmdInfo info_vol_key[] = { }; static const vshCmdOptDef opts_vol_key[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("volume name or path")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; @@ -7921,8 +7921,8 @@ static const vshCmdInfo info_vol_path[] = { }; static const vshCmdOptDef opts_vol_path[] = { - {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("volume name or key")}, + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {NULL, 0, 0, NULL} }; -- 1.7.1
On Tue, Apr 12, 2011 at 15:35:06 -0600, Eric Blake wrote:
The current state of virsh parsing is that:
all lookup the volume by path (technically, the last two also attempt a name lookup within a pool, whereas the first skips that step, but the end result is the same); meanwhile:
Is example virsh command line missing here?
complains about unexpected data. Why? Because the --pool option is optional, so default was parsed as the --vol argument, and /path/to/image.img doesn't match up with any remaining options that require an argument. Therefore, the only way to specify pool is with an explicit "--pool" argument (you can't specify it positionally). However, named arguments can appear in any order, so:
and here
have also always worked. Therefore, this patch has no functional change on vol-info option parsing, but only on 'virsh help vol-info' synopsis layout. However, it also allows the next patch to 1) enforce that required options are always first (without this patch, the next patch would fail the testsuite), and 2) allow the user to omit the "--pool" argument. That is, the next patch makes it possible to do:
and here
instead of the longer
and here as well.
* tools/virsh.c (opts_vol_create_from, opts_vol_clone) (opts_vol_upload, opts_vol_download, opts_vol_delete) (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) (opts_vol_path): List optional pool parameter after required arguments. --- tools/virsh.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
Makes sense. I went through all virsh.c and didn't find more options that would need fixing. The only options which don't follow this "optional after required" rule are bool options which can safely stay where they are. ACK Jirka
On 04/15/2011 02:26 PM, Jiri Denemark wrote:
On Tue, Apr 12, 2011 at 15:35:06 -0600, Eric Blake wrote:
The current state of virsh parsing is that:
all lookup the volume by path (technically, the last two also attempt a name lookup within a pool, whereas the first skips that step, but the end result is the same); meanwhile:
Is example virsh command line missing here?
Aargh. I wrote my commit message by using sample command prompts: # virsh ... But # is a comment character, and ate my message (and now I don't remember it off the top of my head). I'll have to figure that out again, and use $ prompts instead...
* tools/virsh.c (opts_vol_create_from, opts_vol_clone) (opts_vol_upload, opts_vol_download, opts_vol_delete) (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) (opts_vol_path): List optional pool parameter after required arguments. --- tools/virsh.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
Makes sense. I went through all virsh.c and didn't find more options that would need fixing. The only options which don't follow this "optional after required" rule are bool options which can safely stay where they are.
ACK
The review would have been much easier with a non-broken commit message, but you caught on to my drift. I'll post my revised commit message once I remember what it was... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/15/2011 02:33 PM, Eric Blake wrote:
Is example virsh command line missing here?
Aargh. I wrote my commit message by using sample command prompts:
# virsh ...
But # is a comment character, and ate my message (and now I don't remember it off the top of my head). I'll have to figure that out again, and use $ prompts instead...
ACK
The review would have been much easier with a non-broken commit message, but you caught on to my drift. I'll post my revised commit message once I remember what it was...
Pushed with the following commit message: commit 6b75a1a5b0d10b42e3fd344b2067a176ee294f46 Author: Eric Blake <eblake@redhat.com> Date: Tue Apr 12 14:58:02 2011 -0600 virsh: list required options first The current state of virsh parsing is that: $ virsh vol-info /path/to/image $ virsh vol-info --pool default /path/to/image $ virsh vol-info --pool default --vol /path/to/image all lookup the volume by path (technically, the last two also attempt a name lookup within a pool, whereas the first skips that step, but the end result is the same); meanwhile: $ virsh vol-info default /path/to/image complains about unexpected data. Why? Because the --pool option is optional, so default was parsed as the --vol argument, and /path/to/image.img doesn't match up with any remaining options that require an argument. For proof, note that: $ virsh vol-info default --vol /path/to/image complains about looking up 'default' - the parser mis-associated both arguments with --vol. Given the above, the only way to specify pool is with an explicit "--pool" argument (you can't specify it positionally). However, named arguments can appear in any order, so: $ virsh vol-info /path/to/image --pool default $ virsh vol-info --vol /path/to/image --pool default have also always worked. Therefore, this patch has no functional change on vol-info option parsing, but only on 'virsh help vol-info' synopsis layout. However, it also allows the next patch to 1) enforce that required options are always first (without this patch, the next patch would fail the testsuite), and 2) allow the user to omit the "--pool" argument. That is, the next patch makes it possible to do: $ virsh vol-info /path/to/image default which to date was not possible. * tools/virsh.c (opts_vol_create_from, opts_vol_clone) (opts_vol_upload, opts_vol_download, opts_vol_delete) (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) (opts_vol_path): List optional pool parameter after required arguments. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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(-) diff --git a/tools/virsh.c b/tools/virsh.c index cd1d246..486442e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -57,6 +57,7 @@ #include "configmake.h" #include "threads.h" #include "command.h" +#include "count-one-bits.h" static char *progname; @@ -10930,66 +10931,107 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name) return NULL; } +static int +vshCmddefOptParse(const vshCmdDef *cmd, uint32_t* opts_need_arg, + uint32_t *opts_required) +{ + int i; + bool optional = false; + + if (!cmd->opts) + return 0; + + for (i = 0; cmd->opts[i].name; i++) { + const vshCmdOptDef *opt = &cmd->opts[i]; + + if (i > 31) + return -1; /* too many options */ + if (opt->type == VSH_OT_BOOL) { + if (opt->flag & VSH_OFLAG_REQ) + return -1; /* bool options can't be mandatory */ + continue; + } + *opts_need_arg |= 1 << i; + if (opt->flag & VSH_OFLAG_REQ) { + if (optional) + return -1; /* mandatory options must be listed first */ + *opts_required |= 1 << i; + } else { + optional = true; + } + } + return 0; +} + static const vshCmdOptDef * -vshCmddefGetOption(const vshCmdDef * cmd, const char *name) +vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, + uint32_t *opts_seen) { - const vshCmdOptDef *opt; + int i; - for (opt = cmd->opts; opt && opt->name; opt++) - if (STREQ(opt->name, name)) + for (i = 0; cmd->opts && cmd->opts[i].name; i++) { + const vshCmdOptDef *opt = &cmd->opts[i]; + + if (STREQ(opt->name, name)) { + if (*opts_seen & (1 << i)) { + vshError(ctl, _("option --%s already seen"), name); + return NULL; + } + *opts_seen |= 1 << i; return opt; + } + } + + vshError(ctl, _("command '%s' doesn't support option --%s"), + cmd->name, name); return NULL; } static const vshCmdOptDef * -vshCmddefGetData(const vshCmdDef * cmd, int data_ct) +vshCmddefGetData(const vshCmdDef *cmd, uint32_t *opts_need_arg, + uint32_t *opts_seen) { + int i; const vshCmdOptDef *opt; - for (opt = cmd->opts; opt && opt->name; opt++) { - if (opt->type >= VSH_OT_DATA || - (opt->type == VSH_OT_INT && (opt->flag & VSH_OFLAG_REQ))) { - if (data_ct == 0 || opt->type == VSH_OT_ARGV) - return opt; - else - data_ct--; - } - } - return NULL; + if (!*opts_need_arg) + return NULL; + + /* Grab least-significant set bit */ + i = count_one_bits(*opts_need_arg ^ (*opts_need_arg - 1)) - 1; + opt = &cmd->opts[i]; + if (opt->type != VSH_OT_ARGV) + *opts_need_arg &= ~(1 << i); + *opts_seen |= 1 << i; + return opt; } /* * Checks for required options */ static int -vshCommandCheckOpts(vshControl *ctl, const vshCmd *cmd) +vshCommandCheckOpts(vshControl *ctl, const vshCmd *cmd, uint32_t opts_required, + uint32_t opts_seen) { const vshCmdDef *def = cmd->def; - const vshCmdOptDef *d; - int err = 0; - - for (d = def->opts; d && d->name; d++) { - if (d->flag & VSH_OFLAG_REQ) { - vshCmdOpt *o = cmd->opts; - int ok = 0; - - while (o && ok == 0) { - if (o->def == d) - ok = 1; - o = o->next; - } - if (!ok) { - vshError(ctl, - d->type == VSH_OT_DATA ? - _("command '%s' requires <%s> option") : - _("command '%s' requires --%s option"), - def->name, d->name); - err = 1; - } + int i; + + opts_required &= ~opts_seen; + if (!opts_required) + return 0; + for (i = 0; def->opts[i].name; i++) { + if (opts_required & (1 << i)) { + const vshCmdOptDef *opt = &def->opts[i]; + + vshError(ctl, + opt->type == VSH_OT_DATA ? + _("command '%s' requires <%s> option") : + _("command '%s' requires --%s option"), + def->name, opt->name); } } - return !err; + return -1; } static const vshCmdDef * @@ -11055,6 +11097,14 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) const char *desc = _(vshCmddefGetInfo(def, "desc")); const char *help = _(vshCmddefGetInfo(def, "help")); char buf[256]; + uint32_t opts_need_arg; + uint32_t opts_required; + + if (vshCmddefOptParse(def, &opts_need_arg, &opts_required)) { + vshError(ctl, _("internal error: bad options in command: '%s'"), + def->name); + return FALSE; + } fputs(_(" NAME\n"), stdout); fprintf(stdout, " %s - %s\n", def->name, help); @@ -11731,7 +11781,9 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) const vshCmdDef *cmd = NULL; vshCommandToken tk; bool data_only = false; - int data_ct = 0; + uint32_t opts_need_arg = 0; + uint32_t opts_required = 0; + uint32_t opts_seen = 0; first = NULL; @@ -11754,6 +11806,13 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshError(ctl, _("unknown command: '%s'"), tkdata); goto syntaxError; /* ... or ignore this command only? */ } + if (vshCmddefOptParse(cmd, &opts_need_arg, + &opts_required) < 0) { + vshError(ctl, + _("internal error: bad options in command: '%s'"), + tkdata); + goto syntaxError; + } VIR_FREE(tkdata); } else if (data_only) { goto get_data; @@ -11764,10 +11823,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) *optstr = '\0'; /* convert the '=' to '\0' */ optstr = vshStrdup(ctl, optstr + 1); } - if (!(opt = vshCmddefGetOption(cmd, tkdata + 2))) { - vshError(ctl, - _("command '%s' doesn't support option --%s"), - cmd->name, tkdata + 2); + if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2, + &opts_seen))) { VIR_FREE(optstr); goto syntaxError; } @@ -11789,6 +11846,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) VSH_OT_INT ? _("number") : _("string")); goto syntaxError; } + opts_need_arg &= ~opts_seen; } else { tkdata = NULL; if (optstr) { @@ -11804,7 +11862,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) continue; } else { get_data: - if (!(opt = vshCmddefGetData(cmd, data_ct++))) { + if (!(opt = vshCmddefGetData(cmd, &opts_need_arg, + &opts_seen))) { vshError(ctl, _("unexpected data '%s'"), tkdata); goto syntaxError; } @@ -11840,7 +11899,7 @@ get_data: c->def = cmd; c->next = NULL; - if (!vshCommandCheckOpts(ctl, c)) { + if (vshCommandCheckOpts(ctl, c, opts_required, opts_seen) < 0) { VIR_FREE(c); goto syntaxError; } -- 1.7.1
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. ^[ 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 Jirka
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@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Osier Yang