[libvirt] [PATCH 0/4] Test cleanups
Noticed these while I was trying to look for an easy solution to the VIR_TEST_REGENERATE_OUTPUT problem. Ján Tomko (4): drop qemuBuildCommandLineCallbacks tests: clean up includes qemuxml2argvtest: drop FLAG_EXPECT_ERROR qemu: assign addresses before aliases src/qemu/qemu_command.c | 35 +++++++++++------------------------ src/qemu/qemu_command.h | 18 ++---------------- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 29 ++++++++++++++--------------- tests/qemuxml2argvtest.c | 35 ++++++++++------------------------- tests/qemuxml2xmltest.c | 2 +- tests/testutilsqemu.c | 19 ------------------- tests/testutilsqemu.h | 3 +-- 8 files changed, 40 insertions(+), 104 deletions(-) -- 2.7.3
Essentially revert commit 3a6204c which added these to allow the test suite to pass without depending on the host system state. Since commit 4b527c1 we already mock virSCSIDeviceGetSgName, so these callbacks are useless. --- src/qemu/qemu_command.c | 35 +++++++++++------------------------ src/qemu/qemu_command.h | 18 ++---------------- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 3 +-- tests/testutilsqemu.c | 19 ------------------- tests/testutilsqemu.h | 1 - 6 files changed, 15 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd82682..06b96f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4417,20 +4417,16 @@ qemuBuildUSBHostdevUSBDevStr(virDomainHostdevDefPtr dev) } static char * -qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, - qemuBuildCommandLineCallbacksPtr callbacks) +qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev) { virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - char *sg = NULL; - - sg = (callbacks->qemuGetSCSIDeviceSgName)(NULL, - scsihostsrc->adapter, - scsihostsrc->bus, - scsihostsrc->target, - scsihostsrc->unit); - return sg; + + return virSCSIDeviceGetSgName(NULL, + scsihostsrc->adapter, + scsihostsrc->bus, + scsihostsrc->target, + scsihostsrc->unit); } static char * @@ -4475,8 +4471,7 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virConnectPtr conn, char * qemuBuildSCSIHostdevDrvStr(virConnectPtr conn, virDomainHostdevDefPtr dev, - virQEMUCapsPtr qemuCaps, - qemuBuildCommandLineCallbacksPtr callbacks) + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *source = NULL; @@ -4487,8 +4482,7 @@ qemuBuildSCSIHostdevDrvStr(virConnectPtr conn, goto error; virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source); } else { - if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev, qemuCaps, - callbacks))) + if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev))) goto error; virBufferAsprintf(&buf, "file=/dev/%s,if=none", source); } @@ -4785,7 +4779,6 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, virConnectPtr conn, const virDomainDef *def, virQEMUCapsPtr qemuCaps, - qemuBuildCommandLineCallbacksPtr callbacks, unsigned int *bootHostdevNet) { size_t i; @@ -4933,7 +4926,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-drive"); if (!(drvstr = qemuBuildSCSIHostdevDrvStr(conn, hostdev, - qemuCaps, callbacks))) + qemuCaps))) return -1; virCommandAddArg(cmd, drvstr); VIR_FREE(drvstr); @@ -9163,10 +9156,6 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver, } -qemuBuildCommandLineCallbacks buildCommandLineCallbacks = { - .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName, -}; - /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -9185,7 +9174,6 @@ qemuBuildCommandLine(virConnectPtr conn, const char *migrateURI, virDomainSnapshotObjPtr snapshot, virNetDevVPortProfileOp vmop, - qemuBuildCommandLineCallbacksPtr callbacks, bool standalone, bool enableFips, virBitmapPtr nodeset, @@ -9371,8 +9359,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildRedirdevCommandLine(logManager, cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildHostdevCommandLine(cmd, conn, def, qemuCaps, callbacks, - &bootHostdevNet) < 0) + if (qemuBuildHostdevCommandLine(cmd, conn, def, qemuCaps, &bootHostdevNet) < 0) goto error; if (migrateURI) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index a3e6a00..4a8ee4b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -42,18 +42,6 @@ VIR_ENUM_DECL(qemuVideo) -typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks; -typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr; -struct _qemuBuildCommandLineCallbacks { - char *(*qemuGetSCSIDeviceSgName) (const char *sysfs_prefix, - const char *adapter, - unsigned int bus, - unsigned int target, - unsigned long long unit); -}; - -extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks; - char *qemuBuildObjectCommandlineFromJSON(const char *type, const char *alias, virJSONValuePtr props); @@ -68,7 +56,6 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, const char *migrateURI, virDomainSnapshotObjPtr snapshot, virNetDevVPortProfileOp vmop, - qemuBuildCommandLineCallbacksPtr callbacks, bool standalone, bool enableFips, virBitmapPtr nodeset, @@ -182,9 +169,8 @@ char *qemuBuildUSBHostdevDevStr(const virDomainDef *def, char *qemuBuildSCSIHostdevDrvStr(virConnectPtr conn, virDomainHostdevDefPtr dev, - virQEMUCapsPtr qemuCaps, - qemuBuildCommandLineCallbacksPtr callbacks) - ATTRIBUTE_NONNULL(4); + virQEMUCapsPtr qemuCaps); + char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bd1a128..f332b34 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1968,8 +1968,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto cleanup; - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(conn, hostdev, priv->qemuCaps, - &buildCommandLineCallbacks))) + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(conn, hostdev, priv->qemuCaps))) goto cleanup; if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6c870f5..2171665 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5236,7 +5236,7 @@ qemuProcessLaunch(virConnectPtr conn, priv->monJSON, priv->qemuCaps, incoming ? incoming->launchURI : NULL, snapshot, vmop, - &buildCommandLineCallbacks, false, + false, qemuCheckFips(), priv->autoNodeset, &nnicindexes, &nicindexes, @@ -5656,7 +5656,6 @@ qemuProcessCreatePretendCmd(virConnectPtr conn, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, - &buildCommandLineCallbacks, standalone, forceFips ? true : qemuCheckFips(), priv->autoNodeset, diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index eb4c6c8..114cd24 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -448,25 +448,6 @@ virCapsPtr testQemuCapsInit(void) } -static char * -testSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, - const char *adapter ATTRIBUTE_UNUSED, - unsigned int bus ATTRIBUTE_UNUSED, - unsigned int target ATTRIBUTE_UNUSED, - unsigned long long unit ATTRIBUTE_UNUSED) -{ - char *sg = NULL; - - if (VIR_STRDUP(sg, "sg0") < 0) - return NULL; - - return sg; -} - -qemuBuildCommandLineCallbacks testCallbacks = { - .qemuGetSCSIDeviceSgName = testSCSIDeviceGetSgName, -}; - virQEMUCapsPtr qemuTestParseCapabilities(const char *capsFile) { diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 1d5dfc5..196dc23 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -7,7 +7,6 @@ virCapsPtr testQemuCapsInit(void); virDomainXMLOptionPtr testQemuXMLConfInit(void); -extern qemuBuildCommandLineCallbacks testCallbacks; virQEMUCapsPtr qemuTestParseCapabilities(const char *capsFile); -- 2.7.3
On Wed, Apr 13, 2016 at 09:51:38 +0200, Ján Tomko wrote:
Essentially revert commit 3a6204c which added these to allow the test suite to pass without depending on the host system state.
Since commit 4b527c1 we already mock virSCSIDeviceGetSgName, so these callbacks are useless. --- src/qemu/qemu_command.c | 35 +++++++++++------------------------ src/qemu/qemu_command.h | 18 ++---------------- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 3 +-- tests/testutilsqemu.c | 19 ------------------- tests/testutilsqemu.h | 1 - 6 files changed, 15 insertions(+), 64 deletions(-)
ACK I'm glad it's gone
After removing qemuBuildCommandLineCallbacks, testutilsqemu.h does not need to include qemu_command.h. Include just qemu_conf.h here and qemu_domain_address.h in files that need it. --- tests/qemuxml2xmltest.c | 2 +- tests/testutilsqemu.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index dddc775..f4093f1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -13,7 +13,7 @@ #ifdef WITH_QEMU # include "internal.h" -# include "qemu/qemu_conf.h" +# include "qemu/qemu_domain_address.h" # include "qemu/qemu_domain.h" # include "testutilsqemu.h" # include "virstring.h" diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 196dc23..f5a8056 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -2,8 +2,8 @@ # include "capabilities.h" # include "domain_conf.h" -# include "qemu/qemu_command.h" # include "qemu/qemu_capabilities.h" +# include "qemu/qemu_conf.h" virCapsPtr testQemuCapsInit(void); virDomainXMLOptionPtr testQemuXMLConfInit(void); -- 2.7.3
On Wed, Apr 13, 2016 at 09:51:39 +0200, Ján Tomko wrote:
After removing qemuBuildCommandLineCallbacks, testutilsqemu.h does not need to include qemu_command.h.
Include just qemu_conf.h here and qemu_domain_address.h in files that need it. --- tests/qemuxml2xmltest.c | 2 +- tests/testutilsqemu.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
ACK
It is only used for failed address allocation Since we already have FLAG_EXPECT_FAILURE, use that instead. --- tests/qemuxml2argvtest.c | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 975e358..2a57176 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -242,11 +242,10 @@ static virStorageDriver fakeStorageDriver = { }; typedef enum { - FLAG_EXPECT_ERROR = 1 << 0, - FLAG_EXPECT_FAILURE = 1 << 1, - FLAG_EXPECT_PARSE_ERROR = 1 << 2, - FLAG_JSON = 1 << 3, - FLAG_FIPS = 1 << 4, + FLAG_EXPECT_FAILURE = 1 << 0, + FLAG_EXPECT_PARSE_ERROR = 1 << 1, + FLAG_JSON = 1 << 2, + FLAG_FIPS = 1 << 3, } virQemuXML2ArgvTestFlags; static int testCompareXMLToArgvFiles(const char *xml, @@ -313,7 +312,7 @@ static int testCompareXMLToArgvFiles(const char *xml, virQEMUCapsFilterByMachineType(extraFlags, vm->def->os.machine); if (qemuDomainAssignAddresses(vm->def, extraFlags, NULL)) { - if (flags & FLAG_EXPECT_ERROR) + if (flags & FLAG_EXPECT_FAILURE) goto ok; goto out; } @@ -354,20 +353,15 @@ static int testCompareXMLToArgvFiles(const char *xml, ret = 0; ok: - if (ret == 0 && - ((flags & FLAG_EXPECT_ERROR) || - (flags & FLAG_EXPECT_FAILURE))) { + if (ret == 0 && flags & FLAG_EXPECT_FAILURE) { ret = -1; VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); goto out; } if (!virtTestOOMActive()) { - if (flags & FLAG_EXPECT_ERROR) { + if (flags & FLAG_EXPECT_FAILURE) { if ((log = virtTestLogContentAndReset())) VIR_TEST_DEBUG("Got expected error: \n%s", log); - } else if (flags & FLAG_EXPECT_FAILURE) { - VIR_TEST_DEBUG("Got expected failure: %s\n", - virGetLastErrorMessage()); } virResetLastError(); ret = 0; @@ -533,20 +527,17 @@ mymain(void) # define DO_TEST(name, ...) \ DO_TEST_FULL(name, NULL, -1, 0, 0, __VA_ARGS__) -# define DO_TEST_ERROR(name, ...) \ - DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_ERROR, 0, __VA_ARGS__) - # define DO_TEST_FAILURE(name, ...) \ DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, 0, __VA_ARGS__) # define DO_TEST_PARSE_ERROR(name, ...) \ DO_TEST_FULL(name, NULL, -1, \ - FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ + FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ 0, __VA_ARGS__) # define DO_TEST_PARSE_FLAGS_ERROR(name, parseFlags, ...) \ DO_TEST_FULL(name, NULL, -1, \ - FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_ERROR, \ + FLAG_EXPECT_PARSE_ERROR | FLAG_EXPECT_FAILURE, \ parseFlags, __VA_ARGS__) # define DO_TEST_LINUX(name, ...) \ @@ -1389,7 +1380,7 @@ mymain(void) QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION); DO_TEST("pseries-vio-user-assigned", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); - DO_TEST_ERROR("pseries-vio-address-clash", + DO_TEST_FAILURE("pseries-vio-address-clash", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM); DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI, @@ -1552,7 +1543,7 @@ mymain(void) QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); - DO_TEST_ERROR("pcie-root-port-too-many", + DO_TEST_FAILURE("pcie-root-port-too-many", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, -- 2.7.3
On Wed, Apr 13, 2016 at 09:51:40 +0200, Ján Tomko wrote:
It is only used for failed address allocation Since we already have FLAG_EXPECT_FAILURE, use that instead. --- tests/qemuxml2argvtest.c | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 975e358..2a57176 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c
@@ -354,20 +353,15 @@ static int testCompareXMLToArgvFiles(const char *xml, ret = 0;
ok: - if (ret == 0 && - ((flags & FLAG_EXPECT_ERROR) || - (flags & FLAG_EXPECT_FAILURE))) { + if (ret == 0 && flags & FLAG_EXPECT_FAILURE) { ret = -1; VIR_TEST_DEBUG("Error expected but there wasn't any.\n"); goto out; } if (!virtTestOOMActive()) { - if (flags & FLAG_EXPECT_ERROR) { + if (flags & FLAG_EXPECT_FAILURE) { if ((log = virtTestLogContentAndReset())) VIR_TEST_DEBUG("Got expected error: \n%s", log); - } else if (flags & FLAG_EXPECT_FAILURE) { - VIR_TEST_DEBUG("Got expected failure: %s\n", - virGetLastErrorMessage());
This is a semantic change in the format of error messages reported by the test: Current message: 541) QEMU XML-2-ARGV machine-aeskeywrap-off-cap ... Got expected failure: unsupported configuration: key wrap support is not available with this QEMU binary OK New format: 541) QEMU XML-2-ARGV machine-aeskeywrap-off-cap ... Got expected error: 2016-04-13 10:35:46.726+0000: 263072: error : qemuBuildMachineCommandLine:6739 : unsupported configuration: key wrap support is not available with this QEMU binary OK I think I prefer the new one since it also carries the function name. You need to mention that change in the commit message though. ACK with ^^ Peter
The address assigning code might add new pci bridges. We need them to have an alias when building the command line. In real word usage, this is not a problem because all the code paths already call qemuDomainAssignAddresses. However moving this call lets us remove one extra call from qemuxml2argvtest. --- src/qemu/qemu_process.c | 26 +++++++++++++------------- tests/qemuxml2argvtest.c | 6 ------ 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2171665..986f406 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4999,6 +4999,19 @@ qemuProcessPrepareDomain(virConnectPtr conn, } } + /* + * Normally PCI addresses are assigned in the virDomainCreate + * or virDomainDefine methods. We might still need to assign + * some here to cope with the question of upgrades. Regardless + * we also need to populate the PCI address set cache for later + * use in hotplug + */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + VIR_DEBUG("Assigning domain PCI addresses"); + if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) + goto cleanup; + } + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; @@ -5022,19 +5035,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, priv->monStart = 0; priv->gotShutdown = false; - /* - * Normally PCI addresses are assigned in the virDomainCreate - * or virDomainDefine methods. We might still need to assign - * some here to cope with the question of upgrades. Regardless - * we also need to populate the PCI address set cache for later - * use in hotplug - */ - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - VIR_DEBUG("Assigning domain PCI addresses"); - if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) - goto cleanup; - } - ret = 0; cleanup: VIR_FREE(nodeset); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2a57176..be74178 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -311,12 +311,6 @@ static int testCompareXMLToArgvFiles(const char *xml, virQEMUCapsFilterByMachineType(extraFlags, vm->def->os.machine); - if (qemuDomainAssignAddresses(vm->def, extraFlags, NULL)) { - if (flags & FLAG_EXPECT_FAILURE) - goto ok; - goto out; - } - log = virtTestLogContentAndReset(); VIR_FREE(log); virResetLastError(); -- 2.7.3
On Wed, Apr 13, 2016 at 09:51:41 +0200, Ján Tomko wrote:
The address assigning code might add new pci bridges. We need them to have an alias when building the command line.
In real word usage, this is not a problem because all the code paths already call qemuDomainAssignAddresses. However moving this call lets us remove one extra call from qemuxml2argvtest. --- src/qemu/qemu_process.c | 26 +++++++++++++------------- tests/qemuxml2argvtest.c | 6 ------ 2 files changed, 13 insertions(+), 19 deletions(-)
ACK
participants (2)
-
Ján Tomko -
Peter Krempa