8:16 a.m.
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
8:16 a.m.
New subject: [libvirt] [PATCH 1/6] qemu: address: Remove dead code when un-reserving PCI address
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
8:16 a.m.
New subject: [libvirt] [PATCH 2/6] qemu: hotplug: Remove wrong check for empty disks
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
8:16 a.m.
New subject: [libvirt] [PATCH 3/6] qemu: hotplug: Use disk target in debug/warning messages where appropriate
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
8:16 a.m.
New subject: [libvirt] [PATCH 4/6] qemu: hotplug: extract disk hotplug worker code
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
8:16 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachSCSIDisk
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
8:16 a.m.
New subject: [libvirt] [PATCH 6/6] qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachUSBMassStorageDevice
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
8:38 a.m.
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
2591
days inactive
2591
days old
7 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa