[libvirt] [PATCH 00/11] Fix disk dst/wwn/serial checks in hotplug case
Peter Krempa (11): qemu: hotplug: Use typecasted switch qemu: hotplug: Remove unnecessary variable qemu: hotplug: Break up if/else statement into switch qemu: hotplug: Use more common 'cleanup' label in qemuDomainAttachDeviceDiskLive qemu: hotplug: Extract common code to qemuDomainAttachDeviceDiskLive conf: Extract code that checks disk serial/wwn conflict qemu: hotplug: Check duplicate disk serial/wwn on hotplug too qemu: process: Reorder operations on early VM startup qemu: process: Extract pre-start checks into a function tests: Integrate startup checks to qemuxml2argvtest conf: Move and optimize disk target duplicity checking src/conf/domain_conf.c | 82 +++++++++++++++++++-------------------------- src/conf/domain_conf.h | 4 ++- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_hotplug.c | 85 +++++++++++++++++++++-------------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 57 +++++++++++++++++++++---------- src/qemu/qemu_process.h | 9 ++++- tests/qemuxml2argvtest.c | 13 ++++++-- 9 files changed, 136 insertions(+), 121 deletions(-) -- 2.6.2
Remove the default case since all cases are covered. --- src/qemu/qemu_hotplug.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f8db960..b456fed 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -795,7 +795,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) goto end; - switch (disk->device) { + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, @@ -837,10 +837,8 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virDomainDiskBusTypeToString(disk->bus)); } break; - default: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("disk device type '%s' cannot be hotplugged"), - virDomainDiskDeviceTypeToString(disk->device)); + + case VIR_DOMAIN_DISK_DEVICE_LAST: break; } -- 2.6.2
--- src/qemu/qemu_hotplug.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b456fed..0101063 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -773,13 +773,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; int ret = -1; - const char *driverName = virDomainDiskGetDriver(disk); const char *src = virDomainDiskGetSource(disk); - if (driverName && STRNEQ(driverName, "qemu")) { + if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported driver name '%s' for disk '%s'"), - driverName, src); + virDomainDiskGetDriver(disk), src); goto end; } -- 2.6.2
--- src/qemu/qemu_hotplug.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0101063..43f686c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -818,19 +818,31 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: - if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { + switch ((virDomainDiskBus) disk->bus) { + case VIR_DOMAIN_DISK_BUS_USB: if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk device='lun' is not supported for usb bus")); break; } - ret = qemuDomainAttachUSBMassStorageDevice(conn, driver, vm, - disk); - } else if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + ret = qemuDomainAttachUSBMassStorageDevice(conn, driver, vm, disk); + break; + + case VIR_DOMAIN_DISK_BUS_VIRTIO: ret = qemuDomainAttachVirtioDiskDevice(conn, driver, vm, disk); - } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + break; + + case VIR_DOMAIN_DISK_BUS_SCSI: ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk); - } else { + break; + + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_SD: + case VIR_DOMAIN_DISK_BUS_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk bus '%s' cannot be hotplugged."), virDomainDiskBusTypeToString(disk->bus)); -- 2.6.2
--- src/qemu/qemu_hotplug.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 43f686c..fa83c6e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -779,20 +779,20 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported driver name '%s' for disk '%s'"), virDomainDiskGetDriver(disk), src); - goto end; + goto cleanup; } if (virStorageTranslateDiskSourcePool(conn, disk) < 0) - goto end; + goto cleanup; if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) - goto end; + goto cleanup; if (qemuSetUnprivSGIO(dev) < 0) - goto end; + goto cleanup; if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) - goto end; + goto cleanup; switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: @@ -805,12 +805,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, "by libvirt"), virDomainDiskBusTypeToString(disk->bus), disk->dst); - goto end; + goto cleanup; } if (qemuDomainChangeEjectableMedia(driver, conn, vm, orig_disk, disk->src, false) < 0) - goto end; + goto cleanup; disk->src = NULL; ret = 0; @@ -853,7 +853,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, break; } - end: + cleanup: if (ret != 0) ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); return ret; -- 2.6.2
Target uniqueness check was duplicated in all of the three workers called from it. Extract it to the parent. --- src/qemu/qemu_hotplug.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fa83c6e..2e5cf64 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -315,7 +315,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - size_t i; int ret = -1; const char* type = virDomainDiskBusTypeToString(disk->bus); qemuDomainObjPrivatePtr priv = vm->privateData; @@ -338,14 +337,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto cleanup; } - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - } - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -577,14 +568,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - } - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -688,21 +671,12 @@ qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn, virDomainDiskDefPtr disk) { qemuDomainObjPrivatePtr priv = vm->privateData; - size_t i; int ret = -1; char *drivestr = NULL; char *devstr = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - } - if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -770,6 +744,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { + size_t i; virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; int ret = -1; @@ -818,6 +793,14 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: + for (i = 0; i < vm->def->ndisks; i++) { + if (STREQ(vm->def->disks[i]->dst, disk->dst)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("target %s already exists"), disk->dst); + goto cleanup; + } + } + switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_USB: if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { -- 2.6.2
Put it into a separate function that can be called on two disk def pointers. --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 55e7ed9..2ef6609 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23920,6 +23920,28 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) } +static int +virDomainDiskDefCheckDuplicateInfo(virDomainDiskDefPtr a, + virDomainDiskDefPtr b) +{ + if (a->wwn && b->wwn && STREQ(a->wwn, b->wwn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disks '%s' and '%s' have identical WWN"), + a->dst, b->dst); + return -1; + } + + if (a->serial && b->serial && STREQ(a->serial, b->serial)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disks '%s' and '%s' have identical serial"), + a->dst, b->dst); + return -1; + } + + return 0; +} + + int virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) { @@ -23929,25 +23951,9 @@ virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) for (i = 0; i < def->ndisks; i++) { if (def->disks[i]->wwn || def->disks[i]->serial) { for (j = i + 1; j < def->ndisks; j++) { - if (def->disks[i]->wwn && - STREQ_NULLABLE(def->disks[i]->wwn, - def->disks[j]->wwn)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disks '%s' and '%s' have identical WWN"), - def->disks[i]->dst, - def->disks[j]->dst); + if (virDomainDiskDefCheckDuplicateInfo(def->disks[i], + def->disks[j]) < 0) return -1; - } - - if (def->disks[i]->serial && - STREQ_NULLABLE(def->disks[i]->serial, - def->disks[j]->serial)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disks '%s' and '%s' have identical serial"), - def->disks[i]->dst, - def->disks[j]->dst); - return -1; - } } } } -- 2.6.2
We do the check on VM start, but the user could still hotplug a disk with a conflicting serial or WWN. Reuse the checker function to fix the issue. --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 3 +++ 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ef6609..8a7a989 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23920,7 +23920,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) } -static int +int virDomainDiskDefCheckDuplicateInfo(virDomainDiskDefPtr a, virDomainDiskDefPtr b) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9fdfdf2..cbf01bf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3144,6 +3144,9 @@ virDomainParseMemory(const char *xpath, bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); +int virDomainDiskDefCheckDuplicateInfo(virDomainDiskDefPtr a, + virDomainDiskDefPtr b) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 69be352..bf25473 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -256,6 +256,7 @@ virDomainDiskByName; virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress; +virDomainDiskDefCheckDuplicateInfo; virDomainDiskDefDstDuplicates; virDomainDiskDefForeachPath; virDomainDiskDefFree; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2e5cf64..18a5a12 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -799,6 +799,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, _("target %s already exists"), disk->dst); goto cleanup; } + + if (virDomainDiskDefCheckDuplicateInfo(vm->def->disks[i], disk) < 0) + goto cleanup; } switch ((virDomainDiskBus) disk->bus) { -- 2.6.2
Retrieval of the driver capabilities as well as emulator capabilities does not require the complete qemuProcessStop to be executed on failure. --- src/qemu/qemu_process.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7b09ba7..0f617da 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4431,7 +4431,14 @@ qemuProcessInit(virQEMUDriverPtr driver, } if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto stop; + goto cleanup; + + VIR_DEBUG("Determining emulator version"); + virObjectUnref(priv->qemuCaps); + if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, + vm->def->emulator, + vm->def->os.machine))) + goto cleanup; /* Some things, paths, ... are generated here and we want them to persist. * Fill them in prior to setting the domain def as transient. */ @@ -4461,13 +4468,6 @@ qemuProcessInit(virQEMUDriverPtr driver, VIR_HOOK_SUBOP_BEGIN) < 0) goto stop; - VIR_DEBUG("Determining emulator version"); - virObjectUnref(priv->qemuCaps); - if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine))) - goto stop; - ret = 0; cleanup: -- 2.6.2
When starting a qemu process there are certain checks done to ensure that the configuration makes sense. Extract them into a separate function so that they can be reused in the test code. --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 41 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_process.h | 9 ++++++++- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 51e7125..c13e1b5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3552,7 +3552,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto stopjob; } - if (qemuProcessInit(driver, vm, true) < 0) + if (qemuProcessInit(driver, vm, true, false) < 0) goto stopjob; stopProcess = true; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0f617da..ea1e103 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4401,6 +4401,32 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, /** + * qemuProcessStartValidate: + * @vm: domain object + * @qemuCaps: emulator capabilities + * @migration: restoration of eixting state + * + * This function agregates checks independent from host state done prior to + * start of a VM. + */ +int +qemuProcessStartValidate(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + bool migration, + bool snapshot) +{ + if (qemuValidateCpuCount(def, qemuCaps) < 0) + return -1; + + if (!migration && !snapshot && + virDomainDefCheckDuplicateDiskInfo(def) < 0) + return -1; + + return 0; +} + + +/** * qemuProcessInit: * * Prepares the domain up to the point when priv->qemuCaps is initialized. The @@ -4411,7 +4437,8 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, - bool migration) + bool migration, + bool snap) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virCapsPtr caps = NULL; @@ -4440,6 +4467,9 @@ qemuProcessInit(virQEMUDriverPtr driver, vm->def->os.machine))) goto cleanup; + if (qemuProcessStartValidate(vm->def, priv->qemuCaps, migration, snap) < 0) + goto cleanup; + /* Some things, paths, ... are generated here and we want them to persist. * Fill them in prior to setting the domain def as transient. */ VIR_DEBUG("Generating paths"); @@ -4640,9 +4670,6 @@ qemuProcessLaunch(virConnectPtr conn, } } - if (qemuValidateCpuCount(vm->def, priv->qemuCaps) < 0) - goto cleanup; - if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; @@ -4666,10 +4693,6 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; } - if (!incoming && !snapshot && - virDomainDefCheckDuplicateDiskInfo(vm->def) < 0) - goto cleanup; - /* "volume" type disk's source must be translated before * cgroup and security setting. */ @@ -5112,7 +5135,7 @@ qemuProcessStart(virConnectPtr conn, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY, cleanup); - if (qemuProcessInit(driver, vm, !!migrateFrom) < 0) + if (qemuProcessInit(driver, vm, !!migrateFrom, !!snapshot) < 0) goto cleanup; if (migrateFrom) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index cb5cee1..907a58d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -81,9 +81,16 @@ int qemuProcessStart(virConnectPtr conn, virNetDevVPortProfileOp vmop, unsigned int flags); + +int qemuProcessStartValidate(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + bool migration, + bool snap); + int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, - bool migration); + bool migration, + bool snap); int qemuProcessLaunch(virConnectPtr conn, virQEMUDriverPtr driver, -- 2.6.2
On Thu, Feb 04, 2016 at 03:49:46PM +0100, Peter Krempa wrote:
When starting a qemu process there are certain checks done to ensure that the configuration makes sense. Extract them into a separate function so that they can be reused in the test code. --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 41 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_process.h | 9 ++++++++- 3 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0f617da..ea1e103 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4401,6 +4401,32 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
/** + * qemuProcessStartValidate: + * @vm: domain object + * @qemuCaps: emulator capabilities + * @migration: restoration of eixting state
existing?
+ * + * This function agregates checks independent from host state done prior to
aggregates
+ * start of a VM. + */ +int +qemuProcessStartValidate(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + bool migration, + bool snapshot) +{ + if (qemuValidateCpuCount(def, qemuCaps) < 0) + return -1; +
ACK Jan
Some of the tests that are not a part of qemuBuildCommandLine were not executed in the test suite. We can now reuse qemuProcessStartValidate to integrate these tests. --- tests/qemuxml2argvtest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a5d4722..af6f9a5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -18,6 +18,7 @@ # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" # include "qemu/qemu_migration.h" +# include "qemu/qemu_process.h" # include "datatypes.h" # include "conf/storage_conf.h" # include "cpu/cpu_map.h" @@ -262,6 +263,7 @@ static int testCompareXMLToArgvFiles(const char *xml, virCommandPtr cmd = NULL; size_t i; virBitmapPtr nodeset = NULL; + bool buildFailed = false; if (!(conn = virGetConnect())) goto out; @@ -339,13 +341,20 @@ static int testCompareXMLToArgvFiles(const char *xml, goto out; } - if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, + if (qemuProcessStartValidate(vmdef, extraFlags, !!migrateURI, false) < 0) + buildFailed = true; + + if (!buildFailed && + !(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, (flags & FLAG_JSON), extraFlags, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, &testCallbacks, false, (flags & FLAG_FIPS), - nodeset, NULL, NULL))) { + nodeset, NULL, NULL))) + buildFailed = true; + + if (buildFailed) { if (!virtTestOOMActive() && (flags & FLAG_EXPECT_FAILURE)) { ret = 0; -- 2.6.2
On Thu, Feb 04, 2016 at 03:49:47PM +0100, Peter Krempa wrote:
Some of the tests that are not a part of qemuBuildCommandLine were not executed in the test suite. We can now reuse qemuProcessStartValidate to integrate these tests. --- tests/qemuxml2argvtest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a5d4722..af6f9a5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -18,6 +18,7 @@ # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" # include "qemu/qemu_migration.h" +# include "qemu/qemu_process.h" # include "datatypes.h" # include "conf/storage_conf.h" # include "cpu/cpu_map.h" @@ -262,6 +263,7 @@ static int testCompareXMLToArgvFiles(const char *xml, virCommandPtr cmd = NULL; size_t i; virBitmapPtr nodeset = NULL; + bool buildFailed = false;
if (!(conn = virGetConnect())) goto out; @@ -339,13 +341,20 @@ static int testCompareXMLToArgvFiles(const char *xml, goto out; }
- if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, + if (qemuProcessStartValidate(vmdef, extraFlags, !!migrateURI, false) < 0) + buildFailed = true;
The variable is called 'buildFailed' even though we did not get to the Build part yet. How about testFailed?
+ + if (!buildFailed && + !(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr, (flags & FLAG_JSON), extraFlags, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, &testCallbacks, false, (flags & FLAG_FIPS), - nodeset, NULL, NULL))) { + nodeset, NULL, NULL))) + buildFailed = true; + + if (buildFailed) { if (!virtTestOOMActive() && (flags & FLAG_EXPECT_FAILURE)) { ret = 0;
This message: VIR_TEST_DEBUG("qemuBuildCommandLine should have failed\n"); also needs to be rewritten. ACK with that fixed. Jan
Move the logic from virDomainDiskDefDstDuplicates into virDomainDiskDefCheckDuplicateInfo so that we don't have to loop multiple times through the array of disks. Since the original function was called in qemuBuildDriveDevStr, it was actually called for every single disk which was quite wasteful. Additionally the target uniqueness check needed to be duplicated in the disk hotplug case, since the disk was inserted into the domain definition after the device string was formatted and thus virDomainDiskDefDstDuplicates didn't do anything in that case. --- src/conf/domain_conf.c | 44 +++++++++++++------------------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 3 --- src/qemu/qemu_hotplug.c | 6 ------ 5 files changed, 13 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8a7a989..c1dffc4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12988,31 +12988,6 @@ virDomainDiskControllerMatch(int controller_type, int disk_bus) return false; } -/* Return true if there's a duplicate disk[]->dst name for the same bus */ -bool -virDomainDiskDefDstDuplicates(virDomainDefPtr def) -{ - size_t i, j; - - /* optimization */ - if (def->ndisks <= 1) - return false; - - for (i = 1; i < def->ndisks; i++) { - for (j = 0; j < i; j++) { - if (STREQ(def->disks[i]->dst, def->disks[j]->dst)) { - virReportError(VIR_ERR_XML_ERROR, - _("target '%s' duplicated for disk sources " - "'%s' and '%s'"), - def->disks[i]->dst, - NULLSTR(virDomainDiskGetSource(def->disks[i])), - NULLSTR(virDomainDiskGetSource(def->disks[j]))); - return true; - } - } - } - return false; -} int virDomainDiskIndexByAddress(virDomainDefPtr def, @@ -23924,6 +23899,15 @@ int virDomainDiskDefCheckDuplicateInfo(virDomainDiskDefPtr a, virDomainDiskDefPtr b) { + if (STREQ(a->dst, b->dst)) { + virReportError(VIR_ERR_XML_ERROR, + _("target '%s' duplicated for disk sources '%s' and '%s'"), + a->dst, + NULLSTR(virDomainDiskGetSource(a)), + NULLSTR(virDomainDiskGetSource(b))); + return -1; + } + if (a->wwn && b->wwn && STREQ(a->wwn, b->wwn)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Disks '%s' and '%s' have identical WWN"), @@ -23949,12 +23933,10 @@ virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) size_t j; for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->wwn || def->disks[i]->serial) { - for (j = i + 1; j < def->ndisks; j++) { - if (virDomainDiskDefCheckDuplicateInfo(def->disks[i], - def->disks[j]) < 0) - return -1; - } + for (j = i + 1; j < def->ndisks; j++) { + if (virDomainDiskDefCheckDuplicateInfo(def->disks[i], + def->disks[j]) < 0) + return -1; } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cbf01bf..8a95b20 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2763,7 +2763,6 @@ int virDomainDefCompatibleDevice(virDomainDefPtr def, void virDomainRNGDefFree(virDomainRNGDefPtr def); -bool virDomainDiskDefDstDuplicates(virDomainDefPtr def); int virDomainDiskIndexByAddress(virDomainDefPtr def, virDevicePCIAddressPtr pci_controller, unsigned int bus, unsigned int target, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bf25473..11de1f8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -257,7 +257,6 @@ virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress; virDomainDiskDefCheckDuplicateInfo; -virDomainDiskDefDstDuplicates; virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefNew; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8943270..d7f19f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4228,9 +4228,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def, const char *contAlias; int controllerModel; - if (virDomainDiskDefDstDuplicates(def)) - goto error; - if (qemuCheckDiskConfig(disk) < 0) goto error; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 18a5a12..8771780 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -794,12 +794,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - if (virDomainDiskDefCheckDuplicateInfo(vm->def->disks[i], disk) < 0) goto cleanup; } -- 2.6.2
On Thu, Feb 04, 2016 at 03:49:48PM +0100, Peter Krempa wrote:
Move the logic from virDomainDiskDefDstDuplicates into virDomainDiskDefCheckDuplicateInfo so that we don't have to loop multiple times through the array of disks. Since the original function was called in qemuBuildDriveDevStr, it was actually called for every single disk which was quite wasteful.
Additionally the target uniqueness check needed to be duplicated in the disk hotplug case, since the disk was inserted into the domain definition after the device string was formatted and thus virDomainDiskDefDstDuplicates didn't do anything in that case. --- src/conf/domain_conf.c | 44 +++++++++++++------------------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 3 --- src/qemu/qemu_hotplug.c | 6 ------ 5 files changed, 13 insertions(+), 42 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8a7a989..c1dffc4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8943270..d7f19f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4228,9 +4228,6 @@ qemuBuildDriveDevStr(virDomainDefPtr def, const char *contAlias; int controllerModel;
- if (virDomainDiskDefDstDuplicates(def)) - goto error; -
All the callers of qemuBuildDriveDevStr have this check, except qemuConnectDomainXMLToNative which calls qemuBuildCommandLine without calling qemuProcessStartValidate first. ACK with that fixed. Jan
On Thu, Feb 04, 2016 at 03:49:37PM +0100, Peter Krempa wrote:
Peter Krempa (11): qemu: hotplug: Use typecasted switch qemu: hotplug: Remove unnecessary variable qemu: hotplug: Break up if/else statement into switch qemu: hotplug: Use more common 'cleanup' label in qemuDomainAttachDeviceDiskLive qemu: hotplug: Extract common code to qemuDomainAttachDeviceDiskLive conf: Extract code that checks disk serial/wwn conflict qemu: hotplug: Check duplicate disk serial/wwn on hotplug too qemu: process: Reorder operations on early VM startup qemu: process: Extract pre-start checks into a function tests: Integrate startup checks to qemuxml2argvtest conf: Move and optimize disk target duplicity checking
src/conf/domain_conf.c | 82 +++++++++++++++++++-------------------------- src/conf/domain_conf.h | 4 ++- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_hotplug.c | 85 +++++++++++++++++++++-------------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 57 +++++++++++++++++++++---------- src/qemu/qemu_process.h | 9 ++++- tests/qemuxml2argvtest.c | 13 ++++++-- 9 files changed, 136 insertions(+), 121 deletions(-)
ACK series Jan
On Sat, Feb 06, 2016 at 11:18:36 +0100, Ján Tomko wrote:
On Thu, Feb 04, 2016 at 03:49:37PM +0100, Peter Krempa wrote:
Peter Krempa (11): qemu: hotplug: Use typecasted switch qemu: hotplug: Remove unnecessary variable qemu: hotplug: Break up if/else statement into switch qemu: hotplug: Use more common 'cleanup' label in qemuDomainAttachDeviceDiskLive qemu: hotplug: Extract common code to qemuDomainAttachDeviceDiskLive conf: Extract code that checks disk serial/wwn conflict qemu: hotplug: Check duplicate disk serial/wwn on hotplug too qemu: process: Reorder operations on early VM startup qemu: process: Extract pre-start checks into a function tests: Integrate startup checks to qemuxml2argvtest conf: Move and optimize disk target duplicity checking
ACK series
I've fixed the few issues you've pointed out and pushed this series. Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa