[libvirt] [PATCH v3 1/2] vshCommandOptString returns -1 if option is empty and not VSH_OFLAG_EMPTY_OK
Pointed out by Eric. Thanks. --- tools/virsh.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b7cea58..b43af70 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12375,8 +12375,13 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) vshError(NULL, _("Missing required option '%s'"), name); ret = -1; } else { - /* Treat "--option ''" as if option had not been specified. */ - ret = 0; + /* --option '' */ + if (arg->def->flag & VSH_OFLAG_EMPTY_OK) { + ret = 0; + } else { + vshError(NULL, _("option '%s' is empty"), name); + ret = -1; + } } } -- 1.7.5.1
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. --address option allows user to specify address of disk device when attaching a disk device. --- tools/virsh.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 10 ++- 2 files changed, 216 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b43af70..aa7fb56 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10168,19 +10168,172 @@ 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")}, + {"address", VSH_OT_STRING, 0, N_("address of disk device")}, {NULL, 0, 0, NULL} }; +enum { + DISK_ADDR_TYPE_INVALID, + DISK_ADDR_TYPE_PCI, + DISK_ADDR_TYPE_SCSI, + DISK_ADDR_TYPE_IDE, +}; + +struct PCIAddress { + unsigned int domain; + unsigned int bus; + unsigned int slot; + unsigned int function; +}; + +struct SCSIAddress { + unsigned int controller; + unsigned int bus; + unsigned int unit; +}; + +struct IDEAddress { + unsigned int controller; + unsigned int bus; + unsigned int unit; +}; + +struct DiskAddress { + int type; + union { + struct PCIAddress pci; + struct SCSIAddress scsi; + struct IDEAddress ide; + } addr; +}; + +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 int str2SCSIAddress(const char *str, struct SCSIAddress *scsiAddr) +{ + char *controller, *bus, *unit; + + if (!scsiAddr) + return -1; + if (!str) + return -1; + + controller = (char *)str; + + if (virStrToLong_ui(controller, &bus, 0, &scsiAddr->controller) != 0) + return -1; + + bus++; + if (virStrToLong_ui(bus, &unit, 0, &scsiAddr->bus) != 0) + return -1; + + unit++; + if (virStrToLong_ui(unit, NULL, 0, &scsiAddr->unit) != 0) + return -1; + + return 0; +} + +static int str2IDEAddress(const char *str, struct IDEAddress *ideAddr) +{ + char *controller, *bus, *unit; + + if (!ideAddr) + return -1; + if (!str) + return -1; + + controller = (char *)str; + + if (virStrToLong_ui(controller, &bus, 0, &ideAddr->controller) != 0) + return -1; + + bus++; + if (virStrToLong_ui(bus, &unit, 0, &ideAddr->bus) != 0) + return -1; + + unit++; + if (virStrToLong_ui(unit, NULL, 0, &ideAddr->unit) != 0) + return -1; + + return 0; +} + +/* pci address pci:0000.00.0x0a.0 (domain:bus:slot:function) + * ide disk address: ide:00.00.0 (controller:bus:unit) + * scsi disk address: scsi:00.00.0 (controller:bus:unit) + */ + +static int str2DiskAddress(const char *str, struct DiskAddress *diskAddr) +{ + char *type, *addr; + + if (!diskAddr) + return -1; + if (!str) + return -1; + + type = (char *)str; + addr = strchr(type, ':'); + if (!addr) + return -1; + + if (STREQLEN(type, "pci", addr - type)) { + diskAddr->type = DISK_ADDR_TYPE_PCI; + return str2PCIAddress(addr + 1, &diskAddr->addr.pci); + } else if (STREQLEN(type, "scsi", addr - type)) { + diskAddr->type = DISK_ADDR_TYPE_SCSI; + return str2SCSIAddress(addr + 1, &diskAddr->addr.scsi); + } else if (STREQLEN(type, "ide", addr - type)) { + diskAddr->type = DISK_ADDR_TYPE_IDE; + return str2IDEAddress(addr + 1, &diskAddr->addr.ide); + } + + return -1; +} + 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, *straddr = NULL; + struct DiskAddress diskAddr; bool isFile = false, functionReturn = false; int ret; unsigned int flags; @@ -10204,6 +10357,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) vshCommandOptString(cmd, "subdriver", &subdriver) < 0 || vshCommandOptString(cmd, "type", &type) < 0 || vshCommandOptString(cmd, "mode", &mode) < 0 || + vshCommandOptString(cmd, "cache", &cache) < 0 || + vshCommandOptString(cmd, "serial", &serial) < 0 || + vshCommandOptString(cmd, "address", &straddr) < 0 || vshCommandOptString(cmd, "sourcetype", &stype) < 0) { vshError(ctl, "%s", _("missing option")); goto cleanup; @@ -10241,8 +10397,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 +10410,54 @@ 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 (straddr) { + if (str2DiskAddress(straddr, &diskAddr) != 0) { + vshError(ctl, _("Invalid address.")); + goto cleanup; + } + + if (STRPREFIX((const char *)target, "vd")) { + if (diskAddr.type == DISK_ADDR_TYPE_PCI) { + virBufferAsprintf(&buf, + " <address type='pci' domain='0x%04x'" + " bus ='0x%02x' slot='0x%02x' function='0x%0x' />\n", + diskAddr.addr.pci.domain, diskAddr.addr.pci.bus, + diskAddr.addr.pci.slot, diskAddr.addr.pci.function); + } else { + vshError(ctl, "%s", _("expecting a pci:0000.00.00.00 address.")); + goto cleanup; + } + } else if (STRPREFIX((const char *)target, "sd")) { + if (diskAddr.type == DISK_ADDR_TYPE_SCSI) { + virBufferAsprintf(&buf, + " <address type='drive' controller='%d'" + " bus='%d' unit='%d' />\n", + diskAddr.addr.scsi.controller, diskAddr.addr.scsi.bus, + diskAddr.addr.scsi.unit); + } else { + vshError(ctl, "%s", _("expecting a scsi:00.00.00 address.")); + goto cleanup; + } + } else if (STRPREFIX((const char *)target, "hd")) { + if (diskAddr.type == DISK_ADDR_TYPE_IDE) { + virBufferAsprintf(&buf, + " <address type='drive' controller='%d'" + " bus='%d' unit='%d' />\n", + diskAddr.addr.ide.controller, diskAddr.addr.ide.bus, + diskAddr.addr.ide.unit); + } else { + vshError(ctl, "%s", _("expecting an ide:00.00.00 address.")); + goto cleanup; + } + } + } + virBufferAddLit(&buf, "</disk>\n"); if (virBufferError(&buf)) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 52f1549..a285863 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -877,8 +877,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<--address address> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. @@ -889,6 +890,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<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. =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/15/2011 01:06 AM, Hu Tao wrote:
This add three options for virsh command attach-disk.
s/add three/adds four/
--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 here,
sharable between domains when attaching a disk device from virsh command line.
--address option allows user to specify address of disk device when attaching a disk device. --- tools/virsh.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 10 ++- 2 files changed, 216 insertions(+), 4 deletions(-)
I think this patch can be applied prior to cleaning up the vshCommandOptString mess (since that affects more than just your change).
diff --git a/tools/virsh.c b/tools/virsh.c index b43af70..aa7fb56 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10168,19 +10168,172 @@ 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")},
but shareable here. Which do we want? Both are valid in the English dictionary (gotta love that), but google fight picks shareable: http://www.googlefight.com/index.php?lang=en_GB&word1=sharable&word2=shareable as does our XML schema. Also, 'unsharable' makes me think of the shar(1) utility. I've adjusted the commit message accordingly, so ACK and pushed. Meanwhile, I'm followup up with this one under the trivial rule: diff --git i/docs/internals/locking.html.in w/docs/internals/locking.html.in index 3790ef0..e84321b 100644 --- i/docs/internals/locking.html.in +++ w/docs/internals/locking.html.in @@ -15,7 +15,7 @@ <p> The high level goal is to prevent the same disk image being used by more than one QEMU instance at a time (unless the - disk is marked as sharable, or readonly). The scenarios + disk is marked as shareable, or readonly). The scenarios to be prevented are thus: </p> diff --git i/src/locking/lock_driver_sanlock.c w/src/locking/lock_driver_sanlock.c index 87e7815..62eb28b 100644 --- i/src/locking/lock_driver_sanlock.c +++ w/src/locking/lock_driver_sanlock.c @@ -634,7 +634,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, } if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) { virLockError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Sharable leases are not supported")); + _("Shareable leases are not supported")); return -1; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/15/2011 01:06 AM, Hu Tao wrote:
Pointed out by Eric. Thanks. --- tools/virsh.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index b7cea58..b43af70 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12375,8 +12375,13 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) vshError(NULL, _("Missing required option '%s'"), name); ret = -1; } else { - /* Treat "--option ''" as if option had not been specified. */ - ret = 0; + /* --option '' */ + if (arg->def->flag & VSH_OFLAG_EMPTY_OK) {
We can't ever get here. We already set ret = 1 earlier in the function in that scenario. Wow, reading through virsh.c, I noticed some more dead code. All instances of vshCmdOpt are created with a non-NULL arg->def, yet we check for non-NULL arg->def in a lot of places. I think I'll work on a replacement patch that fixes this issue and more. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/15/2011 09:45 AM, Eric Blake wrote:
On 07/15/2011 01:06 AM, Hu Tao wrote:
Pointed out by Eric. Thanks. --- tools/virsh.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index b7cea58..b43af70 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12375,8 +12375,13 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) vshError(NULL, _("Missing required option '%s'"), name); ret = -1; } else { - /* Treat "--option ''" as if option had not been specified. */ - ret = 0; + /* --option '' */ + if (arg->def->flag & VSH_OFLAG_EMPTY_OK) {
We can't ever get here. We already set ret = 1 earlier in the function in that scenario.
Wow, reading through virsh.c, I noticed some more dead code. All instances of vshCmdOpt are created with a non-NULL arg->def, yet we check for non-NULL arg->def in a lot of places. I think I'll work on a replacement patch that fixes this issue and more.
Done here: https://www.redhat.com/archives/libvir-list/2011-July/msg00997.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Hu Tao