Daniel P. Berrange wrote:

On Tue, May 04, 2010 at 05:18:24PM +0200, Wolfgang Mauerer wrote:

...

Detaching disk devices is not only possible for VIR_DOMAIN_DISK_BUS_VIRTIO, but also for VIR_DOMAIN_DISK_BUS_SCSI, so take care of this possibility.

Additionally, when the new-style device syntax is used, we do not need to check if the PCI address is valid since we don't need it to do the hot-unplugging. And while we're at it, drop the "pci" part in qemudDomainDetachPciDiskDevice() -- it's misleading since we do not necessarily have to deal with PCI addresses.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- src/qemu/qemu_driver.c | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 704f824..f2b8517 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7862,10 +7862,10 @@ cleanup: }

-static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - unsigned long long qemuCmdFlags) +static int qemudDomainDetachDiskDevice(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + unsigned long long qemuCmdFlags)

I'd rather we introduced a separate method

qemudDomainDetachSCSIDiskDevice

since logically these are different types of objects/operations, that just happen to share a common monitor command with new QEMU. It also needs to give a more explicit error in the case where QEMUD_CMD_FLAG_DEVICE is not set for SCSI, since we can't support that for SCSI, but can for VirtIO.

okay, I've created a separate function for SCSI disk unplug. Since the differences between VirtIO and SCSI disk unplug are tiny, I've tried to extract at least a few commonalities -- see the two follow-up patches. Albeit there's much more duplicated code lurking in qemu_driver.c, it might make sense to look at this some day. Best, Wolfgang

On 05/05/2010 08:52 AM, Wolfgang Mauerer wrote:

We can reuse some of the code for other purposes.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 47ae52c..63ca57c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7867,6 +7867,36 @@ cleanup: }

+static inline int qemudFindDisk(virDomainDefPtr def, char *dst)

dst can be const char *.

+{ + int i; + + for (i = 0 ; i < def->ndisks ; i++) { + if (STREQ(def->disks[i]->dst, dst)) { + return i; + } + } + + return -1; +} + +static inline void qemudShrinkDisks(virDomainDefPtr def, int i)

And i can be unsigned (better yet, size_t).

+{ + if (def->ndisks > 1) { + memmove(def->disks + i, + def->disks + i + 1, + sizeof(*def->disks) * + (def->ndisks - (i + 1))); + def->ndisks--; + if (VIR_REALLOC_N(def->disks, def->ndisks) < 0) { + /* ignore, harmless */ + } + } else { + VIR_FREE(def->disks); + def->ndisks = 0; + } +}

But since this is just code motion for future use, it looks fine to me. ACK, and I went ahead and pushed it with those edits. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

With the introduction of the generic qemu device model, unplugging SCSI disks works like a charm, so support it in libvirt. * src/qemu/qemu_driver.c: Add qemudDomainDetachSCSIDiskDevice() to do the unplugging, extend qemudDomainDetachDeviceAdd(). Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 63ca57c..7321960 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7940,6 +7940,51 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver, qemudShrinkDisks(vm->def, i); + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(vm, dev->data.disk) < 0) + VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + + ret = 0; + +cleanup: + return ret; +} + +static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + unsigned long long qemuCmdFlags) +{ + int i, ret = -1; + virDomainDiskDefPtr detach = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + i = qemudFindDisk(vm->def, dev->data.disk->dst); + + if (i < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + goto cleanup; + } + + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("Underlying qemu does not support SCSI disk removal")); + goto cleanup; + } + + detach = vm->def->disks[i]; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { + qemuDomainObjExitMonitor(vm); + goto cleanup; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + + qemudShrinkDisks(vm->def, i); + virDomainDiskDefFree(detach); if (driver->securityDriver && @@ -8393,9 +8438,18 @@ static int qemudDomainDetachDevice(virDomainPtr dom, goto endjob; if (dev->type == VIR_DOMAIN_DEVICE_DISK && - dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && - dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { - ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags); + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + ret = qemudDomainDetachPciDiskDevice(driver, vm, dev, qemuCmdFlags); + } + else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + ret = qemudDomainDetachSCSIDiskDevice(driver, vm, dev, + qemuCmdFlags); + } + else { + qemuReportError(VIR_ERR_NO_SUPPORT, + "%s", _("This type of disk cannot be hot unplugged")); + } } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainDetachNetDevice(driver, vm, dev, qemuCmdFlags); } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { -- 1.6.4