1:42 a.m.
From: Chen Hanxiao <chenhanxiao(a)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
1:42 a.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
From: Chen Hanxiao <chenhanxiao(a)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(a)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
8:14 a.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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(a)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
10:16 p.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
-----Original Message-----
From: Michal Privoznik [mailto:mprivozn@redhat.com]
Sent: Monday, October 21, 2013 9:15 PM
To: Chen Hanxiao
Cc: libvir-list(a)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
1:25 a.m.
New subject: [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(a)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
2:17 a.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
-----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(a)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(a)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
3:57 a.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
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(a)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
4:49 a.m.
New subject: [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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:58 a.m.
New subject: [libvirt] [PATCH 1/5]virsh: disable config readonly and shareable in virsh command
-----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(a)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
1:42 a.m.
New subject: [libvirt] [PATCH 2/5]virsh: introduce flags VSH_OFLAG_IGNORE
From: Chen Hanxiao <chenhanxiao(a)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(a)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
1:27 a.m.
New subject: [libvirt] [PATCH 2/5]virsh: introduce flags VSH_OFLAG_IGNORE
On 10/18/2013 07:42 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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(a)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
1:42 a.m.
New subject: [libvirt] [PATCH 3/5]virsh: if VSH_OFLAG_IGNORE set, don't show it in '--help'
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
If VSH_OFLAG_IGNORE set, don't show it in '--help'.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
1:42 a.m.
New subject: [libvirt] [PATCH 4/5]virsh: If VSH_OFLAG_IGNORE set, don't auto complete it in virsh
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
If VSH_OFLAG_IGNORE set, don't auto complete in virsh.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
8:16 a.m.
New subject: [libvirt] [PATCH 4/5]virsh: If VSH_OFLAG_IGNORE set, don't auto complete it in virsh
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
If VSH_OFLAG_IGNORE set, don't auto complete in virsh.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
10:22 p.m.
New subject: [libvirt] [PATCH 4/5]virsh: If VSH_OFLAG_IGNORE set, don't auto complete it in virsh
-----Original Message-----
From: Michal Privoznik [mailto:mprivozn@redhat.com]
Sent: Monday, October 21, 2013 9:16 PM
To: Chen Hanxiao
Cc: libvir-list(a)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
1:42 a.m.
New subject: [libvirt] [PATCH 5/5]virsh: mark '--shareable' as VSH_OFLAG_IGNORE
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
Mark '--shareable' as VSH_OFLAG_IGNORE and
delete related description in docs.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
8:14 a.m.
New subject: [libvirt] [PATCH 5/5]virsh: mark '--shareable' as VSH_OFLAG_IGNORE
On 18.10.2013 07:42, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
Mark '--shareable' as VSH_OFLAG_IGNORE and
delete related description in docs.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
1:13 a.m.
New subject: [libvirt] [PATCH 5/5]virsh: mark '--shareable' as VSH_OFLAG_IGNORE
-----Original Message-----
From: Michal Privoznik [mailto:mprivozn@redhat.com]
> @@ -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
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
1:22 a.m.
New subject: [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.
},
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
1:48 a.m.
New subject: [libvirt] [PATCH 5/5]virsh: mark '--shareable' as VSH_OFLAG_IGNORE
-----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(a)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
4048
days inactive
4053
days old
19 comments
3 participants
participants (3)
-
Chen Hanxiao -
Eric Blake -
Michal Privoznik