[libvirt] [PATCH 1/3] add --cache option for attach-disk
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")}, {"type", VSH_OT_STRING, 0, N_("target device type")}, {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, @@ -10188,7 +10189,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *source = NULL, *target = NULL, *driver = NULL, - *subdriver = NULL, *type = NULL, *mode = NULL; + *subdriver = NULL, *type = NULL, *mode = NULL, + *cache = NULL; bool isFile = false, functionReturn = false; int ret; unsigned int flags; @@ -10217,6 +10219,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + ignore_value(vshCommandOptString(cmd, "cache", &cache)); + if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) isFile = true; @@ -10249,8 +10253,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, " name='%s'", driver); if (subdriver) virBufferAsprintf(&buf, " type='%s'", subdriver); + if (cache) + virBufferAsprintf(&buf, " cache='%s'", cache); - if (driver || subdriver) + if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n"); virBufferAsprintf(&buf, " <source %s='%s'/>\n", diff --git a/tools/virsh.pod b/tools/virsh.pod index 736b919..79f49ba 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -874,8 +874,8 @@ For cdrom and floppy devices, this command only replaces the media within the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional -I<--driver driver> I<--subdriver subdriver> I<--type type> -I<--mode mode> I<--persistent> I<--sourcetype soucetype> +I<--driver driver> I<--subdriver subdriver> I<--cache cache> +I<--type type> I<--mode mode> I<--persistent> I<--sourcetype soucetype> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -886,6 +886,7 @@ floppy device; consider using B<update-device> for this usage instead. I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<persistent> indicates the changes will affect the next boot of the domain. I<sourcetype> can indicate the type of source (block|file) +I<cache> can be one of "default", "none", "writethrough" or "writeback". =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> I<--model model> -- 1.7.5.1
Add --serial option for attach-disk to allow user to specify serial string of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 7 ++++++- tools/virsh.pod | 2 ++ 2 files changed, 8 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index cab5a35..c30f333 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10181,6 +10181,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, {"sourcetype", VSH_OT_STRING, 0, N_("type of source (block|file)")}, + {"serial", VSH_OT_STRING, 0, N_("serial of disk device")}, {NULL, 0, 0, NULL} }; @@ -10190,7 +10191,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *source = NULL, *target = NULL, *driver = NULL, *subdriver = NULL, *type = NULL, *mode = NULL, - *cache = NULL; + *cache = NULL, *serial = NULL; bool isFile = false, functionReturn = false; int ret; unsigned int flags; @@ -10220,6 +10221,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) } ignore_value(vshCommandOptString(cmd, "cache", &cache)); + ignore_value(vshCommandOptString(cmd, "serial", &serial)); if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) @@ -10266,6 +10268,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); + if (serial) + virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); + virBufferAddLit(&buf, "</disk>\n"); if (virBufferError(&buf)) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 79f49ba..95e7744 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -876,6 +876,7 @@ the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional I<--driver driver> I<--subdriver subdriver> I<--cache cache> I<--type type> I<--mode mode> I<--persistent> I<--sourcetype soucetype> +I<--serial serial> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -887,6 +888,7 @@ I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<persistent> indicates the changes will affect the next boot of the domain. I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough" or "writeback". +I<serial> is the serial of disk device. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> I<--model model> -- 1.7.5.1
Add --sharable option for attach-disk to allow user to specify whether the disk device is sharable between domains when attaching a disk device from virsh command line. --- tools/virsh.c | 4 ++++ tools/virsh.pod | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index c30f333..095eede 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10182,6 +10182,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, {"sourcetype", VSH_OT_STRING, 0, N_("type of source (block|file)")}, {"serial", VSH_OT_STRING, 0, N_("serial of disk device")}, + {"shareable", VSH_OT_BOOL, 0, N_("shareable between domains")}, {NULL, 0, 0, NULL} }; @@ -10271,6 +10272,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (serial) virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); + if (vshCommandOptBool(cmd, "shareable")) + virBufferAsprintf(&buf, " <shareable/>\n"); + virBufferAddLit(&buf, "</disk>\n"); if (virBufferError(&buf)) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 95e7744..401f3c6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -876,7 +876,7 @@ the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional I<--driver driver> I<--subdriver subdriver> I<--cache cache> I<--type type> I<--mode mode> I<--persistent> I<--sourcetype soucetype> -I<--serial serial> +I<--serial serial> I<--shareable> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -888,7 +888,8 @@ I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<persistent> indicates the changes will affect the next boot of the domain. I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough" or "writeback". -I<serial> is the serial of disk device. +I<serial> is the serial of disk device. I<shareable> indicates the disk device +is shareable between domains. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> I<--model model> -- 1.7.5.1
On 07/05/2011 01:25 AM, Hu Tao wrote:
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")},
Nice.
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice. --cache '' will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API. Patch 2/3 shares the same problem. And since all three patches are touching the same 'virsh attach-disk' command, I'd be fine if you wanted to squash them into a single patch for v2. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jul 05, 2011 at 11:58:31AM -0600, Eric Blake wrote:
On 07/05/2011 01:25 AM, Hu Tao wrote:
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")},
Nice.
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
Thanks for reply. Will fix in v2.
Patch 2/3 shares the same problem. And since all three patches are touching the same 'virsh attach-disk' command, I'd be fine if you wanted to squash them into a single patch for v2.
ok. -- Thanks, Hu Tao
On Tue, Jul 05, 2011 at 11:58:31AM -0600, Eric Blake wrote:
On 07/05/2011 01:25 AM, Hu Tao wrote:
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")},
Nice.
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
I found that in the case of --cache '' vshCommandOptString returns 0, with cache(3rd parameter) unchanged. So can we safely ignore value or do I miss something? This v2 also adds another new option, --pciaddress.
From 3e3473baf24f1b6ff014cc26f05271a2da87d12e Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Tue, 12 Jul 2011 15:19:42 +0800 Subject: [PATCH] add --cache, --serial, --sharable and --pciaddress options for attach-disk
This add three options for virsh command attach-disk. --cache option allows user to specify cache mode of disk device from virsh command line when attaching a disk device. --serial option allows user to specify serial string of disk device from virsh command line when attaching a disk device. --sharable option allows user to specify whether the disk device is sharable between domains when attaching a disk device from virsh command line. --pciaddress option allows user to specify pci address of disk controller when attaching a disk device. --- tools/virsh.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 10 +++++- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3cdf043..1fa3b98 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10168,19 +10168,64 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")}, {"type", VSH_OT_STRING, 0, N_("target device type")}, {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, {"sourcetype", VSH_OT_STRING, 0, N_("type of source (block|file)")}, + {"serial", VSH_OT_STRING, 0, N_("serial of disk device")}, + {"shareable", VSH_OT_BOOL, 0, N_("shareable between domains")}, + {"pciaddress", VSH_OT_STRING, 0, N_("pci address")}, {NULL, 0, 0, NULL} }; +struct PCIAddress { + unsigned int domain; + unsigned int bus; + unsigned int slot; + unsigned int function; +}; + + +/* pciaddress 0000:00:0a:0 (domain:bus:slot:function) */ + +static int str2PCIAddress(const char *str, struct PCIAddress *pciAddr) +{ + char *domain, *bus, *slot, *function; + + if (!pciAddr) + return -1; + if (!str) + return -1; + + domain = (char *)str; + + if (virStrToLong_ui(domain, &bus, 0, &pciAddr->domain) != 0) + return -1; + + bus++; + if (virStrToLong_ui(bus, &slot, 0, &pciAddr->bus) != 0) + return -1; + + slot++; + if (virStrToLong_ui(slot, &function, 0, &pciAddr->slot) != 0) + return -1; + + function++; + if (virStrToLong_ui(function, NULL, 0, &pciAddr->function) != 0) + return -1; + + return 0; +} + static bool cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *source = NULL, *target = NULL, *driver = NULL, - *subdriver = NULL, *type = NULL, *mode = NULL; + *subdriver = NULL, *type = NULL, *mode = NULL, + *cache = NULL, *serial = NULL, *strpciaddr = NULL; + struct PCIAddress pciAddr; bool isFile = false, functionReturn = false; int ret; unsigned int flags; @@ -10209,6 +10254,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + ignore_value(vshCommandOptString(cmd, "cache", &cache)); + ignore_value(vshCommandOptString(cmd, "serial", &serial)); + ignore_value(vshCommandOptString(cmd, "pciaddress", &strpciaddr)); + if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) isFile = true; @@ -10241,8 +10290,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, " name='%s'", driver); if (subdriver) virBufferAsprintf(&buf, " type='%s'", subdriver); + if (cache) + virBufferAsprintf(&buf, " cache='%s'", cache); - if (driver || subdriver) + if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n"); virBufferAsprintf(&buf, " <source %s='%s'/>\n", @@ -10252,6 +10303,29 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); + if (serial) + virBufferAsprintf(&buf, " <serial>%s</serial>\n", serial); + + if (vshCommandOptBool(cmd, "shareable")) + virBufferAsprintf(&buf, " <shareable/>\n"); + + if (strpciaddr) { + if (!STRPREFIX((const char *)target, "vd")) { + vshError(ctl, _("PCI address can be set only for vdx disks.")); + goto cleanup; + } + if (str2PCIAddress(strpciaddr, &pciAddr) == 0) { + virBufferAsprintf(&buf, + " <address type='pci' domain='0x%04x'" + " bus ='0x%02x' slot='0x%02x' function='0x%0x' />\n", + pciAddr.domain, pciAddr.bus, + pciAddr.slot, pciAddr.function); + } else { + vshError(ctl, _("Invalid PCI address.")); + goto cleanup; + } + } + virBufferAddLit(&buf, "</disk>\n"); if (virBufferError(&buf)) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 8b820d2..306f52e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -876,8 +876,9 @@ For cdrom and floppy devices, this command only replaces the media within the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional -I<--driver driver> I<--subdriver subdriver> I<--type type> -I<--mode mode> I<--persistent> I<--sourcetype soucetype> +I<--driver driver> I<--subdriver subdriver> I<--cache cache> +I<--type type> I<--mode mode> I<--persistent> I<--sourcetype soucetype> +I<--serial serial> I<--shareable> I<--pciaddress pciaddress> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -888,6 +889,11 @@ floppy device; consider using B<update-device> for this usage instead. I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<persistent> indicates the changes will affect the next boot of the domain. I<sourcetype> can indicate the type of source (block|file) +I<cache> can be one of "default", "none", "writethrough" or "writeback". +I<serial> is the serial of disk device. I<shareable> indicates the disk device +is shareable between domains. +I<pciaddress> is the pci address of disk controller. This option can be set +only for vdx disks. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> I<--model model> -- 1.7.5.1
On Tue, Jul 12, 2011 at 03:47:36PM +0800, Hu Tao wrote:
On Tue, Jul 05, 2011 at 11:58:31AM -0600, Eric Blake wrote:
On 07/05/2011 01:25 AM, Hu Tao wrote:
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")},
Nice.
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
I found that in the case of --cache '' vshCommandOptString returns 0, with cache(3rd parameter) unchanged. So can we safely ignore value or do I miss something?
This v2 also adds another new option, --pciaddress.
From 3e3473baf24f1b6ff014cc26f05271a2da87d12e Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Tue, 12 Jul 2011 15:19:42 +0800 Subject: [PATCH] add --cache, --serial, --sharable and --pciaddress options for attach-disk
This add three options for virsh command attach-disk.
--cache option allows user to specify cache mode of disk device from virsh command line when attaching a disk device.
--serial option allows user to specify serial string of disk device from virsh command line when attaching a disk device.
--sharable option allows user to specify whether the disk device is sharable between domains when attaching a disk device from virsh command line.
--pciaddress option allows user to specify pci address of disk controller when attaching a disk device.
"--pciaddress" would only be useful for certain types of disks which are directly attached to the PCI bus. it wouldn't work for IDE, SCSI or USB. IMHO we should have a more general '--address' argument which takes a formatted string '--address pci:1.2.3.4' '--address ide:1.2' '--address scsi:1.2.3' etc, 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 :|
On Tue, Jul 12, 2011 at 09:59:33AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 12, 2011 at 03:47:36PM +0800, Hu Tao wrote:
On Tue, Jul 05, 2011 at 11:58:31AM -0600, Eric Blake wrote:
On 07/05/2011 01:25 AM, Hu Tao wrote:
Add --cache option to allow user to specify cache mode of disk device from virsh command line when attaching a disk device. --- tools/virsh.c | 10 ++++++++-- tools/virsh.pod | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 9a189fd..cab5a35 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10176,6 +10176,7 @@ static const vshCmdOptDef opts_attach_disk[] = { {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"cache", VSH_OT_STRING, 0, N_("cache mode of disk device")},
Nice.
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
I found that in the case of --cache '' vshCommandOptString returns 0, with cache(3rd parameter) unchanged. So can we safely ignore value or do I miss something?
This v2 also adds another new option, --pciaddress.
From 3e3473baf24f1b6ff014cc26f05271a2da87d12e Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Tue, 12 Jul 2011 15:19:42 +0800 Subject: [PATCH] add --cache, --serial, --sharable and --pciaddress options for attach-disk
This add three options for virsh command attach-disk.
--cache option allows user to specify cache mode of disk device from virsh command line when attaching a disk device.
--serial option allows user to specify serial string of disk device from virsh command line when attaching a disk device.
--sharable option allows user to specify whether the disk device is sharable between domains when attaching a disk device from virsh command line.
--pciaddress option allows user to specify pci address of disk controller when attaching a disk device.
"--pciaddress" would only be useful for certain types of disks which are directly attached to the PCI bus. it wouldn't work for IDE, SCSI or USB. IMHO we should have a more general '--address' argument which takes a formatted string
'--address pci:1.2.3.4' '--address ide:1.2' '--address scsi:1.2.3'
Good idea! Will update it in v3. Thanks. -- Thanks, Hu Tao
On 07/12/2011 01:47 AM, Hu Tao wrote:
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
I found that in the case of --cache '' vshCommandOptString returns 0, with cache(3rd parameter) unchanged. So can we safely ignore value or do I miss something?
vshCommandOptString currently returns: 1 if the option was provided and non-empty, or provided and empty and VSH_OFLAG_EMPTY_OK 0 if the option was not provided, or provided but empty -1 if the option was not provided but VSH_OFLAG_REQ --cache isn't marked VSH_OFLAG_REQ, so we won't get -1. And we didn't pass VSH_OFLAG_EMPTY_OK, so an empty string returns 0 instead of 1, leaving the variable NULL the same as if --cache had not been provided. But your code would be the first instance of using ignore_value on vshCommandOptString. I'm starting to think that's a bug in the implementation, and that vshCommandOptString should instead return: 1 if the option was provided and non-empty, or provided and empty and VSH_OFLAG_EMPTY_OK 0 if the option was not provided -1 if the option was not provided but VSH_OFLAG_REQ, or provided and empty and not VSH_OFLAG_EMPTY_OK in which case, it _does_ become important to check for errors.
@@ -10209,6 +10254,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ ignore_value(vshCommandOptString(cmd, "cache", &cache)); + ignore_value(vshCommandOptString(cmd, "serial", &serial)); + ignore_value(vshCommandOptString(cmd, "pciaddress", &strpciaddr));
At any rate, we already have lots of existing code that looks like: if (vshCommandOptString(cmd, "cache", &cache) < 0 || vshCommandOptString(cmd, "serial", &serial) < 0 || vshCommandOptString(cmd, "pciaddress", &strpciaddr) < 0) { vshError(ctl, "%s", _("missing argument")); goto out; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jul 12, 2011 at 11:50:51AM -0600, Eric Blake wrote:
On 07/12/2011 01:47 AM, Hu Tao wrote:
+ ignore_value(vshCommandOptString(cmd, "cache", &cache));
Not so nice.
--cache ''
will make vshCommandOptString return -1, because that usage is a virsh usage error and should be diagnosed as such up front, rather than accidentally passing cache='' through the XML to the libvirt API.
I found that in the case of --cache '' vshCommandOptString returns 0, with cache(3rd parameter) unchanged. So can we safely ignore value or do I miss something?
vshCommandOptString currently returns: 1 if the option was provided and non-empty, or provided and empty and VSH_OFLAG_EMPTY_OK 0 if the option was not provided, or provided but empty -1 if the option was not provided but VSH_OFLAG_REQ
--cache isn't marked VSH_OFLAG_REQ, so we won't get -1. And we didn't pass VSH_OFLAG_EMPTY_OK, so an empty string returns 0 instead of 1, leaving the variable NULL the same as if --cache had not been provided.
But your code would be the first instance of using ignore_value on vshCommandOptString. I'm starting to think that's a bug in the implementation, and that vshCommandOptString should instead return:
1 if the option was provided and non-empty, or provided and empty and VSH_OFLAG_EMPTY_OK 0 if the option was not provided -1 if the option was not provided but VSH_OFLAG_REQ, or provided and empty and not VSH_OFLAG_EMPTY_OK
Sounds good.
in which case, it _does_ become important to check for errors.
@@ -10209,6 +10254,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ ignore_value(vshCommandOptString(cmd, "cache", &cache)); + ignore_value(vshCommandOptString(cmd, "serial", &serial)); + ignore_value(vshCommandOptString(cmd, "pciaddress", &strpciaddr));
At any rate, we already have lots of existing code that looks like:
if (vshCommandOptString(cmd, "cache", &cache) < 0 || vshCommandOptString(cmd, "serial", &serial) < 0 || vshCommandOptString(cmd, "pciaddress", &strpciaddr) < 0) { vshError(ctl, "%s", _("missing argument")); goto out; }
Yes, the code should looks like this. I didn't noticed VSH_OFLAG_REQ(and friends). Will update in v3. -- Thanks, Hu Tao
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao