10:54 p.m.
From: Chen Hanxiao <chenhanxiao(a)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
10:54 p.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
From: Chen Hanxiao <chenhanxiao(a)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(a)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
3:58 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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(a)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
4:41 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
-----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(a)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
4:48 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
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(a)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
5:13 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
-----Original Message-----
From: Peter Krempa [mailto:pkrempa@redhat.com]
Sent: Tuesday, October 15, 2013 5:48 PM
To: Chen Hanxiao; libvir-list(a)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:
>
>
>> -----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(a)cn.fujitsu.com>
>> 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.
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
4:42 a.m.
New subject: [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(a)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 :|
5:06 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
-----Original Message-----
From: Daniel P. Berrange [mailto:berrange@redhat.com]
Sent: Tuesday, October 15, 2013 5:42 PM
To: Chen Hanxiao
Cc: libvir-list(a)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(a)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
--
5:08 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
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(a)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(a)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 :|
5:19 a.m.
New subject: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
-----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
10:54 p.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
'--mode' option could set shareable tag already.
We do not need to duplicate options.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
3:50 a.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
On 10/15/13 05:54, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
'--mode' option could set shareable tag already.
We do not need to duplicate options.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
4:09 a.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
-----Original Message-----
From: Peter Krempa [mailto:pkrempa@redhat.com]
Sent: Tuesday, October 15, 2013 4:50 PM
To: Chen Hanxiao; libvir-list(a)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(a)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
4:18 a.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
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(a)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(a)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
4:43 a.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
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(a)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(a)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 :|
4:55 a.m.
New subject: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option
-----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
--
4042
days inactive
4042
days old
15 comments
3 participants
participants (3)
-
Chen Hanxiao -
Daniel P. Berrange -
Peter Krempa