1:06 p.m.
* tools/virsh.c: Using VSH_OT_STRING instead of VSH_OT_ARGV for diskspec
* tools/virsh.pod: Fix missing -- tag for snapshot-create-as in docs
---
tools/virsh.c | 2 +-
tools/virsh.pod | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 3c6e65a..5c4610e 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -12630,7 +12630,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
{"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no
metadata")},
{"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is
created")},
{"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm
state")},
- {"diskspec", VSH_OT_ARGV, 0,
+ {"diskspec", VSH_OT_STRING, 0,
N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")},
{NULL, 0, 0, NULL}
};
diff --git a/tools/virsh.pod b/tools/virsh.pod
index e82567d..c82cc45 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1773,7 +1773,7 @@ by command such as B<destroy> or by internal guest action).
=item B<snapshot-create-as> I<domain> {[I<--print-xml>]
| [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>]
-[I<--disk-only> [I<diskspec>]...]
+[I<--disk-only>] [I<--diskspec>]...
Create a snapshot for domain I<domain> with the given <name> and
<description>; if either value is omitted, libvirt will choose a
@@ -1784,8 +1784,8 @@ inactive state after the snapshot is created, and if
I<--disk-only>
is specified, the snapshot will not include vm state.
The I<--disk-only> flag is used to request a disk-only snapshot. When
-this flag is in use, the command can also take additional I<diskspec>
-arguments to add <disk> elements to the xml. Each <diskspec> is in the
+this flag is in use, the command can also take additional I<--diskspec>
+arguments to add <disk> elements to the xml. Each I<--diskspec> is in the
form B<disk[,snapshot=type][,driver=type][,file=name]>. To include a
literal comma in B<disk> or in B<file=name>, escape it with a second
comma. For example, a diskspec of "vda,snapshot=external,file=/path/to,,new"
--
1.7.4.4
1:14 p.m.
On 09/14/2011 12:06 PM, Nan Zhang wrote:
* tools/virsh.c: Using VSH_OT_STRING instead of VSH_OT_ARGV for
diskspec
Nack. This MUST be VSH_OT_ARGV, in order to allow multiple diskspec.
* tools/virsh.pod: Fix missing -- tag for snapshot-create-as in docs
Nack. This is intentional - if all earlier options with arguments are
provided, then you can write 'snapshot-create-as dom name description
vda' instead of the identical 'snapashot-create-as dom name description
--diskspec vda'
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:16 p.m.
On 09/14/2011 12:14 PM, Eric Blake wrote:
On 09/14/2011 12:06 PM, Nan Zhang wrote:
> * tools/virsh.c: Using VSH_OT_STRING instead of VSH_OT_ARGV for diskspec
Nack. This MUST be VSH_OT_ARGV, in order to allow multiple diskspec.
> * tools/virsh.pod: Fix missing -- tag for snapshot-create-as in docs
Nack. This is intentional - if all earlier options with arguments are
provided, then you can write 'snapshot-create-as dom name description
vda' instead of the identical 'snapashot-create-as dom name description
--diskspec vda'
That said, I agree that the man page does look a bit confusing, and
perhaps we could go with:
=item B<snapshot-create-as> I<domain> {[I<--print-xml>]
| [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>]
-[I<--disk-only> [I<diskspec>]...]
+[I<--disk-only>] [[I<--diskspec>] B<diskspec>]...
to make it obvious that B<diskspec> is a literal term, optionally
preceded by a literal --diskspec.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:41 p.m.
On 09/15/2011 02:16 AM, Eric Blake wrote:
On 09/14/2011 12:14 PM, Eric Blake wrote:
> On 09/14/2011 12:06 PM, Nan Zhang wrote:
>> * tools/virsh.c: Using VSH_OT_STRING instead of VSH_OT_ARGV for
>> diskspec
>
> Nack. This MUST be VSH_OT_ARGV, in order to allow multiple diskspec.
>
>> * tools/virsh.pod: Fix missing -- tag for snapshot-create-as in docs
>
> Nack. This is intentional - if all earlier options with arguments are
> provided, then you can write 'snapshot-create-as dom name description
> vda' instead of the identical 'snapashot-create-as dom name description
> --diskspec vda'
That said, I agree that the man page does look a bit confusing, and
perhaps we could go with:
=item B<snapshot-create-as> I<domain> {[I<--print-xml>]
| [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>]
-[I<--disk-only> [I<diskspec>]...]
+[I<--disk-only>] [[I<--diskspec>] B<diskspec>]...
to make it obvious that B<diskspec> is a literal term, optionally
preceded by a literal --diskspec.
OK, will re-send a new patch for this.
Thanks :-)
2:53 p.m.
New subject: [libvirt] [PATCH] Clarify the usage of diskspec option in doc
* tools/virsh.pod: Make clear for the usage of this option <diskspec>
---
tools/virsh.pod | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index e82567d..694b3cc 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1773,7 +1773,7 @@ by command such as B<destroy> or by internal guest action).
=item B<snapshot-create-as> I<domain> {[I<--print-xml>]
| [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>]
-[I<--disk-only> [I<diskspec>]...]
+[I<--disk-only>] [[I<--diskspec>] B<diskspec>]...
Create a snapshot for domain I<domain> with the given <name> and
<description>; if either value is omitted, libvirt will choose a
--
1.7.4.4
4:27 p.m.
New subject: [libvirt] [PATCH] snapshot: tweak snapshot-create-as diskspec docs
With this patch, it is hopefully a bit more obvious that for
snapshot-create-as, a literal '--diskspec' is mandatory if name
or description was omitted, but optional if all earlier options
were provided.
These all denote two diskspecs and a description:
virsh snapshot-create-as dom name desc vda vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda vdb
virsh snapshot-create-as dom name desc vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
This gives two diskspecs but no description:
virsh snapshot-create-as dom name --diskspec vda --diskspec vdb
And this treats 'vda' as the description, with only one diskspec:
virsh snapshot-create-as dom name vda vdb
The help output now shows:
snapshot-create-as <domain> [<name>] [<description>] [--print-xml]
[--no-metadata] [--halt] [--disk-only] [[--diskspec] <string>]...
I also checked the help output for echo and send-key, which are two
other variants of argv commands.
* tools/virsh.pod (snapshot-create-as): Document when a literal
--diskspec must preceed a diskspec argument.
* tools/virsh.c (vshCmddefHelp): Update help output for argv when
naming the option is useful.
(vshCmddefGetData): Fix logic on when argv was seen.
Reported by Nan Zhang.
---
I think this is more comprehensive than Nan's patch, for nicer results.
tools/virsh.c | 27 +++++++++++++++++++++------
tools/virsh.pod | 6 ++++--
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 3c6e65a..fdb503a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -13856,9 +13856,10 @@ 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;
}
@@ -13956,6 +13957,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
char buf[256];
uint32_t opts_need_arg;
uint32_t opts_required;
+ bool shortopt = false; /* true if 'arg' works instead of '--opt
arg' */
if (vshCmddefOptParse(def, &opts_need_arg, &opts_required)) {
vshError(ctl, _("internal error: bad options in command:
'%s'"),
@@ -13980,18 +13982,30 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
/* xgettext:c-format */
fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>"
: _("[--%s <number>]"));
+ if (!(opt->flags & VSH_OFLAG_REQ_OPT))
+ shortopt = true;
break;
case VSH_OT_STRING:
/* xgettext:c-format */
fmt = _("[--%s <string>]");
+ if (!(opt->flags & VSH_OFLAG_REQ_OPT))
+ shortopt = true;
break;
case VSH_OT_DATA:
fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>" :
"[<%s>]");
+ if (!(opt->flags & VSH_OFLAG_REQ_OPT))
+ shortopt = true;
break;
case VSH_OT_ARGV:
/* xgettext:c-format */
- fmt = (opt->flags & VSH_OFLAG_REQ) ?
_("<%s>...")
- : _("[<%s>]...");
+ if (shortopt) {
+ fmt = (opt->flags & VSH_OFLAG_REQ)
+ ? _("{[--%s] <string>}...")
+ : _("[[--%s] <string>]...");
+ } else {
+ fmt = (opt->flags & VSH_OFLAG_REQ) ?
_("<%s>...")
+ : _("[<%s>]...");
+ }
break;
default:
assert(0);
@@ -14030,8 +14044,9 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
opt->name);
break;
case VSH_OT_ARGV:
- /* Not really an option. */
- snprintf(buf, sizeof(buf), _("<%s>"), opt->name);
+ snprintf(buf, sizeof(buf),
+ shortopt ? _("[--%s] <string>") :
_("<%s>"),
+ opt->name);
break;
default:
assert(0);
diff --git a/tools/virsh.pod b/tools/virsh.pod
index e82567d..126ace6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1773,7 +1773,7 @@ by command such as B<destroy> or by internal guest action).
=item B<snapshot-create-as> I<domain> {[I<--print-xml>]
| [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>]
-[I<--disk-only> [I<diskspec>]...]
+[I<--disk-only> [[I<--diskspec>] B<diskspec>]...
Create a snapshot for domain I<domain> with the given <name> and
<description>; if either value is omitted, libvirt will choose a
@@ -1788,7 +1788,9 @@ this flag is in use, the command can also take additional
I<diskspec>
arguments to add <disk> elements to the xml. Each <diskspec> is in the
form B<disk[,snapshot=type][,driver=type][,file=name]>. To include a
literal comma in B<disk> or in B<file=name>, escape it with a second
-comma. For example, a diskspec of "vda,snapshot=external,file=/path/to,,new"
+comma. A literal I<--diskspec> must preceed each B<diskspec> unless
+all three of I<domain>, I<name>, and I<description> are also present.
+For example, a diskspec of "vda,snapshot=external,file=/path/to,,new"
results in the following XML:
<disk name='vda' snapshot='external'>
<source file='/path/to,new'/>
--
1.7.4.4
4:50 p.m.
New subject: [libvirt] [PATCH] snapshot: tweak snapshot-create-as diskspec docs
On 09/14/2011 03:27 PM, Eric Blake wrote:
With this patch, it is hopefully a bit more obvious that for
snapshot-create-as, a literal '--diskspec' is mandatory if name
or description was omitted, but optional if all earlier options
were provided.
These all denote two diskspecs and a description:
virsh snapshot-create-as dom name desc vda vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda vdb
virsh snapshot-create-as dom name desc vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
Hmm, I'd better squash this in as well, so that we don't regress ;)
diff --git i/tests/virsh-optparse w/tests/virsh-optparse
index cd5e3eb..18252d2 100755
--- i/tests/virsh-optparse
+++ w/tests/virsh-optparse
@@ -80,13 +80,50 @@ cat <<\EOF > exp-out || framework_failure
</disks>
</domainsnapshot>
-
EOF
-virsh -c $test_url snapshot-create-as --print-xml test \
+virsh -q -c $test_url snapshot-create-as --print-xml test \
--diskspec 'vda,file=a&b,,c,snapshot=external' --description
'1<2' \
--diskspec vdb >out 2>>err || fail=1
compare out exp-out || fail=1
+cat <<\EOF > exp-out || framework_failure
+<domainsnapshot>
+ <name>name</name>
+ <description>vda</description>
+ <disks>
+ <disk name='vdb'/>
+ </disks>
+</domainsnapshot>
+
+EOF
+virsh -q -c $test_url snapshot-create-as --print-xml test name vda vdb \
+ >out 2>>err || fail=1
+compare out exp-out || fail=1
+
+cat <<\EOF > exp-out || framework_failure
+<domainsnapshot>
+ <name>name</name>
+ <description>desc</description>
+ <disks>
+ <disk name='vda'/>
+ <disk name='vdb'/>
+ </disks>
+</domainsnapshot>
+
+EOF
+for args in \
+ 'test name desc vda vdb' \
+ 'test name desc --diskspec vda vdb' \
+ 'test name desc --diskspec vda --diskspec vdb' \
+ 'test name desc vda vdb' \
+ 'test --diskspec vda name --diskspec vdb desc' \
+ '--description desc --name name --domain test vda vdb' \
+; do
+ virsh -q -c $test_url snapshot-create-as --print-xml $args \
+ >out 2>>err || fail=1
+ compare out exp-out || fail=1
+done
+
test -s err && fail=1
(exit $fail); exit $fail
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:39 p.m.
New subject: [libvirt] [PATCH] snapshot: tweak snapshot-create-as diskspec docs
On Wed, Sep 14, 2011 at 15:27:32 -0600, Eric Blake wrote:
With this patch, it is hopefully a bit more obvious that for
snapshot-create-as, a literal '--diskspec' is mandatory if name
or description was omitted, but optional if all earlier options
were provided.
These all denote two diskspecs and a description:
virsh snapshot-create-as dom name desc vda vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda vdb
virsh snapshot-create-as dom name desc vda --diskspec vdb
virsh snapshot-create-as dom name desc --diskspec vda --diskspec vdb
This gives two diskspecs but no description:
virsh snapshot-create-as dom name --diskspec vda --diskspec vdb
And this treats 'vda' as the description, with only one diskspec:
virsh snapshot-create-as dom name vda vdb
The help output now shows:
snapshot-create-as <domain> [<name>] [<description>] [--print-xml]
[--no-metadata] [--halt] [--disk-only] [[--diskspec] <string>]...
I also checked the help output for echo and send-key, which are two
other variants of argv commands.
* tools/virsh.pod (snapshot-create-as): Document when a literal
--diskspec must preceed a diskspec argument.
* tools/virsh.c (vshCmddefHelp): Update help output for argv when
naming the option is useful.
(vshCmddefGetData): Fix logic on when argv was seen.
Reported by Nan Zhang.
---
I think this is more comprehensive than Nan's patch, for nicer results.
tools/virsh.c | 27 +++++++++++++++++++++------
tools/virsh.pod | 6 ++++--
2 files changed, 25 insertions(+), 8 deletions(-)
Option parsing in virsh is... oh well, ACK :)
Jirka
5:27 p.m.
New subject: [libvirt] [PATCH] snapshot: tweak snapshot-create-as diskspec docs
On 09/15/2011 02:39 PM, Jiri Denemark wrote:
On Wed, Sep 14, 2011 at 15:27:32 -0600, Eric Blake wrote:
> With this patch, it is hopefully a bit more obvious that for
> snapshot-create-as, a literal '--diskspec' is mandatory if name
> or description was omitted, but optional if all earlier options
> were provided.
>
> tools/virsh.c | 27 +++++++++++++++++++++------
> tools/virsh.pod | 6 ++++--
> 2 files changed, 25 insertions(+), 8 deletions(-)
Option parsing in virsh is... oh well, ACK :)
Pushed now. I feel better having squashed in my testsuite changes
alongside it.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4817
days inactive
4818
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Nan Zhang