[libvirt] [PATCHv2 0/2] virsh: hide 'attach-disk --shareable'
As promised here: https://www.redhat.com/archives/libvir-list/2013-October/msg01047.html Eric Blake (2): virsh: allow alias to expand to opt=value pair virsh: undocument --shareable (--mode already covers it) tests/virshtest.c | 1 + tools/virsh-domain.c | 11 ++++------ tools/virsh.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 4 ++-- 4 files changed, 57 insertions(+), 17 deletions(-) -- 1.8.3.1
We want to treat 'attach-disk --shareable' as an undocumented alias for 'attach-disk --mode=shareable'. By improving our alias handling, we can allow all such --bool -> --opt=value replacements, and guarantee up front that the alias is not mixed with its replacement. * tools/virsh.c (vshCmddefOptParse, vshCmddefGetOption): Add support for expanding bool alias to --opt=value. (opts_echo): Add another alias to test it. * tests/virshtest.c (mymain): Test it. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virshtest.c | 1 + tools/virsh.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 8367248..6510208 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -393,6 +393,7 @@ mymain(void) DO_TEST(32, "hello\n", "echo --string hello"); DO_TEST(33, "hello\n", "echo", "--str", "hello"); DO_TEST(34, "hello\n", "echo --str hello"); + DO_TEST(35, "hello\n", "echo --hi"); # undef DO_TEST diff --git a/tools/virsh.c b/tools/virsh.c index 8425f53..bad78c9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -885,6 +885,10 @@ static const vshCmdOptDef opts_echo[] = { .type = VSH_OT_ALIAS, .help = "string" }, + {.name = "hi", + .type = VSH_OT_ALIAS, + .help = "string=hello" + }, {.name = "string", .type = VSH_OT_ARGV, .help = N_("arguments to echo") @@ -1011,12 +1015,25 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, } if (opt->type == VSH_OT_ALIAS) { size_t j; + char *name = (char *)opt->help; /* cast away const */ + char *p; + if (opt->flags || !opt->help) return -1; /* alias options are tracked by the original name */ + if ((p = strchr(name, '=')) && + VIR_STRNDUP(name, name, p - name) < 0) + return -1; for (j = i + 1; cmd->opts[j].name; j++) { - if (STREQ(opt->help, cmd->opts[j].name)) + if (STREQ(name, cmd->opts[j].name) && + cmd->opts[j].type != VSH_OT_ALIAS) break; } + if (name != opt->help) { + VIR_FREE(name); + /* If alias comes with value, replacement must not be bool */ + if (cmd->opts[j].type == VSH_OT_BOOL) + return -1; + } if (!cmd->opts[j].name) return -1; /* alias option must map to a later option name */ continue; @@ -1049,9 +1066,11 @@ static vshCmdOptDef helpopt = { }; static const vshCmdOptDef * vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, - uint32_t *opts_seen, int *opt_index) + uint32_t *opts_seen, int *opt_index, char **optstr) { size_t i; + const vshCmdOptDef *ret = NULL; + char *alias = NULL; if (STREQ(name, helpopt.name)) { return &helpopt; @@ -1062,16 +1081,36 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, if (STREQ(opt->name, name)) { if (opt->type == VSH_OT_ALIAS) { - name = opt->help; + char *value; + + /* Two types of replacements: + opt->help = "string": straight replacement of name + opt->help = "string=value": treat boolean flag as + alias of option and its default value */ + sa_assert(!alias); + if (VIR_STRDUP(alias, opt->help) < 0) + goto cleanup; + name = alias; + if ((value = strchr(name, '='))) { + *value = '\0'; + if (*optstr) { + vshError(ctl, _("invalid '=' after option --%s"), + opt->name); + goto cleanup; + } + if (VIR_STRDUP(*optstr, value + 1) < 0) + goto cleanup; + } continue; } if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { vshError(ctl, _("option --%s already seen"), name); - return NULL; + goto cleanup; } *opts_seen |= 1 << i; *opt_index = i; - return opt; + ret = opt; + goto cleanup; } } @@ -1079,7 +1118,9 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, vshError(ctl, _("command '%s' doesn't support option --%s"), cmd->name, name); } - return NULL; +cleanup: + VIR_FREE(alias); + return ret; } static const vshCmdOptDef * @@ -1845,7 +1886,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) } /* Special case 'help' to ignore all spurious options */ if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2, - &opts_seen, &opt_index))) { + &opts_seen, &opt_index, + &optstr))) { VIR_FREE(optstr); if (STREQ(cmd->name, "help")) continue; @@ -1875,7 +1917,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) tkdata = NULL; if (optstr) { vshError(ctl, _("invalid '=' after option --%s"), - opt->name); + opt->name); VIR_FREE(optstr); goto syntaxError; } -- 1.8.3.1
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Thursday, October 24, 2013 3:22 PM To: libvir-list@redhat.com Cc: chenhanxiao@cn.fujitsu.com Subject: [PATCHv2 1/2] virsh: allow alias to expand to opt=value pair
We want to treat 'attach-disk --shareable' as an undocumented alias for 'attach-disk --mode=shareable'. By improving our alias handling, we can allow all such --bool -> --opt=value replacements, and guarantee up front that the alias is not mixed with its replacement.
* tools/virsh.c (vshCmddefOptParse, vshCmddefGetOption): Add support for expanding bool alias to --opt=value. (opts_echo): Add another alias to test it. * tests/virshtest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virshtest.c | 1 + tools/virsh.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 8 deletions(-)
ACK Thanks!
Commit e962a57 added 'attach-disk --shareable', even though we already had 'attach-disk --mode=shareable'. Worse, if the user types 'attach-disk --mode=readonly --shareable', we create non-sensical XML. The best solution is just to undocument the duplicate spelling, by having it fall back to the preferred spelling. * tools/virsh-domain.c (cmdAttachDisk): Let alias handling fix our mistake in exposing a second spelling for an existing option. * tools/virsh.pod: Fix documentation. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 11 ++++------- tools/virsh.pod | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5aabccd..59e3d8d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -295,6 +295,10 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_STRING, .help = N_("target device type") }, + {.name = "shareable", + .type = VSH_OT_ALIAS, + .help = "mode=shareable" + }, {.name = "mode", .type = VSH_OT_STRING, .help = N_("mode of device reading and writing") @@ -311,10 +315,6 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_STRING, .help = N_("wwn of disk device") }, - {.name = "shareable", - .type = VSH_OT_BOOL, - .help = N_("shareable between domains") - }, {.name = "rawio", .type = VSH_OT_BOOL, .help = N_("needs rawio capability") @@ -602,9 +602,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn); - if (vshCommandOptBool(cmd, "shareable")) - virBufferAddLit(&buf, " <shareable/>\n"); - if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) { vshError(ctl, _("Invalid address.")); diff --git a/tools/virsh.pod b/tools/virsh.pod index 7af5503..302f811 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1923,7 +1923,7 @@ expected. [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] [I<--driver driver>] [I<--subdriver subdriver>] [I<--cache cache>] [I<--type type>] [I<--mode mode>] [I<--config>] [I<--sourcetype soucetype>] -[I<--serial serial>] [I<--wwn wwn>] [I<--shareable>] [I<--rawio>] +[I<--serial serial>] [I<--wwn wwn>] [I<--rawio>] [I<--address address>] [I<--multifunction>] [I<--print-xml>] Attach a new disk device to the domain. @@ -1945,7 +1945,6 @@ I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough", "writeback", "directsync" or "unsafe". I<serial> is the serial of disk device. I<wwn> is the wwn of disk device. -I<shareable> indicates the disk device is shareable between domains. I<rawio> indicates the disk needs rawio capability. I<address> is the address of disk device in the form of pci:domain.bus.slot.function, scsi:controller.bus.unit or ide:controller.bus.unit. @@ -1964,6 +1963,7 @@ on the hypervisor driver. For compatibility purposes, I<--persistent> behaves like I<--config> for an offline domain, and like I<--live> I<--config> for a running domain. +Likewise, I<--shareable> is an alias for I<--mode shareable>. =item B<attach-interface> I<domain> I<type> I<source> [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 1.8.3.1
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Thursday, October 24, 2013 3:22 PM To: libvir-list@redhat.com Cc: chenhanxiao@cn.fujitsu.com Subject: [PATCHv2 2/2] virsh: undocument --shareable (--mode already covers it)
Commit e962a57 added 'attach-disk --shareable', even though we already had 'attach-disk --mode=shareable'. Worse, if the user types 'attach-disk --mode=readonly --shareable', we create non-sensical XML. The best solution is just to undocument the duplicate spelling, by having it fall back to the preferred spelling.
* tools/virsh-domain.c (cmdAttachDisk): Let alias handling fix our mistake in exposing a second spelling for an existing option. * tools/virsh.pod: Fix documentation.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 11 ++++-------
ACK Thanks!
On 10/24/13 08:21, Eric Blake wrote:
As promised here: https://www.redhat.com/archives/libvir-list/2013-October/msg01047.html
Eric Blake (2): virsh: allow alias to expand to opt=value pair virsh: undocument --shareable (--mode already covers it)
tests/virshtest.c | 1 + tools/virsh-domain.c | 11 ++++------ tools/virsh.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 4 ++-- 4 files changed, 57 insertions(+), 17 deletions(-)
ACK series. Hopefully we won't need much hacks like this. Peter
On 10/24/2013 11:05 AM, Peter Krempa wrote:
On 10/24/13 08:21, Eric Blake wrote:
As promised here: https://www.redhat.com/archives/libvir-list/2013-October/msg01047.html
Eric Blake (2): virsh: allow alias to expand to opt=value pair virsh: undocument --shareable (--mode already covers it)
tests/virshtest.c | 1 + tools/virsh-domain.c | 11 ++++------ tools/virsh.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 4 ++-- 4 files changed, 57 insertions(+), 17 deletions(-)
ACK series. Hopefully we won't need much hacks like this.
Thanks; pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Chen Hanxiao -
Eric Blake -
Peter Krempa