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

On 07/05/2011 01:25 AM, Hu Tao wrote: Nice. 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(a)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: 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: Hu Tao <hutao(a)cn.fujitsu.com&gt; 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 09:59:33AM +0100, Daniel P. Berrange wrote: Good idea! Will update it in v3. Thanks. -- Thanks, Hu Tao

On Tue, Jul 12, 2011 at 11:50:51AM -0600, Eric Blake wrote: Sounds good. Yes, the code should looks like this. I didn't noticed VSH_OFLAG_REQ(and friends). Will update in v3. -- Thanks, Hu Tao