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 --diskspec vda --diskspec vdb name desc

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.
* tests/virsh-optparse: Add tests to avoid regressions.

diff --git a/tests/virsh-optparse b/tests/virsh-optparse
index cd5e3eb884e797c85db2956c9531cc9adace8e46..18252d2959d99078917cccbe07f0ce536bedee3c 100755 (executable)
--- a/tests/virsh-optparse
+++ b/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

diff --git a/tools/virsh.c b/tools/virsh.c
index 3c6e65ad9217ca5968e8a7889b7fa054f26f8709..fdb503a915131d77ca5a4d2b929d50a85b852fbc 100644 (file)
--- 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 27d8f4270f1301926e12329567492ae64006f7ee..5114580b365084c926fa4ffe1bfd7179d7f7f735 100644 (file)
--- 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'/>