[libvirt] [PATCH] Fix typos for snapshot-create-as in virsh help cmd and doc
* 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :-)
* 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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Nan Zhang