[libvirt] [PATCH 0/2] enable attatch-disk command option '--mode' accept two parameters
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> This patch set enable '--mode' accept both 'readonly' and 'shareable' at the same time. And remove duplicate option '--shareable' Chen Hanxiao (2): [libvirt]virsh: enable attatch-disk command option '--mode' accept two parameters [libvirt]virsh: remove attach-disk '--shareable' option tools/virsh-domain.c | 29 +++++++++++++++++++---------- tools/virsh.pod | 5 ++--- 2 files changed, 21 insertions(+), 13 deletions(-) -- 1.8.2.1
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter. This patch enables '--mode' accept parameters like examples below: virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 2 +- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 856e888..ba67a69 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -563,7 +563,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) } if (mode) { - if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + if (!(STRPREFIX(mode, "readonly") || + STRPREFIX(mode, "shareable"))) { vshError(ctl, _("No support for %s in command 'attach-disk'"), mode); goto cleanup; @@ -600,8 +601,23 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) (isFile) ? "file" : "dev", source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); - if (mode) - virBufferAsprintf(&buf, " <%s/>\n", mode); + if (mode) { + if (STRPREFIX(mode, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else { + virBufferAddLit(&buf, " <shareable/>\n"); + } + + char *rest; + if ((rest = strchr(mode, ','))) { + rest++; + if (STRPREFIX(rest, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else if (STRPREFIX(rest, "shareable")) { + virBufferAddLit(&buf, " <shareable/>\n"); + } + } + } if (serial) virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); diff --git a/tools/virsh.pod b/tools/virsh.pod index e12a800..f6a80a3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1915,7 +1915,7 @@ expected. =item B<attach-disk> I<domain> I<source> I<target> [[[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<--type type>] [I<--mode mode[,mode2]>] [I<--config>] [I<--sourcetype soucetype>] [I<--serial serial>] [I<--wwn wwn>] [I<--shareable>] [I<--rawio>] [I<--address address>] [I<--multifunction>] [I<--print-xml>] -- 1.8.2.1
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 2 +- 2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 856e888..ba67a69 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -563,7 +563,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) }
if (mode) { - if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + if (!(STRPREFIX(mode, "readonly") || + STRPREFIX(mode, "shareable"))) {
This won't work if you use "--mode readonly,asdf". Also indentation of the second line is off.
vshError(ctl, _("No support for %s in command 'attach-disk'"), mode); goto cleanup; @@ -600,8 +601,23 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) (isFile) ? "file" : "dev", source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); - if (mode) - virBufferAsprintf(&buf, " <%s/>\n", mode); + if (mode) { + if (STRPREFIX(mode, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else { + virBufferAddLit(&buf, " <shareable/>\n"); + } + + char *rest; + if ((rest = strchr(mode, ','))) { + rest++; + if (STRPREFIX(rest, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else if (STRPREFIX(rest, "shareable")) { + virBufferAddLit(&buf, " <shareable/>\n"); + } + } + }
Hmmm, this is a very convoluted way to do stuff. I would recommend doing the sanity check right and then you can do either: if (mode && strstr(mode, "readonly")) virBufferAddLit(&buf, " <readonly/>\n"); if (mode && strstr(mode, "shareable")) virBufferAddLit... or tokenize the string properly (see vshStringToArray) and output the elements in a loop
if (serial) virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); diff --git a/tools/virsh.pod b/tools/virsh.pod index e12a800..f6a80a3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1915,7 +1915,7 @@ expected. =item B<attach-disk> I<domain> I<source> I<target> [[[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<--type type>] [I<--mode mode[,mode2]>] [I<--config>] [I<--sourcetype soucetype>] [I<--serial serial>] [I<--wwn wwn>] [I<--shareable>] [I<--rawio>] [I<--address address>] [I<--multifunction>] [I<--print-xml>]
also it might be worth mentioning the meaning of the option in the text after this block. Peter
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:58 PM On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
if (mode) { - if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + if (!(STRPREFIX(mode, "readonly") || + STRPREFIX(mode, "shareable"))) {
This won't work if you use "--mode readonly,asdf". Also indentation of the second line is off.
If we use "--mode readonly,asdf", only 'readonly' will be recognized as valid parameter.
vshError(ctl, _("No support for %s in command
'attach-disk'"),
mode); goto cleanup; @@ -600,8 +601,23 @@ cmdAttachDisk(vshControl *ctl, const vshCmd
*cmd)
(isFile) ? "file" : "dev", source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); - if (mode) - virBufferAsprintf(&buf, " <%s/>\n", mode); + if (mode) { + if (STRPREFIX(mode, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else { + virBufferAddLit(&buf, " <shareable/>\n"); + } + + char *rest; + if ((rest = strchr(mode, ','))) { + rest++; + if (STRPREFIX(rest, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else if (STRPREFIX(rest, "shareable")) { + virBufferAddLit(&buf, " <shareable/>\n"); + } + } + }
Hmmm, this is a very convoluted way to do stuff. I would recommend doing the sanity check right and then you can do either:
if (mode && strstr(mode, "readonly")) virBufferAddLit(&buf, " <readonly/>\n");
if (mode && strstr(mode, "shareable")) virBufferAddLit...
If we use strstr(), --mode XXshareableXX will take effect. I try to let --mode accept: (readonly as A, shareable as B) A B A,B[xxxx] B,A[xxxx] A,A[xxxx] B,B[xxxx]
or tokenize the string properly (see vshStringToArray) and output the elements in a loop
I will check this and see. Thanks for you hints.
if (serial) virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); diff --git a/tools/virsh.pod b/tools/virsh.pod index e12a800..f6a80a3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1915,7 +1915,7 @@ expected. =item B<attach-disk> I<domain> I<source> I<target> [[[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<--type type>] [I<--mode mode[,mode2]>] [I<--config>] [I<--sourcetype soucetype>] [I<--serial serial>] [I<--wwn wwn>] [I<--shareable>] [I<--rawio>] [I<--address address>] [I<--multifunction>] [I<--print-xml>]
also it might be worth mentioning the meaning of the option in the text after this block.
I'll add it in the future patch.
Peter
On 10/15/13 11:41, Chen Hanxiao wrote:
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:58 PM On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
if (mode) { - if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + if (!(STRPREFIX(mode, "readonly") || + STRPREFIX(mode, "shareable"))) {
This won't work if you use "--mode readonly,asdf". Also indentation of the second line is off.
If we use "--mode readonly,asdf", only 'readonly' will be recognized as valid parameter.
'readonly' is valid, but as that condition is only checking the prefix of the @mode string to be 'readonly' or 'shareable' then you can write anything after that and the check won't trigger. If you are going to use a comma separated list here, you need to tokenize the string and check every item in the array.
vshError(ctl, _("No support for %s in command
'attach-disk'"),
mode); goto cleanup; @@ -600,8 +601,23 @@ cmdAttachDisk(vshControl *ctl, const vshCmd
*cmd)
(isFile) ? "file" : "dev", source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); - if (mode) - virBufferAsprintf(&buf, " <%s/>\n", mode); + if (mode) { + if (STRPREFIX(mode, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else { + virBufferAddLit(&buf, " <shareable/>\n"); + } + + char *rest; + if ((rest = strchr(mode, ','))) { + rest++; + if (STRPREFIX(rest, "readonly")) { + virBufferAddLit(&buf, " <readonly/>\n"); + } else if (STRPREFIX(rest, "shareable")) { + virBufferAddLit(&buf, " <shareable/>\n"); + } + } + }
Hmmm, this is a very convoluted way to do stuff. I would recommend doing the sanity check right and then you can do either:
if (mode && strstr(mode, "readonly")) virBufferAddLit(&buf, " <readonly/>\n");
if (mode && strstr(mode, "shareable")) virBufferAddLit...
If we use strstr(), --mode XXshareableXX will take effect. I try to let --mode accept: (readonly as A, shareable as B)
If you make the argument check above bulletproof, which it isn't right now it will not bother you.
A B A,B[xxxx] B,A[xxxx] A,A[xxxx] B,B[xxxx]
or tokenize the string properly (see vshStringToArray) and output the elements in a loop
I will check this and see. Thanks for you hints.
if (serial) virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial);
Peter
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 5:48 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command
option
'--mode' accept two parameters
On 10/15/13 11:41, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> This won't work if you use "--mode readonly,asdf". Also indentation of
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:58 PM On 10/15/13 05:54, Chen Hanxiao wrote: the second line is off.
If we use "--mode readonly,asdf", only 'readonly' will be recognized as valid parameter.
'readonly' is valid, but as that condition is only checking the prefix of the @mode string to be 'readonly' or 'shareable' then you can write anything after that and the check won't trigger. If you are going to use a comma separated list here, you need to tokenize the string and check every item in the array.
Thanks for your kindly advice. If we decide to keep this patch, I'll finish it as you mentioned above. Thanks.
+ } else if (STRPREFIX(rest, "shareable")) { + virBufferAddLit(&buf, " <shareable/>\n"); + } + } + }
Hmmm, this is a very convoluted way to do stuff. I would recommend
doing
the sanity check right and then you can do either:
if (mode && strstr(mode, "readonly")) virBufferAddLit(&buf, " <readonly/>\n");
if (mode && strstr(mode, "shareable")) virBufferAddLit...
If we use strstr(), --mode XXshareableXX will take effect. I try to let --mode accept: (readonly as A, shareable as B)
If you make the argument check above bulletproof, which it isn't right now it will not bother you.
Peter
On Tue, Oct 15, 2013 at 11:54:27AM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
That does not make sense - these options are mutually exclusive. Anything 'readonly' is implicitly 'shareable' without 'shareable' being required. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, October 15, 2013 5:42 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
On Tue, Oct 15, 2013 at 11:54:27AM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
That does not make sense - these options are mutually exclusive. Anything 'readonly' is implicitly 'shareable' without 'shareable' being required.
Do you mean that we should erase all about parameter 'shareable'? Not only '--shareable' but also in '--mode shareable'? If we decided to keep readonly and shareable in '--mode', we had to deal with accepting two parameters at the same command line.
Daniel --
On Tue, Oct 15, 2013 at 06:06:54PM +0800, Chen Hanxiao wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, October 15, 2013 5:42 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
On Tue, Oct 15, 2013 at 11:54:27AM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Current disk could accept both 'readonly' and 'shareable' at the same time. But '--mode' could only accept one parameter.
This patch enables '--mode' accept parameters like examples below:
virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
That does not make sense - these options are mutually exclusive. Anything 'readonly' is implicitly 'shareable' without 'shareable' being required.
Do you mean that we should erase all about parameter 'shareable'? Not only '--shareable' but also in '--mode shareable'? If we decided to keep readonly and shareable in '--mode', we had to deal with accepting two parameters at the same command line.
No, a disk can be in one of three modes - Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag It makes no sense to support both <shareable/> and <readonly/> in the same XML config at once. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, October 15, 2013 6:09 PM To: Chen Hanxiao
Do you mean that we should erase all about parameter 'shareable'? Not only '--shareable' but also in '--mode shareable'? If we decided to keep readonly and shareable in '--mode', we had to deal with accepting two parameters at the same command line.
No, a disk can be in one of three modes
- Exclusive read-write. This is the default - Shared read-write. This is the <shareable/> flag - Shared read-only. This is the <readonly/> flag
It makes no sense to support both <shareable/> and <readonly/> in the same XML config at once.
But we could do this with --sharebale --mode readonly or edit XMLs. I'll try to fix this conflict: let this could not happen in virsh command. Thanks!
Daniel
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> '--mode' option could set shareable tag already. We do not need to duplicate options. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 7 ------- tools/virsh.pod | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ba67a69..eb5ead1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -311,10 +311,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") @@ -625,9 +621,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 f6a80a3..8a22f23 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1916,7 +1916,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[,mode2]>] [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. @@ -1938,7 +1938,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 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
'--mode' option could set shareable tag already. We do not need to duplicate options.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- tools/virsh-domain.c | 7 ------- tools/virsh.pod | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ba67a69..eb5ead1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -311,10 +311,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") - },
We can't remove this whole part. The correct approach is to use correct flags to hide it.
{.name = "rawio", .type = VSH_OT_BOOL, .help = N_("needs rawio capability") @@ -625,9 +621,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) - virBufferAddLit(&buf, " <shareable/>\n"); -
NACK to this part. As DanPB mentioned, we need to remove the documentation, to discourage use of it, but we can't remove the argument as it would break backwards compatibility.
if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) { vshError(ctl, _("Invalid address."));
Peter
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:50 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable'
option
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
'--mode' option could set shareable tag already. We do not need to duplicate options.
}, - {.name = "shareable", - .type = VSH_OT_BOOL, - .help = N_("shareable between domains") - },
We can't remove this whole part. The correct approach is to use correct flags to hide it.
I checked enum ' Command Option Flags', it seems that we do not have an option for 'existed but not for use'. Could you please give me some hints for this?
{.name = "rawio", .type = VSH_OT_BOOL, .help = N_("needs rawio capability") @@ -625,9 +621,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) - virBufferAddLit(&buf, " <shareable/>\n"); -
NACK to this part. As DanPB mentioned, we need to remove the documentation, to discourage use of it, but we can't remove the argument as it would break backwards compatibility.
If --mode could do all the same job. I don't know why '--shareable' introduced. One scenario I can imagine is to set <readonly/> and <shareable/> in one command. As Daniel mentioned, --shareable may be mistakenly introduced. I think at least we should remove it from docs and command --help. So we didn't lose compatibility and do the right way of using this command. Thanks
if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) { vshError(ctl, _("Invalid address."));
Peter
On 10/15/13 11:09, Chen Hanxiao wrote:
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:50 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
'--mode' option could set shareable tag already. We do not need to duplicate options.
}, - {.name = "shareable", - .type = VSH_OT_BOOL, - .help = N_("shareable between domains") - },
We can't remove this whole part. The correct approach is to use correct flags to hide it.
I checked enum ' Command Option Flags', it seems that we do not have an option for 'existed but not for use'. Could you please give me some hints for this?
Hmmm, right. There probably isn't a suitable option as VSH_OT_ALIAS has to be used with the same type.
{.name = "rawio", .type = VSH_OT_BOOL, .help = N_("needs rawio capability") @@ -625,9 +621,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) - virBufferAddLit(&buf, " <shareable/>\n"); -
NACK to this part. As DanPB mentioned, we need to remove the documentation, to discourage use of it, but we can't remove the argument as it would break backwards compatibility.
If --mode could do all the same job. I don't know why '--shareable' introduced.
I don't know either. But it was introduced and released already, thus we can't kill it or we could possibly break someone who is already using it.
One scenario I can imagine is to set <readonly/> and <shareable/> in one command.
As Daniel mentioned, --shareable may be mistakenly introduced. I think at least we should remove it from docs and command --help. So we didn't lose compatibility and do the right way of using this command.
Well that's exactly what I was trying to say in my original review.
Thanks
if (straddr) { if (str2DiskAddress(straddr, &diskAddr) != 0) { vshError(ctl, _("Invalid address."));
Peter
On Tue, Oct 15, 2013 at 05:09:22PM +0800, Chen Hanxiao wrote:
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Tuesday, October 15, 2013 4:50 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
'--mode' option could set shareable tag already. We do not need to duplicate options.
}, - {.name = "shareable", - .type = VSH_OT_BOOL, - .help = N_("shareable between domains") - },
We can't remove this whole part. The correct approach is to use correct flags to hide it.
I checked enum ' Command Option Flags', it seems that we do not have an option for 'existed but not for use'. Could you please give me some hints for this?
{.name = "rawio", .type = VSH_OT_BOOL, .help = N_("needs rawio capability") @@ -625,9 +621,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (wwn) virBufferAsprintf(&buf, " <wwn>%s</wwn>\n", wwn);
- if (vshCommandOptBool(cmd, "shareable")) - virBufferAddLit(&buf, " <shareable/>\n"); -
NACK to this part. As DanPB mentioned, we need to remove the documentation, to discourage use of it, but we can't remove the argument as it would break backwards compatibility.
If --mode could do all the same job. I don't know why '--shareable' introduced.
'--shareable' is completely pointless, but we cannot remove it for backwards compatibility - only hide it from man pages and help output. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com]
If --mode could do all the same job. I don't know why '--shareable' introduced.
'--shareable' is completely pointless, but we cannot remove it for backwards compatibility - only hide it from man pages and help output.
Thanks for clarification. New patch will come soon.
Daniel --
participants (3)
-
Chen Hanxiao -
Daniel P. Berrange -
Peter Krempa