[libvirt] [PATCH 0/6] qemu: hotplug: Kill duplicated code in disk hotplug (blockdev-add saga)
A lot of code was copypasted into the three disk attaching functions which are (almost) identical (one was forgotten), without refactoring it first. Let's reduce two copies of the ugly code. BTW this adds support for disk encryption and storage source authentication for USB disks. Peter Krempa (6): qemu: address: Remove dead code when un-reserving PCI address qemu: hotplug: Remove wrong check for empty disks qemu: hotplug: Use disk target in debug/warning messages where appropriate qemu: hotplug: extract disk hotplug worker code qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachSCSIDisk qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachUSBMassStorageDevice src/conf/domain_addr.c | 3 +- src/conf/domain_addr.h | 4 +- src/qemu/qemu_domain_address.c | 7 +- src/qemu/qemu_hotplug.c | 273 ++++++++--------------------------------- 4 files changed, 53 insertions(+), 234 deletions(-) -- 2.14.1
The code can't fail so having error handling is pointless. --- src/conf/domain_addr.c | 3 +-- src/conf/domain_addr.h | 4 ++-- src/qemu/qemu_domain_address.c | 7 ++----- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 531fc6800..642268239 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -687,12 +687,11 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, } -int +void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) { addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << addr->function); - return 0; } virDomainPCIAddressSetPtr diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 205e7cfe5..173101465 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -166,8 +166,8 @@ int virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr) +void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainPCIAddressSetAllMulti(virDomainDefPtr def) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b94b73eaa..7f4ac0f45 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2892,11 +2892,8 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias; - if (virDeviceInfoPCIAddressPresent(info) && - virDomainPCIAddressReleaseAddr(priv->pciaddrs, - &info->addr.pci) < 0) - VIR_WARN("Unable to release PCI address on %s", - NULLSTR(devstr)); + if (virDeviceInfoPCIAddressPresent(info)) + virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci); if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB && priv->usbaddrs && -- 2.14.1
The check if the disk is empty is wrong and would spuriously reject NBD sources. Remove it. --- src/qemu/qemu_hotplug.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1a0844701..6bec69a5d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -766,7 +766,6 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, char *devstr = NULL; bool driveAdded = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - const char *src = virDomainDiskGetSource(disk); bool releaseaddr = false; if (priv->usbaddrs) { @@ -778,13 +777,6 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; - /* XXX not correct once we allow attaching a USB CDROM */ - if (!src) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("disk source path is missing")); - goto error; - } - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; -- 2.14.1
Some messages deal with the disk itself thus using the disk target is better than using the disk source name which can be NULL in some cases. --- src/qemu/qemu_hotplug.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6bec69a5d..f00d4f546 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -364,7 +364,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, bool secobjAdded = false; bool encobjAdded = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; qemuDomainDiskPrivatePtr diskPriv; @@ -481,7 +480,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); if (releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &disk->info, src); + qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst); ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); goto cleanup; @@ -855,12 +854,11 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; int ret = -1; - const char *src = virDomainDiskGetSource(disk); if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported driver name '%s' for disk '%s'"), - virDomainDiskGetDriver(disk), src); + virDomainDiskGetDriver(disk), disk->dst); goto cleanup; } -- 2.14.1
This horrible piece of spaghetti code is copy-past(ae)d in the SCSI and USB disk hotplug code with minimal changes. Extract it for further reuse. --- src/qemu/qemu_hotplug.c | 50 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f00d4f546..f251af904 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -345,21 +345,24 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } +/** + * qemuDomainAttachDiskGeneric: + * + * Attaches disk to a VM. This function aggregates common code for all bus types. + * In cases when the VM crashed while adding the disk, -2 is returned. */ static int -qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, - virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk) +qemuDomainAttachDiskGeneric(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) { int ret = -1; int rv; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_DISK, { .disk = disk } }; virErrorPtr orig_err; char *devstr = NULL; char *drivestr = NULL; char *drivealias = NULL; - bool releaseaddr = false; bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; @@ -373,9 +376,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; - if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0) - goto error; - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -441,7 +441,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto exit_monitor; if (qemuDomainObjExitMonitor(driver, vm) < 0) { - releaseaddr = false; + ret = -2; goto error; } @@ -471,22 +471,42 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (encobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) - releaseaddr = false; + ret = -2; virErrorRestore(&orig_err); virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); - - if (releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst); - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); goto cleanup; } +static int +qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_DISK, { .disk = disk } }; + bool releaseaddr = false; + int rv; + + if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0) + return -1; + + if ((rv = qemuDomainAttachDiskGeneric(conn, driver, vm, disk)) < 0) { + if (rv == -1 && releaseaddr) + qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst); + + return -1; + } + + return 0; +} + + int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainControllerDefPtr controller) -- 2.14.1
Get rid of the first copy of the mess. --- src/qemu/qemu_hotplug.c | 127 ++---------------------------------------------- 1 file changed, 5 insertions(+), 122 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f251af904..299e25845 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -627,32 +627,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virDomainDiskDefPtr disk) { size_t i; - qemuDomainObjPrivatePtr priv = vm->privateData; - virErrorPtr orig_err; - char *drivestr = NULL; - char *devstr = NULL; - bool driveAdded = false; - bool encobjAdded = false; - bool secobjAdded = false; - char *drivealias = NULL; - int ret = -1; - int rv; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - virJSONValuePtr encobjProps = NULL; - virJSONValuePtr secobjProps = NULL; - qemuDomainDiskPrivatePtr diskPriv; - qemuDomainSecretInfoPtr encinfo; - qemuDomainSecretInfoPtr secinfo; - - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) - goto cleanup; /* We should have an address already, so make sure */ if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk address type %s"), virDomainDeviceAddressTypeToString(disk->info.type)); - goto error; + return -1; } /* Let's make sure the disk has a controller defined and loaded before @@ -664,111 +645,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, */ for (i = 0; i <= disk->info.addr.drive.controller; i++) { if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) - goto error; - } - - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; - - if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) - goto error; - - diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - secinfo = diskPriv->secinfo; - if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { - if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) - goto error; - } - - encinfo = diskPriv->encinfo; - if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) - goto error; - - if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; - - if (qemuDomainPrepareDiskSourceTLS(disk->src, disk->info.alias, cfg) < 0) - goto error; - - if (disk->src->haveTLS && - qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src, - disk->info.alias) < 0) - goto error; - - if (!(drivestr = qemuBuildDriveStr(disk, cfg, false, priv->qemuCaps))) - goto error; - - if (!(drivealias = qemuAliasFromDisk(disk))) - goto error; - - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) - goto error; - - qemuDomainObjEnterMonitor(driver, vm); - - if (secobjProps) { - rv = qemuMonitorAddObject(priv->mon, "secret", secinfo->s.aes.alias, - secobjProps); - secobjProps = NULL; /* qemuMonitorAddObject consumes */ - if (rv < 0) - goto exit_monitor; - secobjAdded = true; - } - - if (encobjProps) { - rv = qemuMonitorAddObject(priv->mon, "secret", encinfo->s.aes.alias, - encobjProps); - encobjProps = NULL; /* qemuMonitorAddObject consumes */ - if (rv < 0) - goto exit_monitor; - encobjAdded = true; - } - - if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) - goto exit_monitor; - driveAdded = true; - - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) - goto exit_monitor; - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto error; - - virDomainAuditDisk(vm, NULL, disk->src, "attach", true); - - virDomainDiskInsertPreAlloced(vm->def, disk); - ret = 0; - - cleanup: - virJSONValueFree(secobjProps); - virJSONValueFree(encobjProps); - qemuDomainSecretDiskDestroy(disk); - VIR_FREE(devstr); - VIR_FREE(drivestr); - VIR_FREE(drivealias); - virObjectUnref(cfg); - return ret; - - exit_monitor: - virErrorPreserveLast(&orig_err); - if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { - VIR_WARN("Unable to remove drive %s (%s) after failed " - "qemuMonitorAddDevice", drivealias, drivestr); + return -1; } - if (secobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); - if (encobjAdded) - ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - virErrorRestore(&orig_err); - virDomainAuditDisk(vm, NULL, disk->src, "attach", false); - - error: - qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); + if (qemuDomainAttachDiskGeneric(conn, driver, vm, disk) < 0) + return -1; - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); - goto cleanup; + return 0; } -- 2.14.1
Apart from killing a lot of code this also "implements" authentication and encryption for USB disks. --- src/qemu/qemu_hotplug.c | 84 +++++-------------------------------------------- 1 file changed, 7 insertions(+), 77 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 299e25845..5efc60aea 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -656,94 +656,24 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, static int -qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, +qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { qemuDomainObjPrivatePtr priv = vm->privateData; - virErrorPtr orig_err; - int ret = -1; - char *drivealias = NULL; - char *drivestr = NULL; - char *devstr = NULL; - bool driveAdded = false; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - bool releaseaddr = false; if (priv->usbaddrs) { if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0) - goto cleanup; - releaseaddr = true; + return -1; } - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) - goto cleanup; - - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; - - if (qemuDomainPrepareDiskSourceTLS(disk->src, disk->info.alias, cfg) < 0) - goto error; - - if (disk->src->haveTLS && - qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src, - disk->info.alias) < 0) - goto error; - - if (!(drivestr = qemuBuildDriveStr(disk, cfg, false, priv->qemuCaps))) - goto error; - - if (!(drivealias = qemuAliasFromDisk(disk))) - goto error; - - if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; - - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) - goto error; - - qemuDomainObjEnterMonitor(driver, vm); - - if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) - goto exit_monitor; - driveAdded = true; - - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) - goto exit_monitor; - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto error; - - virDomainAuditDisk(vm, NULL, disk->src, "attach", true); - - virDomainDiskInsertPreAlloced(vm->def, disk); - ret = 0; - - cleanup: - if (ret < 0 && releaseaddr) + if (qemuDomainAttachDiskGeneric(conn, driver, vm, disk) < 0) { virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); - VIR_FREE(devstr); - VIR_FREE(drivealias); - VIR_FREE(drivestr); - virObjectUnref(cfg); - return ret; - - exit_monitor: - virErrorPreserveLast(&orig_err); - if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { - VIR_WARN("Unable to remove drive %s (%s) after failed " - "qemuMonitorAddDevice", drivealias, drivestr); + return -1; } - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - virErrorRestore(&orig_err); - - virDomainAuditDisk(vm, NULL, disk->src, "attach", false); - error: - qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); - - ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); - goto cleanup; + return 0; } @@ -813,7 +743,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, _("disk device='lun' is not supported for usb bus")); break; } - ret = qemuDomainAttachUSBMassStorageDevice(driver, vm, disk); + ret = qemuDomainAttachUSBMassStorageDevice(conn, driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: -- 2.14.1
On Thu, Oct 19, 2017 at 03:16:32PM +0200, Peter Krempa wrote:
A lot of code was copypasted into the three disk attaching functions which are (almost) identical (one was forgotten), without refactoring it first. Let's reduce two copies of the ugly code. BTW this adds support for disk encryption and storage source authentication for USB disks.
Peter Krempa (6): qemu: address: Remove dead code when un-reserving PCI address qemu: hotplug: Remove wrong check for empty disks qemu: hotplug: Use disk target in debug/warning messages where appropriate qemu: hotplug: extract disk hotplug worker code qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachSCSIDisk qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachUSBMassStorageDevice
src/conf/domain_addr.c | 3 +- src/conf/domain_addr.h | 4 +- src/qemu/qemu_domain_address.c | 7 +- src/qemu/qemu_hotplug.c | 273 ++++++++--------------------------------- 4 files changed, 53 insertions(+), 234 deletions(-)
ACK series Jan
participants (2)
-
Ján Tomko -
Peter Krempa