7:38 a.m.
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
9:50 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:30 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:35 p.m.
New subject: [libvirt] [PATCH 0/2] virsh: improve option parsing, including a regression fix
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(-)
5:01 p.m.
New subject: [libvirt] [PATCH 3/2] tests: test recent virsh option parsing changes
* 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
4:37 p.m.
New subject: [libvirt] [PATCH 3/2] tests: test recent virsh option parsing changes
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
5:18 p.m.
New subject: [libvirt] [PATCH 3/2] tests: test recent virsh option parsing changes
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:35 p.m.
New subject: [libvirt] [PATCH 1/2] virsh: list required options first
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
3:26 p.m.
New subject: [libvirt] [PATCH 1/2] virsh: list required options first
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
3:33 p.m.
New subject: [libvirt] [PATCH 1/2] virsh: list required options first
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:55 p.m.
New subject: [libvirt] [PATCH 1/2] virsh: list required options first
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:35 p.m.
New subject: [libvirt] [PATCH 2/2] 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.
* 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
4:35 p.m.
New subject: [libvirt] [PATCH 2/2] virsh: fix regression in parsing optional integer
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
5:17 p.m.
New subject: [libvirt] [PATCH 2/2] virsh: fix regression in parsing optional integer
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4970
days inactive
4980
days old
13 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Osier Yang