From 85d2810823a31634b12145d6c196930b40425370 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 14 Sep 2011 15:20:08 -0600 Subject: [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 --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 [] [] [--print-xml] [--no-metadata] [--halt] [--disk-only] [[--diskspec] ]... 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. --- tests/virsh-optparse | 41 +++++++++++++++++++++++++++++++++++++++-- tools/virsh.c | 27 +++++++++++++++++++++------ tools/virsh.pod | 6 ++++-- 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index cd5e3eb884..18252d2959 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -80,13 +80,50 @@ cat <<\EOF > exp-out || framework_failure - 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 + + name + vda + + + + + +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 + + name + desc + + + + + + +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 3c6e65ad92..fdb503a915 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 ]")); + if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + shortopt = true; break; case VSH_OT_STRING: /* xgettext:c-format */ fmt = _("[--%s ]"); + 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] }...") + : _("[[--%s] ]..."); + } 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] ") : _("<%s>"), + opt->name); break; default: assert(0); diff --git a/tools/virsh.pod b/tools/virsh.pod index 27d8f4270f..5114580b36 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1773,7 +1773,7 @@ by command such as B or by internal guest action). =item B I {[I<--print-xml>] | [I<--no-metadata>] [I<--halt>]} [I] [I] -[I<--disk-only> [I]...] +[I<--disk-only> [[I<--diskspec>] B]... Create a snapshot for domain I with the given and ; 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 arguments to add elements to the xml. Each is in the form B. To include a literal comma in B or in B, 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 unless +all three of I, I, and I are also present. +For example, a diskspec of "vda,snapshot=external,file=/path/to,,new" results in the following XML: