[libvirt] [PATCH 0/5]virsh: solutions for hiding '--shareable' option
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> As Daniel mentioned, '--shareable' should be hidden from users. But we had to take care of backwards compatibility. These patches give solutions to solve these issues. Chen Hanxiao (5): [libvirt]virsh: disable config readonly and shareable in virsh command [libvirt]virsh: introduce flags VSH_OFLAG_IGNORE [libvirt]virsh: if VSH_OFLAG_IGNORE set, don't show it in '--help' [libvirt]virsh: If VSH_OFLAG_IGNORE set, don't auto complete in virsh [libvirt]virsh: mark '--shareable' as VSH_OFLAG_IGNORE tools/virsh-domain.c | 3 ++- tools/virsh.c | 9 +++++++++ tools/virsh.h | 1 + tools/virsh.pod | 3 +-- 4 files changed, 13 insertions(+), 3 deletions(-) -- 1.8.2.1
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Daneil's suggestion about flag <shareable/> and <readonly/> as follow: - Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag So we should disable config both readonly and shareable in virsh command to solve the confliction. For backwards compatibility we keep the code about '<shareable/>'. In this patch, '--mode' option will have high priority. If set, it will screen the '--shareable' option. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6d241db..2aed9f9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -609,7 +609,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn); - if (vshCommandOptBool(cmd, "shareable")) + if (!mode && vshCommandOptBool(cmd, "shareable")) virBufferAddLit(&buf, " <shareable/>\n"); if (straddr) { -- 1.8.2.1
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Daneil's suggestion about flag <shareable/> and <readonly/> as follow: - Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag
So we should disable config both readonly and shareable in virsh command to solve the confliction. For backwards compatibility we keep the code about '<shareable/>'. In this patch, '--mode' option will have high priority. If set, it will screen the '--shareable' option.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6d241db..2aed9f9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -609,7 +609,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) + if (!mode && vshCommandOptBool(cmd, "shareable")) virBufferAddLit(&buf, " <shareable/>\n");
if (straddr) {
So IIUC, it's still possible to use '--mode readonly' and '--shareable' at the same time (of course, the latter one won't get applied, but no error is thrown either). I think, we should make use of '--mode' and '--shareable' exclusive. So we need something like this: diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b75f331..30fadce 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -602,8 +602,13 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn); - if (vshCommandOptBool(cmd, "shareable")) + if (vshCommandOptBool(cmd, "shareable")) { + if (mode) { + vshError(ctl, "%s", _("--shareable and --mode are mutually exclusive")); + goto cleanup; + } virBufferAddLit(&buf, " <shareable/>\n"); + } if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) { Michal
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Monday, October 21, 2013 9:15 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
- if (vshCommandOptBool(cmd, "shareable")) + if (!mode && vshCommandOptBool(cmd, "shareable")) virBufferAddLit(&buf, " <shareable/>\n");
if (straddr) {
So IIUC, it's still possible to use '--mode readonly' and '--shareable' at
the same
time (of course, the latter one won't get applied, but no error is thrown either).
Yes, we could still use this two parameters at the same time.
I think, we should make use of '--mode' and '--shareable' exclusive. So we
need
something like this:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b75f331..30fadce 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -602,8 +602,13 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) + if (vshCommandOptBool(cmd, "shareable")) { + if (mode) { + vshError(ctl, "%s", _("--shareable and --mode are mutually exclusive")); + goto cleanup; + } virBufferAddLit(&buf, " <shareable/>\n"); + }
if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) {
Looks reasonable. I'll add these in v2 patch due to your comments. Can I add your SOF? Thanks!
Michal
On 10/21/2013 02:14 PM, Michal Privoznik wrote:
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Daneil's suggestion about flag <shareable/> and <readonly/> as follow:
s/Daneil/Daniel/
- Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag
- if (vshCommandOptBool(cmd, "shareable")) + if (!mode && vshCommandOptBool(cmd, "shareable")) virBufferAddLit(&buf, " <shareable/>\n");
- if (vshCommandOptBool(cmd, "shareable")) + if (vshCommandOptBool(cmd, "shareable")) { + if (mode) { + vshError(ctl, "%s", _("--shareable and --mode are mutually exclusive")); + goto cleanup; + } virBufferAddLit(&buf, " <shareable/>\n"); + }
I don't think either of these approaches are needed. If you make --shareable an undocumented alias of --mode=shareable, then the alias handling code will already guarantee that only one of the two spellings appears, and you don't have to do any screening here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Tuesday, October 22, 2013 2:26 PM To: Michal Privoznik; Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
On 10/21/2013 02:14 PM, Michal Privoznik wrote:
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Daneil's suggestion about flag <shareable/> and <readonly/> as follow:
s/Daneil/Daniel/
Apologized for this typo.
- Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag
- if (vshCommandOptBool(cmd, "shareable")) + if (!mode && vshCommandOptBool(cmd, "shareable")) virBufferAddLit(&buf, " <shareable/>\n");
- if (vshCommandOptBool(cmd, "shareable")) + if (vshCommandOptBool(cmd, "shareable")) { + if (mode) { + vshError(ctl, "%s", _("--shareable and --mode are mutually
exclusive"));
+ goto cleanup; + } virBufferAddLit(&buf, " <shareable/>\n"); + }
I don't think either of these approaches are needed. If you make --shareable an undocumented alias of --mode=shareable, then the alias handling code will already guarantee that only one of the two spellings appears, and you don't have to do any screening here.
If I could do based on your description on: https://www.redhat.com/archives/libvir-list/2013-October/msg00965.html This patch could also be abandoned. Thanks
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Tuesday, October 22, 2013 2:26 PM To: Michal Privoznik; Chen Hanxiao Cc: libvir-list@redhat.com
- if (vshCommandOptBool(cmd, "shareable")) + if (vshCommandOptBool(cmd, "shareable")) { + if (mode) { + vshError(ctl, "%s", _("--shareable and --mode are mutually
exclusive"));
+ goto cleanup; + } virBufferAddLit(&buf, " <shareable/>\n"); + }
I don't think either of these approaches are needed. If you make --shareable an undocumented alias of --mode=shareable, then the alias handling code will already guarantee that only one of the two spellings appears, and you don't have to do any screening here.
I think we still need these codes. We can stop <TAB> to complete it and we can also undocument it. But if users just type "--mode=readonly --shareable" and press ENTER, we still need screen here.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/23/2013 09:57 AM, Chen Hanxiao wrote:
I don't think either of these approaches are needed. If you make --shareable an undocumented alias of --mode=shareable, then the alias handling code will already guarantee that only one of the two spellings appears, and you don't have to do any screening here.
I think we still need these codes. We can stop <TAB> to complete it and we can also undocument it. But if users just type "--mode=readonly --shareable" and press ENTER, we still need screen here.
No, the generic argument parser that checks that the user passed sane arguments should have already flagged it before we get into cmdAttachDisk, since the generic argument parser is what already knows how to deal with VSH_OT_ALIAS. Inside cmdAttachDisk, you shouldn't have to check for --shareable at all; the alias code should have already converted it to canonical form before getting to the specific command. I'll see if I can help write a patch to fix VSH_OT_ALIAS. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, October 23, 2013 5:49 PM To: Chen Hanxiao; 'Michal Privoznik' Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
On 10/23/2013 09:57 AM, Chen Hanxiao wrote:
I don't think either of these approaches are needed. If you make --shareable an undocumented alias of --mode=shareable, then the alias handling code will already guarantee that only one of the two spellings appears, and you don't have to do any screening here.
I think we still need these codes. We can stop <TAB> to complete it and we can also undocument it. But if users just type "--mode=readonly --shareable" and press ENTER, we still need screen here.
No, the generic argument parser that checks that the user passed sane arguments should have already flagged it before we get into cmdAttachDisk, since the generic argument parser is what already knows how to deal with VSH_OT_ALIAS. Inside cmdAttachDisk, you shouldn't have to check for --shareable at all; the alias code should have already converted it to canonical form before getting to the specific command.
I'll see if I can help write a patch to fix VSH_OT_ALIAS.
Thanks. My v2 patches almost finished and will be posted in a few hours. If this one could not fit our will, then please do me a favor :)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> If some command option should be phased out, we need to hide it. But for backwards compatibility, we had to keep those obsolete codes. With this flag, we could keep most of the old codes and let tools like virsh know don't show it. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh.h | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh.h b/tools/virsh.h index f978d94..029ae9f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -136,6 +136,7 @@ enum { VSH_OFLAG_REQ = (1 << 0), /* option required */ VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ VSH_OFLAG_REQ_OPT = (1 << 2), /* --optionname required */ + VSH_OFLAG_IGNORE = (1 << 3), /* keep this option but don't show it to users */ }; /* forward declarations */ -- 1.8.2.1
On 10/18/2013 07:42 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
If some command option should be phased out, we need to hide it. But for backwards compatibility, we had to keep those obsolete codes. With this flag, we could keep most of the old codes and let tools like virsh know don't show it.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/virsh.h b/tools/virsh.h index f978d94..029ae9f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -136,6 +136,7 @@ enum { VSH_OFLAG_REQ = (1 << 0), /* option required */ VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ VSH_OFLAG_REQ_OPT = (1 << 2), /* --optionname required */ + VSH_OFLAG_IGNORE = (1 << 3), /* keep this option but don't show it to users */
I'm not convinced. I think the VSH_OT_ALIAS should instead be enhanced to do what we want, rather than adding yet another mechanism for hiding options. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> If VSH_OFLAG_IGNORE set, don't show it in '--help'. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index 6842ed8..f772794 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1004,6 +1004,8 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, if (i > 31) return -1; /* too many options */ + if (opt->flags & VSH_OFLAG_IGNORE) + continue; if (opt->type == VSH_OT_BOOL) { if (opt->flags & VSH_OFLAG_REQ) return -1; /* bool options can't be mandatory */ @@ -1213,6 +1215,8 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) if (def->opts) { const vshCmdOptDef *opt; for (opt = def->opts; opt->name; opt++) { + if (opt->flags & VSH_OFLAG_IGNORE) + continue; const char *fmt = "%s"; switch (opt->type) { case VSH_OT_BOOL: @@ -1267,6 +1271,8 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) const vshCmdOptDef *opt; fputs(_("\n OPTIONS\n"), stdout); for (opt = def->opts; opt->name; opt++) { + if (opt->flags & VSH_OFLAG_IGNORE) + continue; switch (opt->type) { case VSH_OT_BOOL: snprintf(buf, sizeof(buf), "--%s", opt->name); -- 1.8.2.1
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> If VSH_OFLAG_IGNORE set, don't auto complete in virsh. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index f772794..42d79f1 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2677,6 +2677,9 @@ vshReadlineOptionsGenerator(const char *text, int state) /* ignore non --option */ continue; + if (opt->flags & VSH_OFLAG_IGNORE) + continue; + if (len > 2) { if (STRNEQLEN(name, text + 2, len - 2)) continue; -- 1.8.2.1
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
If VSH_OFLAG_IGNORE set, don't auto complete in virsh.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index f772794..42d79f1 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2677,6 +2677,9 @@ vshReadlineOptionsGenerator(const char *text, int state) /* ignore non --option */ continue;
+ if (opt->flags & VSH_OFLAG_IGNORE) + continue; + if (len > 2) { if (STRNEQLEN(name, text + 2, len - 2)) continue;
These three (2/5 3/5 and 4/5) could have been merged into a single patch. In fact, I think they *have to* be merged, otherwise we may end up with broken backports (due to incomplete functionality being backported). Michal
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Monday, October 21, 2013 9:16 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 4/5]virsh: If VSH_OFLAG_IGNORE set, don't auto complete it in virsh
- 2))
continue;
These three (2/5 3/5 and 4/5) could have been merged into a single patch. In fact, I think they *have to* be merged, otherwise we may end up with broken backports (due to incomplete functionality being backported).
OK, I will merge them in v2 patch. I just want to make this patch set more methodical. Thanks!
Michal
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Mark '--shareable' as VSH_OFLAG_IGNORE and delete related description in docs. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 1 + tools/virsh.pod | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2aed9f9..b5b9006 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -313,6 +313,7 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "shareable", .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_IGNORE, .help = N_("shareable between domains") }, {.name = "rawio", diff --git a/tools/virsh.pod b/tools/virsh.pod index 7af5503..cb273e0 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. -- 1.8.2.1
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Mark '--shareable' as VSH_OFLAG_IGNORE and delete related description in docs.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 1 + tools/virsh.pod | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2aed9f9..b5b9006 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -313,6 +313,7 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "shareable", .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_IGNORE, .help = N_("shareable between domains") }, {.name = "rawio", diff --git a/tools/virsh.pod b/tools/virsh.pod index 7af5503..cb273e0 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.
Wow, we've never wanted to hide any option before? I've recall introducing VSH_OT_ALIAS, but there hasn't been anything else? Michal
@@ -313,6 +313,7 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "shareable", .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_IGNORE, .help = N_("shareable between domains") }, {.name = "rawio", diff --git a/tools/virsh.pod b/tools/virsh.pod index 7af5503..cb273e0 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> is the address of disk device in the form of
-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] pci:domain.bus.slot.function,
scsi:controller.bus.unit or ide:controller.bus.unit.
Wow, we've never wanted to hide any option before? I've recall introducing VSH_OT_ALIAS, but there hasn't been anything else?
Do you mean we just treat --shareable as an alias for --mode like this: {.name = "shareable", - .type = VSH_OT_BOOL, + .type = VSH_OT_ALIAS, - .help = N_("shareable between domains") + .help = "mode" }, But with VSH_OT_ALIAS, auto complete could also get it. We still need new flag to hide it. Thanks!
Michal
On 10/22/2013 07:13 AM, Chen Hanxiao wrote:
Wow, we've never wanted to hide any option before? I've recall introducing VSH_OT_ALIAS, but there hasn't been anything else?
We've always hidden one spelling that duplicates another, but this is a case where a boolean option is a duplicate of a non-boolean option, so I'm not sure if VSH_OT_ALIAS alone does what we want (or maybe it just means we enhance VSH_OT_ALIAS to be able to express a boolean alias for a non-boolean default).
Do you mean we just treat --shareable as an alias for --mode like this:
{.name = "shareable", - .type = VSH_OT_BOOL, + .type = VSH_OT_ALIAS, - .help = N_("shareable between domains") + .help = "mode"
If it works. Maybe even by having it be an alias for .help = "mode=shareable" rather than just "mode", and teach VSH_OT_ALIAS to handle '=' within the .help text.
},
But with VSH_OT_ALIAS, auto complete could also get it.
Command line completion is orthogonal. It needs fixing anyways (there were patches posted last month, but I haven't seen any recent action on them). Ideally, we want the following: attach-disk <TAB> to NOT show --shareable (because it can show --mode instead) attach-disk --sh <TAB> would show --shareable (just because it's deprecated and hidden by default does NOT mean it must be hidden if the user explicitly requests something that can't complete in any other way) attach-disk --mode=shareable --sh<TAB> attach-disk --mode readonly --sh<TAB> would both complain about no possible completion (because --shareable has already been eliminated by --mode appearing earlier). attach-disk --shareable --mo<TAB> would complain about no possible completion (--shareable implies --mode=shareable, and --mode can't appear more than once). But getting that working in this patch series is NOT a prerequisite - save it for everything else that needs fixing with completion.
We still need new flag to hide it.
I'm still not convinced we need VSH_OFLAG_IGNORE. VSH_OT_ALIAS, plus new logic that lets a boolean be an alias to non-bool=value, ought to be the way to do it (ie. fix our existing aliasing mechanism, rather than adding a second aliasing mechanism). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Tuesday, October 22, 2013 2:23 PM To: Chen Hanxiao; 'Michal Privoznik' Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 5/5]virsh: mark '--shareable' as VSH_OFLAG_IGNORE
On 10/22/2013 07:13 AM, Chen Hanxiao wrote:
Wow, we've never wanted to hide any option before? I've recall introducing VSH_OT_ALIAS, but there hasn't been anything else?
We've always hidden one spelling that duplicates another, but this is a case where a boolean option is a duplicate of a non-boolean option, so I'm not sure if VSH_OT_ALIAS alone does what we want (or maybe it just means we enhance VSH_OT_ALIAS to be able to express a boolean alias for a non-boolean default).
Do you mean we just treat --shareable as an alias for --mode like this:
{.name = "shareable", - .type = VSH_OT_BOOL, + .type = VSH_OT_ALIAS, - .help = N_("shareable between domains") + .help = "mode"
If it works. Maybe even by having it be an alias for .help = "mode=shareable" rather than just "mode", and teach VSH_OT_ALIAS to handle '=' within the .help text.
Better choice.
},
But with VSH_OT_ALIAS, auto complete could also get it.
Command line completion is orthogonal. It needs fixing anyways (there were patches posted last month, but I haven't seen any recent action on them).
Ideally, we want the following:
attach-disk <TAB>
to NOT show --shareable (because it can show --mode instead)
attach-disk --sh <TAB>
would show --shareable (just because it's deprecated and hidden by default does NOT mean it must be hidden if the user explicitly requests something that can't complete in any other way)
attach-disk --mode=shareable --sh<TAB> attach-disk --mode readonly --sh<TAB>
would both complain about no possible completion (because --shareable has already been eliminated by --mode appearing earlier).
attach-disk --shareable --mo<TAB>
would complain about no possible completion (--shareable implies --mode=shareable, and --mode can't appear more than once).
But getting that working in this patch series is NOT a prerequisite - save it for everything else that needs fixing with completion.
Thanks for your elaborate explanation. I'll try to reach these goals, although they're a little complex.
We still need new flag to hide it.
I'm still not convinced we need VSH_OFLAG_IGNORE. VSH_OT_ALIAS, plus new logic that lets a boolean be an alias to non-bool=value, ought to be the way to do it (ie. fix our existing aliasing mechanism, rather than adding a second aliasing mechanism).
Agree. I'll try to expand the logic of VSH_OT_ALIAS. Thanks!
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Chen Hanxiao -
Eric Blake -
Michal Privoznik