[libvirt] [PATCH v2 00/10] Test persistent device attachment
Changes in v2: * rebased This patch requires another one, otherwise a small conflict appears: https://www.redhat.com/archives/libvir-list/2016-July/msg00402.html In qemu_driver.c, two functions were split to have less responsibility. They were also modified so that they could be used in qemuhotplugtest. Now instead of running different functions to attach different device types, a generic function is run. Hopefully this will result in more device types tested in the future. Suggestions on how to name these two new functions are very welcome. In the future, if one uses the following command: virsh attach-device --live --config the new device will have the same address assigned in both live and config domain. To make sure that everything will work properly after making these changes, I reworked qemuhotplug.c to work with attaching devices to persistent domains. So far, there is only one test for persistent attachment. I will add more testcases as soon as I hear some feedback on these changes. qemuhotplugtest.c should first be modified anyway, so that the three xmls's filenames (basis domain xml, device xml, expected xml) are stated explicitly instead of being calculated. This will allow for more flexibility in testing and less xml files duplicates. Tomasz Tomasz Flendrich (10): Change parameters to qemuDomainAttachDeviceLive Remove an unnecessary variable Split qemuDomainAttachDeviceFlags in two Narrow down a parameter Split qemuDomainDetachDeviceFlags in two Make qemu attach/detach functions public Narrow down a parameter to function Use more generic functions in qemuhotplugtest Make qemuhotplugtest work with persistent domains Add a persistent attachment testcase src/qemu/qemu_driver.c | 221 +++++++++++---------- src/qemu/qemu_driver.h | 14 ++ tests/qemuhotplugtest.c | 213 ++++++++++++-------- .../qemuhotplug-base-config+qemu-agent+config.xml | 45 +++++ .../qemuhotplug-base-config.xml | 40 ++++ 5 files changed, 349 insertions(+), 184 deletions(-) create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml -- 2.7.4
We want to be able to pass a NULL instead of the connection and use this function in tests. To achieve this, the virConnectPtr is passed instead of virDomainPtr, and the driver is a new separate parameter. --- src/qemu/qemu_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cda85f6..9d1f256 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7327,16 +7327,16 @@ qemuDomainUndefine(virDomainPtr dom) static int qemuDomainAttachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, - virDomainPtr dom) + virConnectPtr conn, + virQEMUDriverPtr driver) { - virQEMUDriverPtr driver = dom->conn->privateData; int ret = -1; const char *alias = NULL; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, NULL); - ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev); + ret = qemuDomainAttachDeviceDiskLive(conn, driver, vm, dev); if (!ret) { alias = dev->data.disk->info.alias; dev->data.disk = NULL; @@ -7369,7 +7369,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: qemuDomainObjCheckHostdevTaint(driver, vm, dev->data.hostdev, NULL); - ret = qemuDomainAttachHostDevice(dom->conn, driver, vm, + ret = qemuDomainAttachHostDevice(conn, driver, vm, dev->data.hostdev); if (!ret) { alias = dev->data.hostdev->info->alias; @@ -8151,7 +8151,8 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob; - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom->conn, + dom->conn->privateData)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:46AM +0200, Tomasz Flendrich wrote:
We want to be able to pass a NULL instead of the connection and use this function in tests. To achieve this, the virConnectPtr is passed instead of virDomainPtr, and the driver is a new separate parameter.
--- src/qemu/qemu_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cda85f6..9d1f256 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8151,7 +8151,8 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob;
- if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom->conn, + dom->conn->privateData)) < 0)
You can pass 'driver' here, it exists here in this function. Not only it makes the line shorter, it will not bite us if we change the type of the function parameter.
qemuCaps is no longer used anywhere in these functions, so it can be deleted. --- src/qemu/qemu_driver.c | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9d1f256..0362f0e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8079,8 +8079,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, int ret = -1; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; - virQEMUCapsPtr qemuCaps = NULL; - qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; @@ -8097,8 +8095,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -8125,11 +8121,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, goto endjob; } - if (priv->qemuCaps) - qemuCaps = virObjectRef(priv->qemuCaps); - else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) - goto endjob; - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); @@ -8178,7 +8169,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - virObjectUnref(qemuCaps); virDomainDefFree(vmdef); if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); @@ -8331,8 +8321,6 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; - virQEMUCapsPtr qemuCaps = NULL; - qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; @@ -8347,8 +8335,6 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -8379,11 +8365,6 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, goto endjob; } - if (priv->qemuCaps) - qemuCaps = virObjectRef(priv->qemuCaps); - else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) - goto endjob; - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); @@ -8431,7 +8412,6 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - virObjectUnref(qemuCaps); virDomainDefFree(vmdef); if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); -- 2.7.4
Previously, qemuDomainAttachDeviceFlags was doing two things: handling the job and attaching devices. Now the second part is in a new function. This change is required to make it possible to test more complex device attachment situations, like attaching a device to both config and live at once. --- src/qemu/qemu_driver.c | 95 +++++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0362f0e..5dde81f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8066,49 +8066,34 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, return 0; } - static int -qemuDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) +qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, + virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; + virQEMUDriverConfigPtr cfg = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; + virCapsPtr caps = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; - virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - virNWFilterReadLockFilterUpdates(); - cfg = virQEMUDriverGetConfig(driver); if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; - if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; - - if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - - if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) - goto endjob; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, parse_flags); if (dev == NULL) - goto endjob; + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -8118,33 +8103,32 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); if (!dev_copy) - goto endjob; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) - goto endjob; + goto cleanup; if (virDomainDefCompatibleDevice(vmdef, dev, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) - goto endjob; - - if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, dom->conn, caps, + goto cleanup; + if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, conn, caps, parse_flags, driver->xmlopt)) < 0) - goto endjob; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (virDomainDefCompatibleDevice(vm->def, dev_copy, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) - goto endjob; + goto cleanup; - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom->conn, - dom->conn->privateData)) < 0) - goto endjob; + if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, conn, + driver)) < 0) + goto cleanup; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8152,7 +8136,7 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) { ret = -1; - goto endjob; + goto cleanup; } } @@ -8165,17 +8149,50 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, } } - endjob: - qemuDomainObjEndJob(driver, vm); - cleanup: virDomainDefFree(vmdef); if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); virDomainDeviceDefFree(dev); - virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); + virObjectUnref(caps); + + return ret; +} + +static int +qemuDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virNWFilterReadLockFilterUpdates(); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (qemuDomainAttachDeviceLiveAndConfig(dom->conn, vm, driver, xml, flags) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); virNWFilterUnlockFilterUpdates(); return ret; } -- 2.7.4
This will make splitting up qemuDomainDetachDeviceFlags into two functions easier. --- src/qemu/qemu_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5dde81f..ce22eff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7472,9 +7472,8 @@ qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, static int qemuDomainDetachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, - virDomainPtr dom) + virQEMUDriverPtr driver) { - virQEMUDriverPtr driver = dom->conn->privateData; int ret = -1; switch ((virDomainDeviceType) dev->type) { @@ -8403,7 +8402,7 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob; - if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom->conn->privateData)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:49AM +0200, Tomasz Flendrich wrote:
This will make splitting up qemuDomainDetachDeviceFlags into two functions easier.
--- src/qemu/qemu_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5dde81f..ce22eff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7472,9 +7472,8 @@ qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, static int qemuDomainDetachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, - virDomainPtr dom) + virQEMUDriverPtr driver) { - virQEMUDriverPtr driver = dom->conn->privateData; int ret = -1;
switch ((virDomainDeviceType) dev->type) { @@ -8403,7 +8402,7 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob;
- if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom->conn->privateData)) < 0)
As mentioned in patch #1, I'd rather see non-void pointer passed here.
goto endjob; /* * update domain status forcibly because the domain status may be -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Previously, qemuDomainDetachDeviceFlags was doing two things: handling the job and detaching devices. Now the second part is in a new function. --- src/qemu/qemu_driver.c | 92 +++++++++++++++++++++++++++++--------------------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce22eff..4a62e18 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8325,40 +8325,26 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, return ret; } - static int -qemuDomainDetachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) +qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; - virDomainDefPtr vmdef = NULL; + virCapsPtr caps = NULL; + virQEMUDriverConfigPtr cfg = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; - int ret = -1; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; - virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; + virDomainDefPtr vmdef = NULL; + int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - cfg = virQEMUDriverGetConfig(driver); - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; - if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; - - if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - - if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) - goto endjob; + cfg = virQEMUDriverGetConfig(driver); if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !(flags & VIR_DOMAIN_AFFECT_LIVE)) @@ -8368,7 +8354,7 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, caps, driver->xmlopt, parse_flags); if (dev == NULL) - goto endjob; + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -8378,32 +8364,32 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); if (!dev_copy) - goto endjob; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) - goto endjob; + goto cleanup; if (virDomainDefCompatibleDevice(vmdef, dev, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) - goto endjob; + goto cleanup; if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev, caps, parse_flags, driver->xmlopt)) < 0) - goto endjob; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (virDomainDefCompatibleDevice(vm->def, dev_copy, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) - goto endjob; + goto cleanup; - if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom->conn->privateData)) < 0) - goto endjob; + if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, driver)) < 0) + goto cleanup; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8411,7 +8397,7 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) { ret = -1; - goto endjob; + goto cleanup; } } @@ -8424,17 +8410,47 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, } } - endjob: - qemuDomainObjEndJob(driver, vm); - cleanup: - virDomainDefFree(vmdef); + virObjectUnref(caps); + virObjectUnref(cfg); if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); virDomainDeviceDefFree(dev); + virDomainDefFree(vmdef); + return ret; +} + +static int +qemuDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (qemuDomainDetachDeviceLiveAndConfig(driver, vm, xml, flags) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); - virObjectUnref(cfg); return ret; } -- 2.7.4
They will be used to test device attachment and detachment, so they have to be visible to the outside. --- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_driver.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4a62e18..0671d24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8065,7 +8065,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, return 0; } -static int +int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, virDomainObjPtr vm, virQEMUDriverPtr driver, @@ -8325,7 +8325,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, return ret; } -static int +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *xml, diff --git a/src/qemu/qemu_driver.h b/src/qemu/qemu_driver.h index df7533a..3284933 100644 --- a/src/qemu/qemu_driver.h +++ b/src/qemu/qemu_driver.h @@ -24,6 +24,20 @@ #ifndef __QEMU_DRIVER_H__ # define __QEMU_DRIVER_H__ +# include "domain_conf.h" +# include "qemu_conf.h" + int qemuRegister(void); +int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, + virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags); + +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags); + #endif /* __QEMU_DRIVER_H__ */ -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:51AM +0200, Tomasz Flendrich wrote:
They will be used to test device attachment and detachment, so they have to be visible to the outside.
--- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_driver.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4a62e18..0671d24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8065,7 +8065,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, return 0; }
-static int +int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, virDomainObjPtr vm, virQEMUDriverPtr driver, @@ -8325,7 +8325,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, return ret; }
-static int +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *xml, diff --git a/src/qemu/qemu_driver.h b/src/qemu/qemu_driver.h index df7533a..3284933 100644 --- a/src/qemu/qemu_driver.h +++ b/src/qemu/qemu_driver.h @@ -24,6 +24,20 @@ #ifndef __QEMU_DRIVER_H__ # define __QEMU_DRIVER_H__
+# include "domain_conf.h" +# include "qemu_conf.h" + int qemuRegister(void);
+int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, + virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags); + +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags); + #endif /* __QEMU_DRIVER_H__ */
This should not be exposed in qemu_driver.h, instead the functions should be moved to qemu_domain.h if possible, but they now fit there. If that's not possible, we need to do similar thing as we now have with qemu_processpriv.h, so this could be called qemu_domainpriv.h I guess.
+int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn,
+ virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags); + +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags); + #endif /* __QEMU_DRIVER_H__ */
This should not be exposed in qemu_driver.h, instead the functions should be moved to qemu_domain.h if possible, but they now fit there.
If that's not possible, we need to do similar thing as we now have with qemu_processpriv.h, so this could be called qemu_domainpriv.h I guess
Did you mean qemu_driverpriv.h? As you predicted, it's not easily done. qemuDomainAttachDeviceLiveAndConfig() indirectly uses a few functions from qemu_hotplug. It would be pointless to move them all to qemu_domain.h, because then we would have to drag the whole qemu_hotplug.[hc] along. What can be done is moving these to qemu_hotplug.h: - qemuDomainAttachDeviceLive() - qemuDomainDetachDeviceLive(), and these to qemu_domain.h: - qemuDomainAttachDeviceConfig() - qemuDomainDetachDeviceConfig() - qemuDomainChrPreInsert() - qemuDomainChrInsertPreAlloced() - qemuDomainChrInsertPreAllocCleanup() - qemuDomainChrInsert() - qemuDomainChrRemove(). But then where should qemuDomainAttachDeviceLiveAndConfig() and qemuDomainDetachDeviceLiveAndConfig() be? In that qemu_domainpriv/qemu_driverpriv.h? And should their definitions still be in qemu_driver.c, or in a brand new .c file? Btw qemuDomainUpdateDeviceFlags() is still unsplit and it deserves the same treatment as the other two *DeviceFlags functions. It will be done in v3. Thank you again for such thorough reviews! I really appreciate it. Tomasz
On Wed, Jul 27, 2016 at 01:08:17AM +0200, Tomasz Flendrich wrote:
+int qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn,
+ virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *xml, + unsigned int flags); + +int qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *xml, + unsigned int flags); + #endif /* __QEMU_DRIVER_H__ */
This should not be exposed in qemu_driver.h, instead the functions should be moved to qemu_domain.h if possible, but they now fit there.
If that's not possible, we need to do similar thing as we now have with qemu_processpriv.h, so this could be called qemu_domainpriv.h I guess
Did you mean qemu_driverpriv.h?
I really meant 'domain' as I was thinking these are qemu_domain related, but since they live in qemu_driver.c let's do qemu_driverpriv.h in order not to complicate things.
As you predicted, it's not easily done.
qemuDomainAttachDeviceLiveAndConfig() indirectly uses a few functions from qemu_hotplug. It would be pointless to move them all to qemu_domain.h, because then we would have to drag the whole qemu_hotplug.[hc] along.
What can be done is moving these to qemu_hotplug.h: - qemuDomainAttachDeviceLive() - qemuDomainDetachDeviceLive(),
and these to qemu_domain.h: - qemuDomainAttachDeviceConfig() - qemuDomainDetachDeviceConfig() - qemuDomainChrPreInsert() - qemuDomainChrInsertPreAlloced() - qemuDomainChrInsertPreAllocCleanup() - qemuDomainChrInsert() - qemuDomainChrRemove().
But then where should qemuDomainAttachDeviceLiveAndConfig() and qemuDomainDetachDeviceLiveAndConfig() be? In that qemu_domainpriv/qemu_driverpriv.h? And should their definitions still be in qemu_driver.c, or in a brand new .c file?
Hoinestly? I think the code should be split in all the drivers a bit differently. There should be xxx_driver.h as is, then xxx_driver.c with minimum functions (ideally just the xxxRegister one) and then all static methods from the xxx_driver.c should be moved to a different file, e.g. xxx_apis.c and those that are needed in the driver or tests would be made non-static without problems as it as its own header file. But that's not what was done or will happen, so let's not complicate it and leave it as is with just the private header for those functions needed in tests.
Btw qemuDomainUpdateDeviceFlags() is still unsplit and it deserves the same treatment as the other two *DeviceFlags functions. It will be done in v3.
Cool, I can't wait!
Thank you again for such thorough reviews! I really appreciate it. Tomasz
Attaching to both live and config will soon be tested, so testQemuHotplugCheckResult will be used on both def and newDef. --- tests/qemuhotplugtest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 0a5f068..d862a0e 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -175,7 +175,7 @@ testQemuHotplugUpdate(virDomainObjPtr vm, } static int -testQemuHotplugCheckResult(virDomainObjPtr vm, +testQemuHotplugCheckResult(virDomainDefPtr def, const char *expected, const char *expectedFile, bool fail) @@ -183,11 +183,11 @@ testQemuHotplugCheckResult(virDomainObjPtr vm, char *actual; int ret; - actual = virDomainDefFormat(vm->def, driver.caps, + actual = virDomainDefFormat(def, driver.caps, VIR_DOMAIN_DEF_FORMAT_SECURE); if (!actual) return -1; - vm->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID; + def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID; if (STREQ(expected, actual)) { if (fail) @@ -299,14 +299,14 @@ testQemuHotplug(const void *data) VIR_FREE(dev); } if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm, result_xml, + ret = testQemuHotplugCheckResult(vm->def, result_xml, result_filename, fail); break; case DETACH: ret = testQemuHotplugDetach(vm, dev); if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm, domain_xml, + ret = testQemuHotplugCheckResult(vm->def, domain_xml, domain_filename, fail); break; -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:52AM +0200, Tomasz Flendrich wrote:
Attaching to both live and config will soon be tested, so testQemuHotplugCheckResult will be used on both def and newDef.
Or you could just pass the modification impact flag in as well and select ->def or ->newDef based on that.
--- tests/qemuhotplugtest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 0a5f068..d862a0e 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -175,7 +175,7 @@ testQemuHotplugUpdate(virDomainObjPtr vm, }
static int -testQemuHotplugCheckResult(virDomainObjPtr vm, +testQemuHotplugCheckResult(virDomainDefPtr def, const char *expected, const char *expectedFile, bool fail) @@ -183,11 +183,11 @@ testQemuHotplugCheckResult(virDomainObjPtr vm, char *actual; int ret;
- actual = virDomainDefFormat(vm->def, driver.caps, + actual = virDomainDefFormat(def, driver.caps, VIR_DOMAIN_DEF_FORMAT_SECURE); if (!actual) return -1; - vm->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID; + def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
This should not be set for newDef, it needs to be kept at '-1' for config updates, because 'id != -1' means the domain is running.
More generic functions, qemuDomainAttachDeviceLiveAndConfig and qemuDomainDetachDeviceLiveAndConfig, are now used instead of other functions in qemuhotplugtest to attach and detach devices. --- tests/qemuhotplugtest.c | 157 +++++++++++++++++++++++++++--------------------- 1 file changed, 88 insertions(+), 69 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index d862a0e..da361a2 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -23,6 +23,7 @@ #include "qemu/qemu_conf.h" #include "qemu/qemu_hotplug.h" #include "qemu/qemu_hotplugpriv.h" +#include "qemu/qemu_driver.h" #include "qemumonitortestutils.h" #include "testutils.h" #include "testutilsqemu.h" @@ -52,6 +53,7 @@ struct qemuHotplugTestData { bool keep; virDomainObjPtr vm; bool deviceDeletedEvent; + unsigned int target; }; static int @@ -105,19 +107,20 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, static int testQemuHotplugAttach(virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + const char *device_xml, + unsigned int target) { int ret = -1; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_CHR: /* conn in only used for storage pool and secrets lookup so as long * as we don't use any of them, passing NULL should be safe */ - ret = qemuDomainAttachDeviceDiskLive(NULL, &driver, vm, dev); - break; - case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + ret = qemuDomainAttachDeviceLiveAndConfig(NULL, vm, &driver, + device_xml, target); break; default: VIR_TEST_VERBOSE("device type '%s' cannot be attached\n", @@ -130,16 +133,17 @@ testQemuHotplugAttach(virDomainObjPtr vm, static int testQemuHotplugDetach(virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + const char *device_xml, + unsigned int target) { int ret = -1; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(&driver, vm, dev); - break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr); + ret = qemuDomainDetachDeviceLiveAndConfig(&driver, vm, + device_xml, target); break; default: VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", @@ -225,6 +229,7 @@ testQemuHotplug(const void *data) virCapsPtr caps = NULL; qemuMonitorTestPtr test_mon = NULL; qemuDomainObjPrivatePtr priv = NULL; + unsigned int target = test->target; if (virAsprintf(&domain_filename, "%s/qemuhotplugtestdomains/qemuhotplug-%s.xml", abs_srcdir, test->domain_filename) < 0 || @@ -292,7 +297,7 @@ testQemuHotplug(const void *data) switch (test->action) { case ATTACH: - ret = testQemuHotplugAttach(vm, dev); + ret = testQemuHotplugAttach(vm, dev, device_xml, target); if (ret == 0) { /* vm->def stolen dev->data.* so we just need to free the dev * envelope */ @@ -304,7 +309,7 @@ testQemuHotplug(const void *data) break; case DETACH: - ret = testQemuHotplugDetach(vm, dev); + ret = testQemuHotplugDetach(vm, dev, device_xml, target); if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm->def, domain_xml, domain_filename, fail); @@ -371,7 +376,7 @@ mymain(void) /* wait only 100ms for DEVICE_DELETED event */ qemuDomainRemoveDeviceWaitTime = 100; -#define DO_TEST(file, ACTION, dev, event, fial, kep, ...) \ +#define DO_TEST(file, ACTION, dev, event, fial, kep, targt, ...) \ do { \ const char *my_mon[] = { __VA_ARGS__, NULL}; \ const char *name = file " " #ACTION " " dev; \ @@ -382,28 +387,30 @@ mymain(void) data.mon = my_mon; \ data.keep = kep; \ data.deviceDeletedEvent = event; \ + data.target = targt; \ if (virTestRun(name, testQemuHotplug, &data) < 0) \ ret = -1; \ } while (0) -#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, false, fial, kep, __VA_ARGS__) +#define DO_TEST_ATTACH(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, ATTACH, dev, false, fial, kep, targt, __VA_ARGS__) -#define DO_TEST_DETACH(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, false, fial, kep, __VA_ARGS__) +#define DO_TEST_DETACH(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, DETACH, dev, false, fial, kep, targt, __VA_ARGS__) -#define DO_TEST_ATTACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, true, fial, kep, __VA_ARGS__) +#define DO_TEST_ATTACH_EVENT(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, ATTACH, dev, true, fial, kep, targt, __VA_ARGS__) -#define DO_TEST_DETACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, true, fial, kep, __VA_ARGS__) +#define DO_TEST_DETACH_EVENT(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, DETACH, dev, true, fial, kep, targt, __VA_ARGS__) -#define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ - DO_TEST(file, UPDATE, dev, false, fial, kep, __VA_ARGS__) +#define DO_TEST_UPDATE(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, UPDATE, dev, false, fial, kep, targt, __VA_ARGS__) #define QMP_OK "{\"return\": {}}" #define HMP(msg) "{\"return\": \"" msg "\"}" +#define QOM_OK "{ \"return\": []}" #define QMP_DEVICE_DELETED(dev) \ "{" \ @@ -418,77 +425,86 @@ mymain(void) " }" \ "}\r\n" - DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, NULL); - DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, + DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, VIR_DOMAIN_AFFECT_LIVE, NULL); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, VIR_DOMAIN_AFFECT_LIVE, "set_password", QMP_OK, "expire_password", QMP_OK); - DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-password", false, false, + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-password", false, false, VIR_DOMAIN_AFFECT_LIVE, "set_password", QMP_OK, "expire_password", QMP_OK); - DO_TEST_UPDATE("graphics-spice", "graphics-spice-listen", true, false, NULL); - DO_TEST_UPDATE("graphics-spice-listen-network", "graphics-spice-listen-network-password", false, false, + DO_TEST_UPDATE("graphics-spice", "graphics-spice-listen", true, false, VIR_DOMAIN_AFFECT_LIVE, NULL); + DO_TEST_UPDATE("graphics-spice-listen-network", "graphics-spice-listen-network-password", false, false, VIR_DOMAIN_AFFECT_LIVE, "set_password", QMP_OK, "expire_password", QMP_OK); /* Strange huh? Currently, only graphics can be updated :-P */ - DO_TEST_UPDATE("disk-cdrom", "disk-cdrom-nochange", true, false, NULL); + DO_TEST_UPDATE("disk-cdrom", "disk-cdrom-nochange", true, false, VIR_DOMAIN_AFFECT_LIVE, NULL); - DO_TEST_ATTACH("console-compat-2-live", "console-virtio", false, true, + DO_TEST_ATTACH("console-compat-2-live", "console-virtio", false, true, VIR_DOMAIN_AFFECT_LIVE, "chardev-add", "{\"return\": {\"pty\": \"/dev/pts/26\"}}", "device_add", QMP_OK); - DO_TEST_DETACH("console-compat-2-live", "console-virtio", false, false, + DO_TEST_DETACH("console-compat-2-live", "console-virtio", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "chardev-remove", QMP_OK); - DO_TEST_ATTACH("base-live", "disk-virtio", false, true, + DO_TEST_ATTACH("base-live", "disk-virtio", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-virtio", false, false, + DO_TEST_DETACH("base-live", "disk-virtio", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH_EVENT("base-live", "disk-virtio", false, true, + DO_TEST_ATTACH_EVENT("base-live", "disk-virtio", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), - "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-virtio", true, true, + "device_add", QMP_OK, + "qom-list", QOM_OK); + DO_TEST_DETACH("base-live", "disk-virtio", true, true, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, + "qom-list", QOM_OK, "human-monitor-command", HMP("")); - DO_TEST_DETACH("base-live", "disk-virtio", false, false, + DO_TEST_DETACH("base-live", "disk-virtio", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_DEVICE_DELETED("virtio-disk4") QMP_OK, - "human-monitor-command", HMP("")); + "human-monitor-command", HMP(""), + "qom-list", QOM_OK); - DO_TEST_ATTACH("base-live", "disk-usb", false, true, + DO_TEST_ATTACH("base-live", "disk-usb", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-usb", false, false, + DO_TEST_DETACH("base-live", "disk-usb", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH_EVENT("base-live", "disk-usb", false, true, + DO_TEST_ATTACH_EVENT("base-live", "disk-usb", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), - "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-usb", true, true, + "device_add", QMP_OK, + "qom-list", QOM_OK); + DO_TEST_DETACH("base-live", "disk-usb", true, true, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, + "qom-list", QOM_OK, "human-monitor-command", HMP("")); - DO_TEST_DETACH("base-live", "disk-usb", false, false, + DO_TEST_DETACH("base-live", "disk-usb", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_DEVICE_DELETED("usb-disk16") QMP_OK, - "human-monitor-command", HMP("")); + "human-monitor-command", HMP(""), + "qom-list", QOM_OK); - DO_TEST_ATTACH("base-live", "disk-scsi", false, true, + DO_TEST_ATTACH("base-live", "disk-scsi", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-scsi", false, false, + DO_TEST_DETACH("base-live", "disk-scsi", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH_EVENT("base-live", "disk-scsi", false, true, + DO_TEST_ATTACH_EVENT("base-live", "disk-scsi", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), - "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "disk-scsi", true, true, + "device_add", QMP_OK, + "qom-list", QOM_OK); + DO_TEST_DETACH("base-live", "disk-scsi", true, true, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, + "qom-list", QOM_OK, "human-monitor-command", HMP("")); - DO_TEST_DETACH("base-live", "disk-scsi", false, false, + DO_TEST_DETACH("base-live", "disk-scsi", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK, - "human-monitor-command", HMP("")); + "human-monitor-command", HMP(""), + "qom-list", QOM_OK); - DO_TEST_ATTACH("base-without-scsi-controller-live", "disk-scsi-2", false, true, + DO_TEST_ATTACH("base-without-scsi-controller-live", "disk-scsi-2", false, true, VIR_DOMAIN_AFFECT_LIVE, /* Four controllers added */ "device_add", QMP_OK, "device_add", QMP_OK, @@ -497,11 +513,11 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), /* Disk added */ "device_add", QMP_OK); - DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false, + DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH_EVENT("base-without-scsi-controller-live", "disk-scsi-2", false, true, + DO_TEST_ATTACH_EVENT("base-without-scsi-controller-live", "disk-scsi-2", false, true, VIR_DOMAIN_AFFECT_LIVE, /* Four controllers added */ "device_add", QMP_OK, "device_add", QMP_OK, @@ -509,54 +525,57 @@ mymain(void) "device_add", QMP_OK, "human-monitor-command", HMP("OK\\r\\n"), /* Disk added */ - "device_add", QMP_OK); - DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", true, true, + "device_add", QMP_OK, + "qom-list", QOM_OK); + DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", true, true, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, + "qom-list", QOM_OK, "human-monitor-command", HMP("")); - DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false, + DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_DEVICE_DELETED("scsi3-0-5-7") QMP_OK, - "human-monitor-command", HMP("")); + "human-monitor-command", HMP(""), + "qom-list", QOM_OK); - DO_TEST_ATTACH("base-live", "qemu-agent", false, true, + DO_TEST_ATTACH("base-live", "qemu-agent", false, true, VIR_DOMAIN_AFFECT_LIVE, "chardev-add", QMP_OK, "device_add", QMP_OK); - DO_TEST_DETACH("base-live", "qemu-agent-detach", false, false, + DO_TEST_DETACH("base-live", "qemu-agent-detach", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "chardev-remove", QMP_OK); - DO_TEST_ATTACH("base-ccw-live", "ccw-virtio", false, true, + DO_TEST_ATTACH("base-ccw-live", "ccw-virtio", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-ccw-live", "ccw-virtio", false, false, + DO_TEST_DETACH("base-ccw-live", "ccw-virtio", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, true, + DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, false, + DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true, + DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, false, + DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, false, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); /* Attach a second device, then detach the first one. Then attach the first one again. */ - DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true, + DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); - DO_TEST_DETACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-explicit", false, true, + DO_TEST_DETACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-explicit", false, true, VIR_DOMAIN_AFFECT_LIVE, "device_del", QMP_OK, "human-monitor-command", HMP("")); - DO_TEST_ATTACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-reverse", false, false, + DO_TEST_ATTACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-reverse", false, false, VIR_DOMAIN_AFFECT_LIVE, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:53AM +0200, Tomasz Flendrich wrote:
More generic functions, qemuDomainAttachDeviceLiveAndConfig and qemuDomainDetachDeviceLiveAndConfig, are now used instead of other functions in qemuhotplugtest to attach and detach devices.
--- tests/qemuhotplugtest.c | 157 +++++++++++++++++++++++++++--------------------- 1 file changed, 88 insertions(+), 69 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index d862a0e..da361a2 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -23,6 +23,7 @@ #include "qemu/qemu_conf.h" #include "qemu/qemu_hotplug.h" #include "qemu/qemu_hotplugpriv.h" +#include "qemu/qemu_driver.h"
This will be qemu_domainpriv.h based on previous patches. Or, hopefully, just qemu_domain.h ;)
#include "qemumonitortestutils.h" #include "testutils.h" #include "testutilsqemu.h" @@ -52,6 +53,7 @@ struct qemuHotplugTestData { bool keep; virDomainObjPtr vm; bool deviceDeletedEvent; + unsigned int target;
virDomainModificationImpact would be more suitable here.
@@ -382,28 +387,30 @@ mymain(void) data.mon = my_mon; \ data.keep = kep; \ data.deviceDeletedEvent = event; \ + data.target = targt; \ if (virTestRun(name, testQemuHotplug, &data) < 0) \ ret = -1; \ } while (0)
-#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, false, fial, kep, __VA_ARGS__) +#define DO_TEST_ATTACH(file, dev, fial, kep, targt, ...) \ + DO_TEST(file, ATTACH, dev, false, fial, kep, targt, __VA_ARGS__)
Worse readability of the macros, but better readability of the tests would be if we had DO_TEST_ATTACH, DO_TEST_ATTACH_LIVE and DO_TEST_ATTACH_CONFIG (the first one would be both live and config).
@@ -418,77 +425,86 @@ mymain(void) " }" \ "}\r\n"
- DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, NULL); - DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, + DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, VIR_DOMAIN_AFFECT_LIVE, NULL); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, VIR_DOMAIN_AFFECT_LIVE,
We would also not have such long lines :)
It was previously working only with attachments/detachments to the live domain. Now it can test attaching/detaching to the persistent domain too. --- tests/qemuhotplugtest.c | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index da361a2..5cf29e5 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -234,12 +234,29 @@ testQemuHotplug(const void *data) if (virAsprintf(&domain_filename, "%s/qemuhotplugtestdomains/qemuhotplug-%s.xml", abs_srcdir, test->domain_filename) < 0 || virAsprintf(&device_filename, "%s/qemuhotplugtestdevices/qemuhotplug-%s.xml", - abs_srcdir, test->device_filename) < 0 || - virAsprintf(&result_filename, + abs_srcdir, test->device_filename) < 0) + goto cleanup; + + switch (target) { + case VIR_DOMAIN_AFFECT_LIVE: + if (virAsprintf(&result_filename, "%s/qemuhotplugtestdomains/qemuhotplug-%s+%s.xml", abs_srcdir, test->domain_filename, test->device_filename) < 0) + goto cleanup; + break; + case VIR_DOMAIN_AFFECT_CONFIG: + if (virAsprintf(&result_filename, + "%s/qemuhotplugtestdomains/qemuhotplug-%s+%s+config.xml", + abs_srcdir, test->domain_filename, + test->device_filename) < 0) + goto cleanup; + break; + default: + VIR_TEST_VERBOSE("target can either be VIR_DOMAIN_AFFECT_LIVE" + " or VIR_DOMAIN_AFFECT_CONFIG\n"); goto cleanup; + } if (virTestLoadFile(domain_filename, &domain_xml) < 0 || virTestLoadFile(device_filename, &device_xml) < 0) @@ -303,16 +320,26 @@ testQemuHotplug(const void *data) * envelope */ VIR_FREE(dev); } - if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm->def, result_xml, - result_filename, fail); + if (ret == 0 || fail) { + if (target == VIR_DOMAIN_AFFECT_LIVE) + ret = testQemuHotplugCheckResult(vm->def, result_xml, + result_filename, fail); + else if (target == VIR_DOMAIN_AFFECT_CONFIG) + ret = testQemuHotplugCheckResult(vm->newDef, result_xml, + result_filename, fail); + } break; case DETACH: ret = testQemuHotplugDetach(vm, dev, device_xml, target); - if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm->def, domain_xml, - domain_filename, fail); + if (ret == 0 || fail) { + if (target == VIR_DOMAIN_AFFECT_LIVE) + ret = testQemuHotplugCheckResult(vm->def, domain_xml, + domain_filename, fail); + else if (target == VIR_DOMAIN_AFFECT_CONFIG) + ret = testQemuHotplugCheckResult(vm->newDef, domain_xml, + domain_filename, fail); + } break; case UPDATE: -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:54AM +0200, Tomasz Flendrich wrote:
It was previously working only with attachments/detachments to the live domain. Now it can test attaching/detaching to the persistent domain too.
--- tests/qemuhotplugtest.c | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index da361a2..5cf29e5 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -234,12 +234,29 @@ testQemuHotplug(const void *data) if (virAsprintf(&domain_filename, "%s/qemuhotplugtestdomains/qemuhotplug-%s.xml", abs_srcdir, test->domain_filename) < 0 || virAsprintf(&device_filename, "%s/qemuhotplugtestdevices/qemuhotplug-%s.xml", - abs_srcdir, test->device_filename) < 0 || - virAsprintf(&result_filename, + abs_srcdir, test->device_filename) < 0) + goto cleanup; + + switch (target) { + case VIR_DOMAIN_AFFECT_LIVE: + if (virAsprintf(&result_filename, "%s/qemuhotplugtestdomains/qemuhotplug-%s+%s.xml", abs_srcdir, test->domain_filename, test->device_filename) < 0)
Indentation is off here.
+ goto cleanup; + break; + case VIR_DOMAIN_AFFECT_CONFIG: + if (virAsprintf(&result_filename, + "%s/qemuhotplugtestdomains/qemuhotplug-%s+%s+config.xml", + abs_srcdir, test->domain_filename, + test->device_filename) < 0) + goto cleanup; + break; + default:
No need to do default in case the switch condition is an enum, although I'm not sure how that works with flags (non-continuous values).
+ VIR_TEST_VERBOSE("target can either be VIR_DOMAIN_AFFECT_LIVE" + " or VIR_DOMAIN_AFFECT_CONFIG\n");
We should also take into account the fact that it can be both, but should not be _CURRENT in tests. That way we don't have to do two lines of DO_TEST_ATTACH_. Although that might cause confusion with the 'keep' parameter.
goto cleanup; + }
if (virTestLoadFile(domain_filename, &domain_xml) < 0 || virTestLoadFile(device_filename, &device_xml) < 0) @@ -303,16 +320,26 @@ testQemuHotplug(const void *data) * envelope */ VIR_FREE(dev); } - if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm->def, result_xml, - result_filename, fail); + if (ret == 0 || fail) { + if (target == VIR_DOMAIN_AFFECT_LIVE) + ret = testQemuHotplugCheckResult(vm->def, result_xml, + result_filename, fail); + else if (target == VIR_DOMAIN_AFFECT_CONFIG) + ret = testQemuHotplugCheckResult(vm->newDef, result_xml, + result_filename, fail); + } break;
case DETACH: ret = testQemuHotplugDetach(vm, dev, device_xml, target); - if (ret == 0 || fail) - ret = testQemuHotplugCheckResult(vm->def, domain_xml, - domain_filename, fail); + if (ret == 0 || fail) { + if (target == VIR_DOMAIN_AFFECT_LIVE) + ret = testQemuHotplugCheckResult(vm->def, domain_xml, + domain_filename, fail); + else if (target == VIR_DOMAIN_AFFECT_CONFIG) + ret = testQemuHotplugCheckResult(vm->newDef, domain_xml, + domain_filename, fail); + } break;
case UPDATE: -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
Indentation is off here.
Thank you for noticing that. Indeed, 4 spaces are missing.
+ goto cleanup;
+ break; + case VIR_DOMAIN_AFFECT_CONFIG: + if (virAsprintf(&result_filename, + "%s/qemuhotplugtestdomains/qemuhotplug-%s+%s+config.xml", + abs_srcdir, test->domain_filename, + test->device_filename) < 0) + goto cleanup; + break; + default:
No need to do default in case the switch condition is an enum, although I'm not sure how that works with flags (non-continuous values).
You are right. It will make more sense when I change these enums to flags.
+ VIR_TEST_VERBOSE("target can either be VIR_DOMAIN_AFFECT_LIVE"
+ " or VIR_DOMAIN_AFFECT_CONFIG\n");
We should also take into account the fact that it can be both, but should not be _CURRENT in tests. That way we don't have to do two lines of DO_TEST_ATTACH_. Although that might cause confusion with the 'keep' parameter.
I just wanted to make sure that both *_LIVE and *_CONFIG aren't supplied at once, because handling them both isn't implemented yet. Tomasz
This is the first testcase for qemuhotplugtest for attaching and detaching a device to the persistent domain. --- tests/qemuhotplugtest.c | 7 ++++ .../qemuhotplug-base-config+qemu-agent+config.xml | 45 ++++++++++++++++++++++ .../qemuhotplug-base-config.xml | 40 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 5cf29e5..1f94e67 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -606,6 +606,13 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); + DO_TEST_ATTACH("base-config", "qemu-agent", false, true, VIR_DOMAIN_AFFECT_CONFIG, + "chardev-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("base-config", "qemu-agent", false, false, VIR_DOMAIN_AFFECT_CONFIG, + "device_del", QMP_OK, + "chardev-remove", QMP_OK); + qemuTestDriverFree(&driver); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml new file mode 100644 index 0000000..e209f67 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <channel type='unix'> + <source mode='bind'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> + <seclabel type='none' model='none'/> +</domain> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml new file mode 100644 index 0000000..20ad0a5 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml @@ -0,0 +1,40 @@ +<domain type='kvm'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> + <seclabel type='none' model='none'/> +</domain> -- 2.7.4
On Sat, Jul 16, 2016 at 02:42:55AM +0200, Tomasz Flendrich wrote:
This is the first testcase for qemuhotplugtest for attaching and detaching a device to the persistent domain.
--- tests/qemuhotplugtest.c | 7 ++++ .../qemuhotplug-base-config+qemu-agent+config.xml | 45 ++++++++++++++++++++++ .../qemuhotplug-base-config.xml | 40 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 5cf29e5..1f94e67 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -606,6 +606,13 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK);
+ DO_TEST_ATTACH("base-config", "qemu-agent", false, true, VIR_DOMAIN_AFFECT_CONFIG, + "chardev-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("base-config", "qemu-agent", false, false, VIR_DOMAIN_AFFECT_CONFIG, + "device_del", QMP_OK, + "chardev-remove", QMP_OK); + qemuTestDriverFree(&driver); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml new file mode 100644 index 0000000..e209f67 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-config+qemu-agent+config.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <channel type='unix'> + <source mode='bind'/>
This line needs to be removed due to Jiri's patches. Also the test fails for me, but that might be just another upstream patch that was applied.
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml new file mode 100644 index 0000000..20ad0a5 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml
I don't think we need two base files, the live one should parse cleanly as config one. Although having this one makes it way easier to just see the diff of base and base+something to see the difference. OK, let's keep it here :)
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml
b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml new file mode 100644 index 0000000..20ad0a5 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-config.xml
I don't think we need two base files, the live one should parse cleanly as config one. Although having this one makes it way easier to just see the diff of base and base+something to see the difference. OK, let's keep it here :)
I agree that a live one would work in DO_TEST_ATTACH. We need the one with "-config" because in DO_TEST_DETACH it is used as the expected xml. Tomasz
On Sat, Jul 16, 2016 at 02:42:45AM +0200, Tomasz Flendrich wrote:
Changes in v2: * rebased
This patch requires another one, otherwise a small conflict appears: https://www.redhat.com/archives/libvir-list/2016-July/msg00402.html
In qemu_driver.c, two functions were split to have less responsibility. They were also modified so that they could be used in qemuhotplugtest. Now instead of running different functions to attach different device types, a generic function is run. Hopefully this will result in more device types tested in the future.
Suggestions on how to name these two new functions are very welcome.
In the future, if one uses the following command: virsh attach-device --live --config the new device will have the same address assigned in both live and config domain. To make sure that everything will work properly after making these changes, I reworked qemuhotplug.c to work with attaching devices to persistent domains.
So far, there is only one test for persistent attachment. I will add more testcases as soon as I hear some feedback on these changes. qemuhotplugtest.c should first be modified anyway, so that the three xmls's filenames (basis domain xml, device xml, expected xml) are stated explicitly instead of being calculated. This will allow for more flexibility in testing and less xml files duplicates.
ACK to patches 01-05, I'll push those in a while. I fixed up the nits pointed out in the review for some of those. Also reworded some subject lines to be a little more descriptive. The rest looks fine, it just needs a tiny bit more of modification so I would rather make it go through review again so that at least two pairs of eyes see it. Thanks a lot for your patience, Martin
participants (2)
-
Martin Kletzander -
Tomasz Flendrich