[PATCH v3 00/12] Implement detach device related APIs for test driver
v3: - Rebase to current master branch - Split some tests to different functions in PATCH 12/12 and add some tests link to v2: https://listman.redhat.com/archives/libvir-list/2021-August/msg00637.html link to CI: https://gitlab.com/lukedyue/libvirt/-/pipelines/405150749 Luke Yue (12): conf: Introduce virDomainInputDefRemove and fix memory leak domain_driver: extract DetachXXXDeviceConfig related functions and use them test_driver: Implement virDomainDetachDeviceFlags test_driver: Implement virDomainDetachDeviceAlias test_driver: Implement virDomainDetachDevice conf: Add tpm helpers for future use test_driver: add TPM support for testDomainDetachDeviceLiveAndConfig conf: Add a memballoon helper for future use test_driver: add memballoon support for testDomainDetachDeviceLiveAndConfig examples: xml: test: add xml for testing devices related APIs virshtest: add expectError parameter to testCompareOutputLit tests: Test detach-device and detach-device-alias for test driver examples/xml/test/testdevcontroller.xml | 1 + examples/xml/test/testdevdiskcdrom.xml | 5 + examples/xml/test/testdevfs.xml | 6 + examples/xml/test/testdevhostdev.xml | 5 + examples/xml/test/testdevif.xml | 6 + examples/xml/test/testdevinput.xml | 1 + examples/xml/test/testdevlease.xml | 5 + examples/xml/test/testdevmem.xml | 6 + examples/xml/test/testdevmemballoon.xml | 3 + examples/xml/test/testdevrng.xml | 4 + examples/xml/test/testdevshmem.xml | 4 + examples/xml/test/testdevsound.xml | 3 + examples/xml/test/testdevtpm.xml | 5 + examples/xml/test/testdevvsock.xml | 3 + examples/xml/test/testdevwatchdog.xml | 1 + examples/xml/test/testdomfc5.xml | 54 +++++ examples/xml/test/testnodeinline.xml | 54 +++++ src/conf/domain_conf.c | 103 ++++++++ src/conf/domain_conf.h | 12 + src/hypervisor/domain_driver.c | 302 ++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 47 ++++ src/libvirt_private.syms | 20 ++ src/libxl/libxl_driver.c | 41 +--- src/lxc/lxc_driver.c | 37 +-- src/qemu/qemu_driver.c | 124 ++-------- src/test/test_driver.c | 239 +++++++++++++++++++ tests/virshtest.c | 157 +++++++++--- 27 files changed, 1054 insertions(+), 194 deletions(-) create mode 100644 examples/xml/test/testdevcontroller.xml create mode 100644 examples/xml/test/testdevdiskcdrom.xml create mode 100644 examples/xml/test/testdevfs.xml create mode 100644 examples/xml/test/testdevhostdev.xml create mode 100644 examples/xml/test/testdevif.xml create mode 100644 examples/xml/test/testdevinput.xml create mode 100644 examples/xml/test/testdevlease.xml create mode 100644 examples/xml/test/testdevmem.xml create mode 100644 examples/xml/test/testdevmemballoon.xml create mode 100644 examples/xml/test/testdevrng.xml create mode 100644 examples/xml/test/testdevshmem.xml create mode 100644 examples/xml/test/testdevsound.xml create mode 100644 examples/xml/test/testdevtpm.xml create mode 100644 examples/xml/test/testdevvsock.xml create mode 100644 examples/xml/test/testdevwatchdog.xml -- 2.33.1
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 3 ++- 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da0c64b460..3193120b79 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16901,6 +16901,18 @@ virDomainInputDefFind(const virDomainDef *def, } +virDomainInputDef * +virDomainInputDefRemove(virDomainDef *def, + size_t idx) +{ + virDomainInputDef *ret = def->inputs[idx]; + + VIR_DELETE_ELEMENT(def->inputs, idx, def->ninputs); + + return ret; +} + + bool virDomainVsockDefEquals(const virDomainVsockDef *a, const virDomainVsockDef *b) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ab9a7d66f8..a089b0b3de 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3851,6 +3851,8 @@ virDomainShmemDef *virDomainShmemDefRemove(virDomainDef *def, size_t idx) ssize_t virDomainInputDefFind(const virDomainDef *def, const virDomainInputDef *input) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +virDomainInputDef *virDomainInputDefRemove(virDomainDef *def, size_t idx) + ATTRIBUTE_NONNULL(1); bool virDomainVsockDefEquals(const virDomainVsockDef *a, const virDomainVsockDef *b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9ee8fda25f..000c9893f0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -478,6 +478,7 @@ virDomainInputBusTypeToString; virDomainInputDefFind; virDomainInputDefFree; virDomainInputDefGetPath; +virDomainInputDefRemove; virDomainInputSourceGrabToggleTypeFromString; virDomainInputSourceGrabToggleTypeToString; virDomainInputSourceGrabTypeFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6a77d9f513..cf9407c358 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7678,7 +7678,8 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef, _("matching input device not found")); return -1; } - VIR_DELETE_ELEMENT(vmdef->inputs, idx, vmdef->ninputs); + + virDomainInputDefFree(virDomainInputDefRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_VSOCK: -- 2.33.1
libxl / lxc / qemu drivers share some common codes in their DomainDetachDeviceConfig functions, so extract them to domain_driver and reuse them. At the same time, this will enable test driver to test these functions with virshtest in the future. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- Not pretty sure whether this is a proper way to make these functions reusable, maybe there is a more elegant choice. --- src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 41 +++++ src/libvirt_private.syms | 14 ++ src/libxl/libxl_driver.c | 41 +---- src/lxc/lxc_driver.c | 37 +---- src/qemu/qemu_driver.c | 123 +++------------ 6 files changed, 356 insertions(+), 166 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 31737b0f4a..01ecb4e30e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef, return ret; } + + +int +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainDiskDef *disk; + virDomainDiskDef *det_disk; + + disk = dev->data.disk; + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"), + disk->dst); + return -1; + } + virDomainDiskDefFree(det_disk); + + return 0; +} + +int +virDomainDriverDetachNetDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainNetDef *net; + int idx; + + net = dev->data.net; + if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + return -1; + + /* this is guaranteed to succeed */ + virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachSoundDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainSoundDef *sound; + int idx; + + sound = dev->data.sound; + if ((idx = virDomainSoundDefFind(vmdef, sound)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("device not present in domain configuration")); + return -1; + } + virDomainSoundDefFree(virDomainSoundDefRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachHostdevDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainHostdevDef *hostdev; + virDomainHostdevDef *det_hostdev; + int idx; + + hostdev = dev->data.hostdev; + if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("device not present in domain configuration")); + return -1; + } + virDomainHostdevRemove(vmdef, idx); + virDomainHostdevDefFree(det_hostdev); + + return 0; +} + + +int +virDomainDriverDetachLeaseDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainLeaseDef *lease; + virDomainLeaseDef *det_lease; + + lease = dev->data.lease; + if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) { + virReportError(VIR_ERR_DEVICE_MISSING, + _("Lease %s in lockspace %s does not exist"), + lease->key, NULLSTR(lease->lockspace)); + return -1; + } + virDomainLeaseDefFree(det_lease); + + return 0; +} + + +int +virDomainDriverDetachControllerDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainControllerDef *cont; + virDomainControllerDef *det_cont; + int idx; + + cont = dev->data.controller; + if ((idx = virDomainControllerFind(vmdef, cont->type, + cont->idx)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("device not present in domain configuration")); + return -1; + } + det_cont = virDomainControllerRemove(vmdef, idx); + virDomainControllerDefFree(det_cont); + + return 0; +} + + +int +virDomainDriverDetachFSDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainFSDef *fs; + int idx; + + fs = dev->data.fs; + idx = virDomainFSIndexByName(vmdef, fs->dst); + if (idx < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("no matching filesystem device was found")); + return -1; + } + + fs = virDomainFSRemove(vmdef, idx); + virDomainFSDefFree(fs); + + return 0; +} + + +int +virDomainDriverDetachRNGDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + int idx; + + if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("no matching RNG device was found")); + return -1; + } + + virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachMemoryDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainMemoryDef *mem; + int idx; + + if ((idx = virDomainMemoryFindInactiveByDef(vmdef, + dev->data.memory)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("matching memory device was not found")); + return -1; + } + mem = virDomainMemoryRemove(vmdef, idx); + vmdef->mem.cur_balloon -= mem->size; + virDomainMemoryDefFree(mem); + + return 0; +} + + +int +virDomainDriverDetachRedirdevDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + int idx; + + if ((idx = virDomainRedirdevDefFind(vmdef, + dev->data.redirdev)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("no matching redirdev was not found")); + return -1; + } + + virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachShmemDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + int idx; + + if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("matching shmem device was not found")); + return -1; + } + + virDomainShmemDefFree(virDomainShmemDefRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachWatchdogDeviceConfig(virDomainDef *vmdef) +{ + if (!vmdef->watchdog) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("domain has no watchdog")); + return -1; + } + virDomainWatchdogDefFree(vmdef->watchdog); + vmdef->watchdog = NULL; + + return 0; +} + + +int +virDomainDriverDetachInputDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + int idx; + + if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("matching input device not found")); + return -1; + } + + virDomainInputDefFree(virDomainInputDefRemove(vmdef, idx)); + + return 0; +} + + +int +virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + if (!vmdef->vsock || + !virDomainVsockDefEquals(dev->data.vsock, vmdef->vsock)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching vsock device not found")); + return -1; + } + virDomainVsockDefFree(vmdef->vsock); + vmdef->vsock = NULL; + + return 0; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 7b0fbae2fd..e7fbd70d7b 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -70,3 +70,44 @@ int virDomainDriverDelIOThreadCheck(virDomainDef *def, int virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef, virDomainIOThreadInfoPtr **info, unsigned int bitmap_size); + +int virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachNetDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachSoundDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachHostdevDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachLeaseDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachControllerDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachFSDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachRNGDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachMemoryDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachRedirdevDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachShmemDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachWatchdogDeviceConfig(virDomainDef *vmdef); + +int virDomainDriverDetachInputDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); + +int virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 000c9893f0..0252f7c9d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1551,6 +1551,20 @@ virDomainCgroupSetupMemtune; # hypervisor/domain_driver.h virDomainDriverAddIOThreadCheck; virDomainDriverDelIOThreadCheck; +virDomainDriverDetachControllerDeviceConfig; +virDomainDriverDetachDiskDeviceConfig; +virDomainDriverDetachFSDeviceConfig; +virDomainDriverDetachHostdevDeviceConfig; +virDomainDriverDetachInputDeviceConfig; +virDomainDriverDetachLeaseDeviceConfig; +virDomainDriverDetachMemoryDeviceConfig; +virDomainDriverDetachNetDeviceConfig; +virDomainDriverDetachRedirdevDeviceConfig; +virDomainDriverDetachRNGDeviceConfig; +virDomainDriverDetachShmemDeviceConfig; +virDomainDriverDetachSoundDeviceConfig; +virDomainDriverDetachVsockDeviceConfig; +virDomainDriverDetachWatchdogDeviceConfig; virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverGetIOThreadsConfig; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7ea157f9c4..1e3d40f448 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3912,56 +3912,29 @@ libxlDomainDetachDeviceLive(libxlDriverPrivate *driver, static int libxlDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) { - virDomainDiskDef *disk; - virDomainDiskDef *detach; - virDomainHostdevDef *hostdev; - virDomainHostdevDef *det_hostdev; - virDomainControllerDef *cont; - virDomainControllerDef *det_cont; - virDomainNetDef *net; - int idx; - switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - disk = dev->data.disk; - if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) { - virReportError(VIR_ERR_INVALID_ARG, - _("no target device %s"), disk->dst); + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainDiskDefFree(detach); + break; case VIR_DOMAIN_DEVICE_CONTROLLER: - cont = dev->data.controller; - if ((idx = virDomainControllerFind(vmdef, cont->type, - cont->idx)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0) return -1; - } - det_cont = virDomainControllerRemove(vmdef, idx); - virDomainControllerDefFree(det_cont); + break; case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) return -1; - /* this is guaranteed to succeed */ - virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_HOSTDEV: { - hostdev = dev->data.hostdev; - if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainHostdevRemove(vmdef, idx); - virDomainHostdevDefFree(det_hostdev); + break; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e2720a6f89..485683fe3a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3129,56 +3129,33 @@ static int lxcDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) { - int ret = -1; - virDomainDiskDef *disk; - virDomainDiskDef *det_disk; - virDomainNetDef *net; - virDomainHostdevDef *hostdev; - virDomainHostdevDef *det_hostdev; - int idx; - switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - disk = dev->data.disk; - if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { - virReportError(VIR_ERR_INVALID_ARG, - _("no target device %s"), disk->dst); + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainDiskDefFree(det_disk); - ret = 0; + break; case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) return -1; - /* this is guaranteed to succeed */ - virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); - ret = 0; break; case VIR_DOMAIN_DEVICE_HOSTDEV: { - hostdev = dev->data.hostdev; - if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainHostdevRemove(vmdef, idx); - virDomainHostdevDefFree(det_hostdev); - ret = 0; + break; } default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent detach of device is not supported")); - break; + return -1; } - return ret; + return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cf9407c358..d149cd22b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7517,84 +7517,43 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef, unsigned int parse_flags, virDomainXMLOption *xmlopt) { - virDomainDiskDef *disk; - virDomainDiskDef *det_disk; - virDomainNetDef *net; - virDomainSoundDef *sound; - virDomainHostdevDef *hostdev; - virDomainHostdevDef *det_hostdev; - virDomainLeaseDef *lease; - virDomainLeaseDef *det_lease; - virDomainControllerDef *cont; - virDomainControllerDef *det_cont; virDomainChrDef *chr; - virDomainFSDef *fs; - virDomainMemoryDef *mem; - int idx; switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_DISK: - disk = dev->data.disk; - if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { - virReportError(VIR_ERR_DEVICE_MISSING, - _("no target device %s"), disk->dst); + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainDiskDefFree(det_disk); + break; case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) return -1; - /* this is guaranteed to succeed */ - virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_SOUND: - sound = dev->data.sound; - if ((idx = virDomainSoundDefFind(vmdef, sound)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachSoundDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainSoundDefFree(virDomainSoundDefRemove(vmdef, idx)); + break; case VIR_DOMAIN_DEVICE_HOSTDEV: { - hostdev = dev->data.hostdev; - if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainHostdevRemove(vmdef, idx); - virDomainHostdevDefFree(det_hostdev); + break; } case VIR_DOMAIN_DEVICE_LEASE: - lease = dev->data.lease; - if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) { - virReportError(VIR_ERR_DEVICE_MISSING, - _("Lease %s in lockspace %s does not exist"), - lease->key, NULLSTR(lease->lockspace)); + if (virDomainDriverDetachLeaseDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainLeaseDefFree(det_lease); + break; case VIR_DOMAIN_DEVICE_CONTROLLER: - cont = dev->data.controller; - if ((idx = virDomainControllerFind(vmdef, cont->type, - cont->idx)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0) return -1; - } - det_cont = virDomainControllerRemove(vmdef, idx); - virDomainControllerDefFree(det_cont); break; @@ -7606,91 +7565,51 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef, break; case VIR_DOMAIN_DEVICE_FS: - fs = dev->data.fs; - idx = virDomainFSIndexByName(vmdef, fs->dst); - if (idx < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("no matching filesystem device was found")); + if (virDomainDriverDetachFSDeviceConfig(vmdef, dev) < 0) return -1; - } - fs = virDomainFSRemove(vmdef, idx); - virDomainFSDefFree(fs); break; case VIR_DOMAIN_DEVICE_RNG: - if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("no matching RNG device was found")); + if (virDomainDriverDetachRNGDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_MEMORY: - if ((idx = virDomainMemoryFindInactiveByDef(vmdef, - dev->data.memory)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("matching memory device was not found")); + if (virDomainDriverDetachMemoryDeviceConfig(vmdef, dev) < 0) return -1; - } - mem = virDomainMemoryRemove(vmdef, idx); - vmdef->mem.cur_balloon -= mem->size; - virDomainMemoryDefFree(mem); + break; case VIR_DOMAIN_DEVICE_REDIRDEV: - if ((idx = virDomainRedirdevDefFind(vmdef, - dev->data.redirdev)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("no matching redirdev was not found")); + if (virDomainDriverDetachRedirdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_SHMEM: - if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("matching shmem device was not found")); + if (virDomainDriverDetachShmemDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainShmemDefFree(virDomainShmemDefRemove(vmdef, idx)); break; - case VIR_DOMAIN_DEVICE_WATCHDOG: - if (!vmdef->watchdog) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("domain has no watchdog")); + if (virDomainDriverDetachWatchdogDeviceConfig(vmdef) < 0) return -1; - } - virDomainWatchdogDefFree(vmdef->watchdog); - vmdef->watchdog = NULL; + break; case VIR_DOMAIN_DEVICE_INPUT: - if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("matching input device not found")); + if (virDomainDriverDetachInputDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainInputDefFree(virDomainInputDefRemove(vmdef, idx)); break; case VIR_DOMAIN_DEVICE_VSOCK: - if (!vmdef->vsock || - !virDomainVsockDefEquals(dev->data.vsock, vmdef->vsock)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("matching vsock device not found")); + if (virDomainDriverDetachVsockDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainVsockDefFree(vmdef->vsock); - vmdef->vsock = NULL; + break; case VIR_DOMAIN_DEVICE_VIDEO: -- 2.33.1
On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
libxl / lxc / qemu drivers share some common codes in their DomainDetachDeviceConfig functions, so extract them to domain_driver and reuse them.
I would argue that these specific functions are actually dealing just with the domain definition which is done mostly in domain_conf.c, so I would pick that place.
At the same time, this will enable test driver to test these functions with virshtest in the future.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- Not pretty sure whether this is a proper way to make these functions reusable, maybe there is a more elegant choice.
The patch does a good job in deduplicating some of the code and except the file placement I think the overall approach is fine. However unfortunately the functions are just copy-paste from various places and they could be cleaned up to look the same. For example...
--- src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 41 +++++ src/libvirt_private.syms | 14 ++ src/libxl/libxl_driver.c | 41 +---- src/lxc/lxc_driver.c | 37 +---- src/qemu/qemu_driver.c | 123 +++------------ 6 files changed, 356 insertions(+), 166 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 31737b0f4a..01ecb4e30e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
return ret; } + + +int +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainDiskDef *disk; + virDomainDiskDef *det_disk; + + disk = dev->data.disk; + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"), + disk->dst); + return -1; + } + virDomainDiskDefFree(det_disk); + + return 0;
This function uses temporary variables for everything and adds an error message (which could be moved to the virDomainDiskRemoveByName() as all callers do report the same error message when it is not found. Other functions do essentially the same job, but have random comments, some have extra helper variables. All that could be nicely unified and it would read so much nicer.
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7ea157f9c4..1e3d40f448 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3912,56 +3912,29 @@ libxlDomainDetachDeviceLive(libxlDriverPrivate *driver, static int libxlDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) { - virDomainDiskDef *disk; - virDomainDiskDef *detach; - virDomainHostdevDef *hostdev; - virDomainHostdevDef *det_hostdev; - virDomainControllerDef *cont; - virDomainControllerDef *det_cont; - virDomainNetDef *net; - int idx; - switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - disk = dev->data.disk; - if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) { - virReportError(VIR_ERR_INVALID_ARG, - _("no target device %s"), disk->dst); + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainDiskDefFree(detach); + break;
case VIR_DOMAIN_DEVICE_CONTROLLER: - cont = dev->data.controller; - if ((idx = virDomainControllerFind(vmdef, cont->type, - cont->idx)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0) return -1; - } - det_cont = virDomainControllerRemove(vmdef, idx); - virDomainControllerDefFree(det_cont); + break;
case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) return -1;
- /* this is guaranteed to succeed */ - virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); break;
case VIR_DOMAIN_DEVICE_HOSTDEV: { - hostdev = dev->data.hostdev; - if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainHostdevRemove(vmdef, idx); - virDomainHostdevDefFree(det_hostdev); + break; }
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e2720a6f89..485683fe3a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3129,56 +3129,33 @@ static int lxcDomainDetachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) { - int ret = -1; - virDomainDiskDef *disk; - virDomainDiskDef *det_disk; - virDomainNetDef *net; - virDomainHostdevDef *hostdev; - virDomainHostdevDef *det_hostdev; - int idx; - switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - disk = dev->data.disk; - if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { - virReportError(VIR_ERR_INVALID_ARG, - _("no target device %s"), disk->dst); + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainDiskDefFree(det_disk); - ret = 0; + break;
case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) return -1;
- /* this is guaranteed to succeed */ - virDomainNetDefFree(virDomainNetRemove(vmdef, idx)); - ret = 0; break;
case VIR_DOMAIN_DEVICE_HOSTDEV: { - hostdev = dev->data.hostdev; - if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("device not present in domain configuration")); + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) return -1; - } - virDomainHostdevRemove(vmdef, idx); - virDomainHostdevDefFree(det_hostdev); - ret = 0; + break; }
default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent detach of device is not supported")); - break; + return -1; }
- return ret; + return 0; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cf9407c358..d149cd22b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7517,84 +7517,43 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef, unsigned int parse_flags, virDomainXMLOption *xmlopt)
What I really like about this change is that apart from roughly two device types, like video and chardev, this function could be de-duplicated as well. Adding a new functionality to that de-duplicated function would then add that functionality into all the drivers using this function. Of course the special devices would need some xmlopt callbacks or something that drivers can modify themselves for such reasons, but such change would be really, really helpful, especially in the long term.
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
libxl / lxc / qemu drivers share some common codes in their DomainDetachDeviceConfig functions, so extract them to domain_driver and reuse them.
I would argue that these specific functions are actually dealing just with the domain definition which is done mostly in domain_conf.c, so I would pick that place.
Thanks for the review! No problem, I would move the code to domain_conf.c in next version.
At the same time, this will enable test driver to test these functions with virshtest in the future.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- Not pretty sure whether this is a proper way to make these functions reusable, maybe there is a more elegant choice.
The patch does a good job in deduplicating some of the code and except the file placement I think the overall approach is fine.
However unfortunately the functions are just copy-paste from various places and they could be cleaned up to look the same. For example...
--- src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 41 +++++ src/libvirt_private.syms | 14 ++ src/libxl/libxl_driver.c | 41 +---- src/lxc/lxc_driver.c | 37 +---- src/qemu/qemu_driver.c | 123 +++------------ 6 files changed, 356 insertions(+), 166 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 31737b0f4a..01ecb4e30e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
return ret; } + + +int +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainDiskDef *disk; + virDomainDiskDef *det_disk; + + disk = dev->data.disk; + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"), + disk->dst); + return -1; + } + virDomainDiskDefFree(det_disk); + + return 0;
This function uses temporary variables for everything and adds an error message (which could be moved to the virDomainDiskRemoveByName() as all callers do report the same error message when it is not found. Other functions do essentially the same job, but have random comments, some have extra helper variables. All that could be nicely unified and it would read so much nicer.
What I really like about this change is that apart from roughly two device types, like video and chardev, this function could be de-duplicated as well. Adding a new functionality to that de- duplicated function would then add that functionality into all the drivers using this function. Of course the special devices would need some xmlopt callbacks or something that drivers can modify themselves for such reasons, but such change would be really, really helpful, especially in the long term.
So if I understand you correctly, I will unified all these functions into one, just like the original ones for QEMU / lxc / libxl, but this one is for all, and maybe I should create a new enum for flags of detachable device that different hypervisor support, and xmlopt etc for special devices. I will do that in next version. Thanks, Luke
On Mon, Nov 29, 2021 at 09:06:57PM +0800, Luke Yue wrote:
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
libxl / lxc / qemu drivers share some common codes in their DomainDetachDeviceConfig functions, so extract them to domain_driver and reuse them.
I would argue that these specific functions are actually dealing just with the domain definition which is done mostly in domain_conf.c, so I would pick that place.
Thanks for the review! No problem, I would move the code to domain_conf.c in next version.
At the same time, this will enable test driver to test these functions with virshtest in the future.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- Not pretty sure whether this is a proper way to make these functions reusable, maybe there is a more elegant choice.
The patch does a good job in deduplicating some of the code and except the file placement I think the overall approach is fine.
However unfortunately the functions are just copy-paste from various places and they could be cleaned up to look the same. For example...
--- src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 41 +++++ src/libvirt_private.syms | 14 ++ src/libxl/libxl_driver.c | 41 +---- src/lxc/lxc_driver.c | 37 +---- src/qemu/qemu_driver.c | 123 +++------------ 6 files changed, 356 insertions(+), 166 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 31737b0f4a..01ecb4e30e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
return ret; } + + +int +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainDiskDef *disk; + virDomainDiskDef *det_disk; + + disk = dev->data.disk; + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"), + disk->dst); + return -1; + } + virDomainDiskDefFree(det_disk); + + return 0;
This function uses temporary variables for everything and adds an error message (which could be moved to the virDomainDiskRemoveByName() as all callers do report the same error message when it is not found. Other functions do essentially the same job, but have random comments, some have extra helper variables. All that could be nicely unified and it would read so much nicer.
What I really like about this change is that apart from roughly two device types, like video and chardev, this function could be de-duplicated as well. Adding a new functionality to that de- duplicated function would then add that functionality into all the drivers using this function. Of course the special devices would need some xmlopt callbacks or something that drivers can modify themselves for such reasons, but such change would be really, really helpful, especially in the long term.
So if I understand you correctly, I will unified all these functions into one,
Not really into one, although if you figure out a way to make that easily possible feel free to go on, what I meant make them look and read the same.
just like the original ones for QEMU / lxc / libxl, but this one is for all, and maybe I should create a new enum for flags of detachable device that different hypervisor support, and xmlopt etc for special devices. I will do that in next version.
Deduplicating the bigger function would be nice to have for the future, it does not need to happen together in this one. It will require some more work to make it "modular" enough to be used by various drivers.
On Mon, 2021-11-29 at 15:33 +0100, Martin Kletzander wrote:
On Mon, Nov 29, 2021 at 09:06:57PM +0800, Luke Yue wrote:
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
libxl / lxc / qemu drivers share some common codes in their DomainDetachDeviceConfig functions, so extract them to domain_driver and reuse them.
I would argue that these specific functions are actually dealing just with the domain definition which is done mostly in domain_conf.c, so I would pick that place.
Thanks for the review! No problem, I would move the code to domain_conf.c in next version.
At the same time, this will enable test driver to test these functions with virshtest in the future.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- Not pretty sure whether this is a proper way to make these functions reusable, maybe there is a more elegant choice.
The patch does a good job in deduplicating some of the code and except the file placement I think the overall approach is fine.
However unfortunately the functions are just copy-paste from various places and they could be cleaned up to look the same. For example...
--- src/hypervisor/domain_driver.c | 266 +++++++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 41 +++++ src/libvirt_private.syms | 14 ++ src/libxl/libxl_driver.c | 41 +---- src/lxc/lxc_driver.c | 37 +---- src/qemu/qemu_driver.c | 123 +++------------ 6 files changed, 356 insertions(+), 166 deletions(-)
diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 31737b0f4a..01ecb4e30e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -644,3 +644,269 @@ virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
dst))) { + virReportError(VIR_ERR_DEVICE_MISSING, _("no target device %s"), + disk->dst); + return -1; + } + virDomainDiskDefFree(det_disk);
return ret; } + + +int +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainDiskDef *disk; + virDomainDiskDef *det_disk; + + disk = dev->data.disk; + if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk- + + return 0;
This function uses temporary variables for everything and adds an error message (which could be moved to the virDomainDiskRemoveByName() as all callers do report the same error message when it is not found. Other functions do essentially the same job, but have random comments, some have extra helper variables. All that could be nicely unified and it would read so much nicer.
What I really like about this change is that apart from roughly two device types, like video and chardev, this function could be de-duplicated as well. Adding a new functionality to that de- duplicated function would then add that functionality into all the drivers using this function. Of course the special devices would need some xmlopt callbacks or something that drivers can modify themselves for such reasons, but such change would be really, really helpful, especially in the long term.
So if I understand you correctly, I will unified all these functions into one,
Not really into one, although if you figure out a way to make that easily possible feel free to go on, what I meant make them look and read the same.
just like the original ones for QEMU / lxc / libxl, but this one is for all, and maybe I should create a new enum for flags of detachable device that different hypervisor support, and xmlopt etc for special devices. I will do that in next version.
Deduplicating the bigger function would be nice to have for the future, it does not need to happen together in this one. It will require some more work to make it "modular" enough to be used by various drivers.
OK, thanks, now I see.
Introduce testDomainChgDevice for further development (just like what we did for IOThread). And introduce testDomainDetachDeviceLiveAndConfig for detaching devices. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 202 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ea474d55ac..6a7eb12f77 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10051,6 +10051,207 @@ testConnectGetAllDomainStats(virConnectPtr conn, return ret; } +static int +testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + virDomainChrDef *chr; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + if (virDomainDriverDetachDiskDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_NET: + if (virDomainDriverDetachNetDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_SOUND: + if (virDomainDriverDetachSoundDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_HOSTDEV: + if (virDomainDriverDetachHostdevDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_LEASE: + if (virDomainDriverDetachLeaseDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + if (virDomainDriverDetachControllerDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_FS: + if (virDomainDriverDetachFSDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_RNG: + if (virDomainDriverDetachRNGDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_MEMORY: + if (virDomainDriverDetachMemoryDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_REDIRDEV: + if (virDomainDriverDetachRedirdevDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_SHMEM: + if (virDomainDriverDetachShmemDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_WATCHDOG: + if (virDomainDriverDetachWatchdogDeviceConfig(vmdef) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_INPUT: + if (virDomainDriverDetachInputDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_VSOCK: + if (virDomainDriverDetachVsockDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + + case VIR_DOMAIN_DEVICE_CHR: + if (!(chr = virDomainChrRemove(vmdef, dev->data.chr))) + return -1; + + virDomainChrDefFree(chr); + break; + + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_AUDIO: + case VIR_DOMAIN_DEVICE_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("detach of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); + return -1; + } + + return 0; +} + +static int +testDomainChgDevice(virDomainPtr dom, + virDomainDeviceAction action, + const char *xml, + const char *alias, + unsigned int flags) +{ + testDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + virDomainDef *def; + virDomainDeviceDef *dev = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(vm = testDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; + + if (!(def = virDomainObjGetOneDef(vm, flags))) + goto cleanup; + + if (action == VIR_DOMAIN_DEVICE_ACTION_DETACH) + parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + + if (xml) { + if (!(dev = virDomainDeviceDefParse(xml, def, driver->xmlopt, + driver->caps, parse_flags))) + goto cleanup; + } else if (alias) { + dev = g_new0(virDomainDeviceDef, 1); + if (virDomainDefFindDevice(def, alias, dev, true) < 0) + goto cleanup; + } + + if (dev == NULL) + goto cleanup; + + switch (action) { + case VIR_DOMAIN_DEVICE_ACTION_ATTACH: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("attaching devices is not supported")); + goto cleanup; + break; + + case VIR_DOMAIN_DEVICE_ACTION_DETACH: + if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0) + goto cleanup; + break; + + case VIR_DOMAIN_DEVICE_ACTION_UPDATE: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("updating devices is not supported")); + goto cleanup; + break; + } + + ret = 0; + + cleanup: + if (xml) { + virDomainDeviceDefFree(dev); + } else { + g_free(dev); + } + virDomainObjEndAPI(&vm); + return ret; +} + +static int +testDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + return testDomainChgDevice(dom, VIR_DOMAIN_DEVICE_ACTION_DETACH, + xml, NULL, flags); +} + /* * Test driver */ @@ -10148,6 +10349,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */ .domainFSThaw = testDomainFSThaw, /* 5.7.0 */ .domainFSTrim = testDomainFSTrim, /* 5.7.0 */ + .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 7.10.0 */ .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */ -- 2.33.1
On Wed, Nov 10, 2021 at 10:24:22PM +0800, Luke Yue wrote:
Introduce testDomainChgDevice for further development (just like what we did for IOThread). And introduce testDomainDetachDeviceLiveAndConfig for detaching devices.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 202 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ea474d55ac..6a7eb12f77 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10051,6 +10051,207 @@ testConnectGetAllDomainStats(virConnectPtr conn, return ret; }
+static int +testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev)
My comment from the previous patch would be nicely usable here as we could easily just make use of it. Also I see no reason for splitting the next two patches from this one. [...]
+ +static int +testDomainChgDevice(virDomainPtr dom, + virDomainDeviceAction action, + const char *xml, + const char *alias, + unsigned int flags) +{ + testDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + virDomainDef *def; + virDomainDeviceDef *dev = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1);
So here you check for both live and config, saying both of them are supported, ...
+ + if (!(vm = testDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; + + if (!(def = virDomainObjGetOneDef(vm, flags))) + goto cleanup; +
But here it would fail with both since you are explicitly saying you want just one definition.
+ if (action == VIR_DOMAIN_DEVICE_ACTION_DETACH) + parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + + if (xml) { + if (!(dev = virDomainDeviceDefParse(xml, def, driver->xmlopt, + driver->caps, parse_flags))) + goto cleanup; + } else if (alias) { + dev = g_new0(virDomainDeviceDef, 1); + if (virDomainDefFindDevice(def, alias, dev, true) < 0) + goto cleanup; + } + + if (dev == NULL) + goto cleanup; + + switch (action) { + case VIR_DOMAIN_DEVICE_ACTION_ATTACH: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("attaching devices is not supported")); + goto cleanup; + break; + + case VIR_DOMAIN_DEVICE_ACTION_DETACH: + if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0)
And I kind of see now why you call the function "LiveAndConfig" since it can do both based on what DomainDef you call it with. This function could be very easily modified to do both live and config properly.
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:22PM +0800, Luke Yue wrote:
Introduce testDomainChgDevice for further development (just like what we did for IOThread). And introduce testDomainDetachDeviceLiveAndConfig for detaching devices.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 202 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ea474d55ac..6a7eb12f77 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10051,6 +10051,207 @@ testConnectGetAllDomainStats(virConnectPtr conn, return ret; }
+static int +testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev)
My comment from the previous patch would be nicely usable here as we could easily just make use of it.
Also I see no reason for splitting the next two patches from this one.
OK, I will squash them in next version.
[...]
+ +static int +testDomainChgDevice(virDomainPtr dom, + virDomainDeviceAction action, + const char *xml, + const char *alias, + unsigned int flags) +{ + testDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + virDomainDef *def; + virDomainDeviceDef *dev = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1);
So here you check for both live and config, saying both of them are supported, ...
+ + if (!(vm = testDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; + + if (!(def = virDomainObjGetOneDef(vm, flags))) + goto cleanup; +
But here it would fail with both since you are explicitly saying you want just one definition.
xmlopt, + driver->caps,
+ if (action == VIR_DOMAIN_DEVICE_ACTION_DETACH) + parse_flags |= VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + + if (xml) { + if (!(dev = virDomainDeviceDefParse(xml, def, driver- parse_flags))) + goto cleanup; + } else if (alias) { + dev = g_new0(virDomainDeviceDef, 1); + if (virDomainDefFindDevice(def, alias, dev, true) < 0) + goto cleanup; + } + + if (dev == NULL) + goto cleanup; + + switch (action) { + case VIR_DOMAIN_DEVICE_ACTION_ATTACH: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("attaching devices is not supported")); + goto cleanup; + break; + + case VIR_DOMAIN_DEVICE_ACTION_DETACH: + if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0)
And I kind of see now why you call the function "LiveAndConfig" since it can do both based on what DomainDef you call it with. This function could be very easily modified to do both live and config properly.
Sorry, I missed that situation, will fix it in next version. Thanks, Luke
As we already implement testDomainChgDevice for both DetachDeviceFlags and DetachDeviceAlias, so it's simple to implement this API by changing the parameter. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6a7eb12f77..9843340fec 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10252,6 +10252,15 @@ testDomainDetachDeviceFlags(virDomainPtr dom, xml, NULL, flags); } +static int +testDomainDetachDeviceAlias(virDomainPtr dom, + const char *alias, + unsigned int flags) +{ + return testDomainChgDevice(dom, VIR_DOMAIN_DEVICE_ACTION_DETACH, + NULL, alias, flags); +} + /* * Test driver */ @@ -10349,6 +10358,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */ .domainFSThaw = testDomainFSThaw, /* 5.7.0 */ .domainFSTrim = testDomainFSTrim, /* 5.7.0 */ + .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 7.10.0 */ .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 7.10.0 */ .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */ -- 2.33.1
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/test/test_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 9843340fec..b1ca6a7b97 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10261,6 +10261,14 @@ testDomainDetachDeviceAlias(virDomainPtr dom, NULL, alias, flags); } +static int +testDomainDetachDevice(virDomainPtr dom, + const char *xml) +{ + return testDomainDetachDeviceFlags(dom, xml, + VIR_DOMAIN_AFFECT_LIVE); +} + /* * Test driver */ @@ -10358,6 +10366,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */ .domainFSThaw = testDomainFSThaw, /* 5.7.0 */ .domainFSTrim = testDomainFSTrim, /* 5.7.0 */ + .domainDetachDevice = testDomainDetachDevice, /* 7.10.0 */ .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 7.10.0 */ .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 7.10.0 */ .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */ -- 2.33.1
Currently it will only be used in the test driver. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 75 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3193120b79..512bfab9e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16931,6 +16931,73 @@ virDomainVsockDefEquals(const virDomainVsockDef *a, } +static bool +virDomainTPMDefEquals(const virDomainTPMDef *a, + const virDomainTPMDef *b) +{ + if (a->type != b->type) + return false; + + if (a->model != b->model) + return false; + + if (a->version != b->version) + return false; + + if (a->type == VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { + if (STRNEQ_NULLABLE(a->data.passthrough.source.data.file.path, + b->data.passthrough.source.data.file.path)) + return false; + } else { + if (a->data.emulator.hassecretuuid != b->data.emulator.hassecretuuid) + return false; + + if (a->data.emulator.hassecretuuid == true && + memcmp(a->data.emulator.secretuuid, + b->data.emulator.secretuuid, + VIR_UUID_BUFLEN)) + return false; + + if (a->data.emulator.persistent_state != + b->data.emulator.persistent_state) + return false; + } + + if (a->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&a->info, &b->info)) + return false; + + return true; +} + + +ssize_t +virDomainTPMDefFind(const virDomainDef *def, + const virDomainTPMDef *tpm) +{ + size_t i; + + for (i = 0; i < def->ntpms; i++) { + if (virDomainTPMDefEquals(tpm, def->tpms[i])) + return i; + } + + return -1; +} + + +virDomainTPMDef * +virDomainTPMDefRemove(virDomainDef *def, + size_t idx) +{ + virDomainTPMDef *ret = def->tpms[idx]; + + VIR_DELETE_ELEMENT(def->tpms, idx, def->ntpms); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDef *def, virCaps *caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a089b0b3de..715c8fbd16 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3857,6 +3857,12 @@ bool virDomainVsockDefEquals(const virDomainVsockDef *a, const virDomainVsockDef *b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +ssize_t virDomainTPMDefFind(const virDomainDef *def, + const virDomainTPMDef *tpm) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +virDomainTPMDef *virDomainTPMDefRemove(virDomainDef *def, size_t idx) + ATTRIBUTE_NONNULL(1); + VIR_ENUM_DECL(virDomainTaint); VIR_ENUM_DECL(virDomainTaintMessage); VIR_ENUM_DECL(virDomainVirt); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0252f7c9d6..68cc9c51cb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -665,7 +665,9 @@ virDomainTimerTrackTypeFromString; virDomainTimerTrackTypeToString; virDomainTPMBackendTypeFromString; virDomainTPMBackendTypeToString; +virDomainTPMDefFind; virDomainTPMDefFree; +virDomainTPMDefRemove; virDomainTPMModelTypeFromString; virDomainTPMModelTypeToString; virDomainTPMPcrBankTypeFromString; -- 2.33.1
On Wed, Nov 10, 2021 at 10:24:25PM +0800, Luke Yue wrote:
Currently it will only be used in the test driver.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 75 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3193120b79..512bfab9e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16931,6 +16931,73 @@ virDomainVsockDefEquals(const virDomainVsockDef *a, }
+static bool +virDomainTPMDefEquals(const virDomainTPMDef *a, + const virDomainTPMDef *b) +{ + if (a->type != b->type) + return false; + + if (a->model != b->model) + return false; + + if (a->version != b->version) + return false; + + if (a->type == VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { + if (STRNEQ_NULLABLE(a->data.passthrough.source.data.file.path, + b->data.passthrough.source.data.file.path))
This will not compile as `source` is a pointer here.
+ return false; + } else { + if (a->data.emulator.hassecretuuid != b->data.emulator.hassecretuuid) + return false; + + if (a->data.emulator.hassecretuuid == true && + memcmp(a->data.emulator.secretuuid, + b->data.emulator.secretuuid, + VIR_UUID_BUFLEN)) + return false; + + if (a->data.emulator.persistent_state != + b->data.emulator.persistent_state)
Also it would make sense to try to detach a device without all the information, e.g. those that libvirt itself adds by default if they are missing, or in this case when persistent_state is set to the specified default (in formatdomain.html this is said to be "no" currently, but not filled in). Luckily the test driver itself does not do any of this, so I guess it's fine here.
On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:25PM +0800, Luke Yue wrote:
Currently it will only be used in the test driver.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 75 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3193120b79..512bfab9e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16931,6 +16931,73 @@ virDomainVsockDefEquals(const virDomainVsockDef *a, }
+static bool +virDomainTPMDefEquals(const virDomainTPMDef *a, + const virDomainTPMDef *b) +{ + if (a->type != b->type) + return false; + + if (a->model != b->model) + return false; + + if (a->version != b->version) + return false; + + if (a->type == VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { + if (STRNEQ_NULLABLE(a-
data.passthrough.source.data.file.path, + b- data.passthrough.source.data.file.path))
This will not compile as `source` is a pointer here.
I just noticed in commit 42b0000, the `source` was changed to be a pointer, will fix in next version.
data.emulator.hassecretuuid) + return false;
+ return false; + } else { + if (a->data.emulator.hassecretuuid != b- + + if (a->data.emulator.hassecretuuid == true && + memcmp(a->data.emulator.secretuuid, + b->data.emulator.secretuuid, + VIR_UUID_BUFLEN)) + return false; + + if (a->data.emulator.persistent_state != + b->data.emulator.persistent_state)
Also it would make sense to try to detach a device without all the information, e.g. those that libvirt itself adds by default if they are missing, or in this case when persistent_state is set to the specified default (in formatdomain.html this is said to be "no" currently, but not filled in). Luckily the test driver itself does not do any of this, so I guess it's fine here.
Hmm, I will look into how libvirt adds these default devices and refine this function in next version or in the future. Thanks, Luke
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/hypervisor/domain_driver.c | 18 ++++++++++++++++++ src/hypervisor/domain_driver.h | 3 +++ src/libvirt_private.syms | 1 + src/test/test_driver.c | 7 ++++++- 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 01ecb4e30e..2461d977a3 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -910,3 +910,21 @@ virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef, return 0; } + + +int +virDomainDriverDetachTPMDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + int idx; + + if ((idx = virDomainTPMDefFind(vmdef, dev->data.tpm)) < 0) { + virReportError(VIR_ERR_DEVICE_MISSING, "%s", + _("matching tpm device not found")); + return -1; + } + + virDomainTPMDefFree(virDomainTPMDefRemove(vmdef, idx)); + + return 0; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index e7fbd70d7b..51dc109c38 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -111,3 +111,6 @@ int virDomainDriverDetachInputDeviceConfig(virDomainDef *vmdef, int virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev); + +int virDomainDriverDetachTPMDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68cc9c51cb..cfda58320a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1565,6 +1565,7 @@ virDomainDriverDetachRedirdevDeviceConfig; virDomainDriverDetachRNGDeviceConfig; virDomainDriverDetachShmemDeviceConfig; virDomainDriverDetachSoundDeviceConfig; +virDomainDriverDetachTPMDeviceConfig; virDomainDriverDetachVsockDeviceConfig; virDomainDriverDetachWatchdogDeviceConfig; virDomainDriverGenerateMachineName; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b1ca6a7b97..15b4332769 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10149,6 +10149,12 @@ testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, virDomainChrDefFree(chr); break; + case VIR_DOMAIN_DEVICE_TPM: + if (virDomainDriverDetachTPMDeviceConfig(vmdef, dev) < 0) + return -1; + + break; + case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: @@ -10156,7 +10162,6 @@ testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_NONE: - case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_AUDIO: -- 2.33.1
Currently it will only be used in test driver. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 512bfab9e9..92a8bd63f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16998,6 +16998,30 @@ virDomainTPMDefRemove(virDomainDef *def, } +bool +virDomainMemballoonDefEquals(const virDomainMemballoonDef *a, + const virDomainMemballoonDef *b) +{ + if (a->model != b->model) + return false; + + if (a->period != b->period) + return false; + + if (a->autodeflate != b->autodeflate) + return false; + + if (a->free_page_reporting != b->free_page_reporting) + return false; + + if (a->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&a->info, &b->info)) + return false; + + return true; +} + + char * virDomainDefGetDefaultEmulator(virDomainDef *def, virCaps *caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 715c8fbd16..f60ba37d19 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3863,6 +3863,10 @@ ssize_t virDomainTPMDefFind(const virDomainDef *def, virDomainTPMDef *virDomainTPMDefRemove(virDomainDef *def, size_t idx) ATTRIBUTE_NONNULL(1); +bool virDomainMemballoonDefEquals(const virDomainMemballoonDef *a, + const virDomainMemballoonDef *b) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + VIR_ENUM_DECL(virDomainTaint); VIR_ENUM_DECL(virDomainTaintMessage); VIR_ENUM_DECL(virDomainVirt); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cfda58320a..b143537bb4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -506,6 +506,7 @@ virDomainLoaderTypeFromString; virDomainLoaderTypeToString; virDomainLockFailureTypeFromString; virDomainLockFailureTypeToString; +virDomainMemballoonDefEquals; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemoryDefFree; -- 2.33.1
On Wed, Nov 10, 2021 at 10:24:27PM +0800, Luke Yue wrote:
Currently it will only be used in test driver.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 512bfab9e9..92a8bd63f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16998,6 +16998,30 @@ virDomainTPMDefRemove(virDomainDef *def, }
+bool +virDomainMemballoonDefEquals(const virDomainMemballoonDef *a, + const virDomainMemballoonDef *b)
It is really weird that you picked the membaloon as that particular one does not really make sense to be present multiple times, it is similar to the watchdog device. It is not a problem in this case, but I feel like this function is a bit useless.
On Fri, 2021-11-26 at 16:44 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:27PM +0800, Luke Yue wrote:
Currently it will only be used in test driver.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 512bfab9e9..92a8bd63f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16998,6 +16998,30 @@ virDomainTPMDefRemove(virDomainDef *def, }
+bool +virDomainMemballoonDefEquals(const virDomainMemballoonDef *a, + const virDomainMemballoonDef *b)
It is really weird that you picked the membaloon as that particular one does not really make sense to be present multiple times, it is similar to the watchdog device. It is not a problem in this case, but I feel like this function is a bit useless.
Oh, sorry, I didn't realize that, I forgot that the device would just present once, so that we can just remove the device without comparison, I will remove the comparison function. Thanks, Luke
As the memballoon device shouldn't be hot-(un)pluggable, so error if try to remove it on a running domain, for removing from config, it's fine. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/hypervisor/domain_driver.c | 18 ++++++++++++++++++ src/hypervisor/domain_driver.h | 3 +++ src/libvirt_private.syms | 1 + src/test/test_driver.c | 19 ++++++++++++++++--- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 2461d977a3..0a0ff7b361 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -928,3 +928,21 @@ virDomainDriverDetachTPMDeviceConfig(virDomainDef *vmdef, return 0; } + + +int +virDomainDriverDetachMemballoonDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev) +{ + if (!vmdef->memballoon || + !virDomainMemballoonDefEquals(dev->data.memballoon, + vmdef->memballoon)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching memballoon device not found")); + return -1; + } + virDomainMemballoonDefFree(vmdef->memballoon); + vmdef->memballoon = NULL; + + return 0; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 51dc109c38..5920854b13 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -114,3 +114,6 @@ int virDomainDriverDetachVsockDeviceConfig(virDomainDef *vmdef, int virDomainDriverDetachTPMDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev); + +int virDomainDriverDetachMemballoonDeviceConfig(virDomainDef *vmdef, + virDomainDeviceDef *dev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b143537bb4..19cc301a20 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1560,6 +1560,7 @@ virDomainDriverDetachFSDeviceConfig; virDomainDriverDetachHostdevDeviceConfig; virDomainDriverDetachInputDeviceConfig; virDomainDriverDetachLeaseDeviceConfig; +virDomainDriverDetachMemballoonDeviceConfig; virDomainDriverDetachMemoryDeviceConfig; virDomainDriverDetachNetDeviceConfig; virDomainDriverDetachRedirdevDeviceConfig; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 15b4332769..317aa0181b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -10053,7 +10053,8 @@ testConnectGetAllDomainStats(virConnectPtr conn, static int testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, - virDomainDeviceDef *dev) + virDomainDeviceDef *dev, + unsigned int flags) { virDomainChrDef *chr; @@ -10155,11 +10156,23 @@ testDomainDetachDeviceLiveAndConfig(virDomainDef *vmdef, break; + case VIR_DOMAIN_DEVICE_MEMBALLOON: + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("detach of device '%s' on running domain " + "is not supported"), + virDomainDeviceTypeToString(dev->type)); + return -1; + } else { + if (virDomainDriverDetachMemballoonDeviceConfig(vmdef, dev) < 0) + return -1; + } + break; + case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_PANIC: @@ -10225,7 +10238,7 @@ testDomainChgDevice(virDomainPtr dom, break; case VIR_DOMAIN_DEVICE_ACTION_DETACH: - if (testDomainDetachDeviceLiveAndConfig(def, dev) < 0) + if (testDomainDetachDeviceLiveAndConfig(def, dev, flags) < 0) goto cleanup; break; -- 2.33.1
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- examples/xml/test/testdevcontroller.xml | 1 + examples/xml/test/testdevdiskcdrom.xml | 5 +++ examples/xml/test/testdevfs.xml | 6 +++ examples/xml/test/testdevhostdev.xml | 5 +++ examples/xml/test/testdevif.xml | 6 +++ examples/xml/test/testdevinput.xml | 1 + examples/xml/test/testdevlease.xml | 5 +++ examples/xml/test/testdevmem.xml | 6 +++ examples/xml/test/testdevmemballoon.xml | 3 ++ examples/xml/test/testdevrng.xml | 4 ++ examples/xml/test/testdevshmem.xml | 4 ++ examples/xml/test/testdevsound.xml | 3 ++ examples/xml/test/testdevtpm.xml | 5 +++ examples/xml/test/testdevvsock.xml | 3 ++ examples/xml/test/testdevwatchdog.xml | 1 + examples/xml/test/testdomfc5.xml | 54 +++++++++++++++++++++++++ examples/xml/test/testnodeinline.xml | 54 +++++++++++++++++++++++++ 17 files changed, 166 insertions(+) create mode 100644 examples/xml/test/testdevcontroller.xml create mode 100644 examples/xml/test/testdevdiskcdrom.xml create mode 100644 examples/xml/test/testdevfs.xml create mode 100644 examples/xml/test/testdevhostdev.xml create mode 100644 examples/xml/test/testdevif.xml create mode 100644 examples/xml/test/testdevinput.xml create mode 100644 examples/xml/test/testdevlease.xml create mode 100644 examples/xml/test/testdevmem.xml create mode 100644 examples/xml/test/testdevmemballoon.xml create mode 100644 examples/xml/test/testdevrng.xml create mode 100644 examples/xml/test/testdevshmem.xml create mode 100644 examples/xml/test/testdevsound.xml create mode 100644 examples/xml/test/testdevtpm.xml create mode 100644 examples/xml/test/testdevvsock.xml create mode 100644 examples/xml/test/testdevwatchdog.xml diff --git a/examples/xml/test/testdevcontroller.xml b/examples/xml/test/testdevcontroller.xml new file mode 100644 index 0000000000..d855bfa17f --- /dev/null +++ b/examples/xml/test/testdevcontroller.xml @@ -0,0 +1 @@ +<controller type='ide' index='0'/> diff --git a/examples/xml/test/testdevdiskcdrom.xml b/examples/xml/test/testdevdiskcdrom.xml new file mode 100644 index 0000000000..edc90556bb --- /dev/null +++ b/examples/xml/test/testdevdiskcdrom.xml @@ -0,0 +1,5 @@ +<disk type='block' device='cdrom'> + <source dev='/dev/sr0'/> + <target dev='hdb' bus='ide'/> + <readonly/> +</disk> diff --git a/examples/xml/test/testdevfs.xml b/examples/xml/test/testdevfs.xml new file mode 100644 index 0000000000..cee5ed4ed9 --- /dev/null +++ b/examples/xml/test/testdevfs.xml @@ -0,0 +1,6 @@ +<filesystem type='file' accessmode='passthrough'> + <driver type='loop' format='raw'/> + <source file='/root/test/guest.img'/> + <target dir='/root/libvirt/test'/> + <readonly/> +</filesystem> diff --git a/examples/xml/test/testdevhostdev.xml b/examples/xml/test/testdevhostdev.xml new file mode 100644 index 0000000000..e364b50f36 --- /dev/null +++ b/examples/xml/test/testdevhostdev.xml @@ -0,0 +1,5 @@ +<hostdev mode='capabilities' type='storage'> + <source> + <block>/dev/sdf1</block>s + </source> +</hostdev> diff --git a/examples/xml/test/testdevif.xml b/examples/xml/test/testdevif.xml new file mode 100644 index 0000000000..7e0be80050 --- /dev/null +++ b/examples/xml/test/testdevif.xml @@ -0,0 +1,6 @@ +<interface type='network'> + <source network='testbrigde' /> + <mac address='00:11:22:33:44:55' /> + <model type='virtio' /> + <alias name='ua-testNIC' /> +</interface> diff --git a/examples/xml/test/testdevinput.xml b/examples/xml/test/testdevinput.xml new file mode 100644 index 0000000000..d958f5b931 --- /dev/null +++ b/examples/xml/test/testdevinput.xml @@ -0,0 +1 @@ +<input type='mouse' bus='virtio'/> diff --git a/examples/xml/test/testdevlease.xml b/examples/xml/test/testdevlease.xml new file mode 100644 index 0000000000..c53c0c7e7d --- /dev/null +++ b/examples/xml/test/testdevlease.xml @@ -0,0 +1,5 @@ +<lease> + <lockspace>testarea</lockspace> + <key>testkey</key> + <target path='/root/test/lease/path' offset='1024'/> +</lease> diff --git a/examples/xml/test/testdevmem.xml b/examples/xml/test/testdevmem.xml new file mode 100644 index 0000000000..49efd4af55 --- /dev/null +++ b/examples/xml/test/testdevmem.xml @@ -0,0 +1,6 @@ +<memory model='dimm' access='private' discard='yes'> + <target> + <size unit='KiB'>524287</size> + <node>0</node> + </target> +</memory> diff --git a/examples/xml/test/testdevmemballoon.xml b/examples/xml/test/testdevmemballoon.xml new file mode 100644 index 0000000000..fde18a9db6 --- /dev/null +++ b/examples/xml/test/testdevmemballoon.xml @@ -0,0 +1,3 @@ +<memballoon model='virtio'> + <stats period='10' /> +</memballoon> diff --git a/examples/xml/test/testdevrng.xml b/examples/xml/test/testdevrng.xml new file mode 100644 index 0000000000..369f423740 --- /dev/null +++ b/examples/xml/test/testdevrng.xml @@ -0,0 +1,4 @@ +<rng model='virtio'> + <rate period="2000" bytes="1234" /> + <backend model='builtin' /> +</rng> diff --git a/examples/xml/test/testdevshmem.xml b/examples/xml/test/testdevshmem.xml new file mode 100644 index 0000000000..04bf3d9a53 --- /dev/null +++ b/examples/xml/test/testdevshmem.xml @@ -0,0 +1,4 @@ +<shmem name='my_shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> +</shmem> diff --git a/examples/xml/test/testdevsound.xml b/examples/xml/test/testdevsound.xml new file mode 100644 index 0000000000..cf7323077f --- /dev/null +++ b/examples/xml/test/testdevsound.xml @@ -0,0 +1,3 @@ +<sound model='ich6'> + <codec type='micro'/> +</sound> diff --git a/examples/xml/test/testdevtpm.xml b/examples/xml/test/testdevtpm.xml new file mode 100644 index 0000000000..cc08b7bf6d --- /dev/null +++ b/examples/xml/test/testdevtpm.xml @@ -0,0 +1,5 @@ +<tpm model='tpm-tis'> + <backend type='emulator' version='2.0'> + <encryption secret='6dd3e4a5-1d76-44ce-961f-f119f5aad935'/> + </backend> +</tpm> diff --git a/examples/xml/test/testdevvsock.xml b/examples/xml/test/testdevvsock.xml new file mode 100644 index 0000000000..dda45780c4 --- /dev/null +++ b/examples/xml/test/testdevvsock.xml @@ -0,0 +1,3 @@ +<vsock model='virtio'> + <cid auto='no' address='3'/> +</vsock> diff --git a/examples/xml/test/testdevwatchdog.xml b/examples/xml/test/testdevwatchdog.xml new file mode 100644 index 0000000000..a02086f296 --- /dev/null +++ b/examples/xml/test/testdevwatchdog.xml @@ -0,0 +1 @@ +<watchdog model='i6300esb'/> diff --git a/examples/xml/test/testdomfc5.xml b/examples/xml/test/testdomfc5.xml index a8afc211f6..3b9edb9da9 100644 --- a/examples/xml/test/testdomfc5.xml +++ b/examples/xml/test/testdomfc5.xml @@ -29,6 +29,12 @@ <mac address='00:16:3e:5d:c7:26'/> <script path='vif-bridge'/> </interface> + <interface type='network'> + <source network='testbrigde' /> + <mac address='00:11:22:33:44:55' /> + <model type='virtio' /> + <alias name='ua-testNIC' /> + </interface> <disk type='file'> <source file='/root/fv0'/> <target dev='hda'/> @@ -36,6 +42,7 @@ <disk type='block' device='cdrom'> <source dev='/dev/sr0'/> <target dev='hdb' bus='ide'/> + <alias name='ua-testCD' /> <readonly/> </disk> <disk type='file' device='floppy'> @@ -47,5 +54,52 @@ <target dev='sda' bus='scsi'/> </disk> <graphics type='vnc' port='5904'/> + <sound model='ich6'> + <codec type='micro'/> + </sound> + <hostdev mode='capabilities' type='storage'> + <source> + <block>/dev/sdf1</block> + </source> + </hostdev> + <lease> + <lockspace>testarea</lockspace> + <key>testkey</key> + <target path='/root/test/lease/path' offset='1024'/> + </lease> + <controller type='ide' index='0'/> + <filesystem type='file' accessmode='passthrough'> + <driver type='loop' format='raw'/> + <source file='/root/test/guest.img'/> + <target dir='/root/libvirt/test'/> + <readonly/> + </filesystem> + <rng model='virtio'> + <rate period="2000" bytes="1234" /> + <backend model='builtin' /> + </rng> + <memory model='dimm' access='private' discard='yes'> + <target> + <size unit='KiB'>524287</size> + <node>0</node> + </target> + </memory> + <shmem name='my_shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + </shmem> + <watchdog model='i6300esb'/> + <input type='mouse' bus='virtio'/> + <vsock model='virtio'> + <cid auto='no' address='3'/> + </vsock> + <tpm model='tpm-tis'> + <backend type='emulator' version='2.0'> + <encryption secret='6dd3e4a5-1d76-44ce-961f-f119f5aad935'/> + </backend> + </tpm> + <memballoon model='virtio'> + <stats period='10' /> + </memballoon> </devices> </domain> diff --git a/examples/xml/test/testnodeinline.xml b/examples/xml/test/testnodeinline.xml index 9165d9302d..1124146f73 100644 --- a/examples/xml/test/testnodeinline.xml +++ b/examples/xml/test/testnodeinline.xml @@ -117,6 +117,12 @@ <mac address='00:16:3e:5d:c7:26'/> <script path='vif-bridge'/> </interface> + <interface type='network'> + <source network='testbrigde' /> + <mac address='00:11:22:33:44:55' /> + <model type='virtio' /> + <alias name='ua-testNIC' /> + </interface> <disk type='file'> <source file='/root/fv0'/> <target dev='hda'/> @@ -124,6 +130,7 @@ <disk type='block' device='cdrom'> <source dev='/dev/sr0'/> <target dev='hdb' bus='ide'/> + <alias name='ua-testCD' /> <readonly/> </disk> <disk type='file' device='floppy'> @@ -135,6 +142,53 @@ <target dev='sda' bus='scsi'/> </disk> <graphics type='vnc' port='5904'/> + <sound model='ich6'> + <codec type='micro'/> + </sound> + <hostdev mode='capabilities' type='storage'> + <source> + <block>/dev/sdf1</block> + </source> + </hostdev> + <lease> + <lockspace>testarea</lockspace> + <key>testkey</key> + <target path='/root/test/lease/path' offset='1024'/> + </lease> + <controller type='ide' index='0'/> + <filesystem type='file' accessmode='passthrough'> + <driver type='loop' format='raw'/> + <source file='/root/test/guest.img'/> + <target dir='/root/libvirt/test'/> + <readonly/> + </filesystem> + <rng model='virtio'> + <rate period="2000" bytes="1234" /> + <backend model='builtin' /> + </rng> + <memory model='dimm' access='private' discard='yes'> + <target> + <size unit='KiB'>524287</size> + <node>0</node> + </target> + </memory> + <shmem name='my_shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + </shmem> + <watchdog model='i6300esb'/> + <input type='mouse' bus='virtio'/> + <vsock model='virtio'> + <cid auto='no' address='3'/> + </vsock> + <tpm model='tpm-tis'> + <backend type='emulator' version='2.0'> + <encryption secret='6dd3e4a5-1d76-44ce-961f-f119f5aad935'/> + </backend> + </tpm> + <memballoon model='virtio'> + <stats period='10' /> + </memballoon> </devices> </domain> <network> -- 2.33.1
Add expectError so that we can make some test fail on purpose and compare the error message. The problem is that the test would pass, but the failed test would raise an internal error, with VIR_TEST_DEBUG=1, the error message would still be printed. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 61 +++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index c2c892c60d..af2a70f5fb 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -110,7 +110,7 @@ static int testFilterLine(char *buffer, } static int -testCompareOutputLit(const char *expectData, +testCompareOutputLit(const char *expectData, const char *expectError, const char *filter, const char *const argv[]) { g_autofree char *actualData = NULL; @@ -126,14 +126,19 @@ testCompareOutputLit(const char *expectData, virCommandSetOutputBuffer(cmd, &actualData); virCommandSetErrorBuffer(cmd, &errbuf); - if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, NULL) < 0 && STREQ(expectError, "")) return -1; - if (STRNEQ(errbuf, "")) { + if (STREQ(expectError, "") && STRNEQ(errbuf, "")) { fprintf(stderr, "Command reported error: %s", errbuf); return -1; } + if (STRNEQ(errbuf, expectError)) { + virTestDifference(stderr, errbuf, expectError); + return -1; + } + if (filter && testFilterLine(actualData, filter) < 0) return -1; @@ -163,7 +168,7 @@ static int testCompareListDefault(const void *data G_GNUC_UNUSED) ----------------------\n\ 1 test running\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareListCustom(const void *data G_GNUC_UNUSED) @@ -176,7 +181,7 @@ static int testCompareListCustom(const void *data G_GNUC_UNUSED) 2 fc4 running\n\ 3 fc5 running\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareNodeinfoDefault(const void *data G_GNUC_UNUSED) @@ -192,7 +197,7 @@ Thread(s) per core: 2\n\ NUMA cell(s): 2\n\ Memory size: 3145728 KiB\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareNodeinfoCustom(const void *data G_GNUC_UNUSED) @@ -212,112 +217,112 @@ Thread(s) per core: 2\n\ NUMA cell(s): 4\n\ Memory size: 8192000 KiB\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDominfoByID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "dominfo", "2", NULL }; const char *exp = dominfo_fc4; - return testCompareOutputLit(exp, "\nCPU time:", argv); + return testCompareOutputLit(exp, "", "\nCPU time:", argv); } static int testCompareDominfoByUUID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "dominfo", DOM_FC4_UUID, NULL }; const char *exp = dominfo_fc4; - return testCompareOutputLit(exp, "\nCPU time:", argv); + return testCompareOutputLit(exp, "", "\nCPU time:", argv); } static int testCompareDominfoByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "dominfo", "fc4", NULL }; const char *exp = dominfo_fc4; - return testCompareOutputLit(exp, "\nCPU time:", argv); + return testCompareOutputLit(exp, "", "\nCPU time:", argv); } static int testCompareTaintedDominfoByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "dominfo", "fc5", NULL }; const char *exp = dominfo_fc5; - return testCompareOutputLit(exp, "\nCPU time:", argv); + return testCompareOutputLit(exp, "", "\nCPU time:", argv); } static int testCompareDomuuidByID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domuuid", "2", NULL }; const char *exp = domuuid_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomuuidByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domuuid", "fc4", NULL }; const char *exp = domuuid_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomidByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domid", "fc4", NULL }; const char *exp = domid_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomidByUUID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domid", DOM_FC4_UUID, NULL }; const char *exp = domid_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomnameByID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domname", "2", NULL }; const char *exp = domname_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomnameByUUID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domname", DOM_FC4_UUID, NULL }; const char *exp = domname_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomstateByID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domstate", "2", NULL }; const char *exp = domstate_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomstateByUUID(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domstate", DOM_FC4_UUID, NULL }; const char *exp = domstate_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomstateByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domstate", "fc4", NULL }; const char *exp = domstate_fc4; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareDomControlInfoByName(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "domcontrol", "fc4", NULL }; const char *exp = "ok\n\n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareGetBlkioParameters(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_CUSTOM, "blkiotune", "fv0", NULL }; const char *exp = get_blkio_parameters; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testCompareSetBlkioParameters(const void *data G_GNUC_UNUSED) @@ -336,7 +341,7 @@ static int testCompareSetBlkioParameters(const void *data G_GNUC_UNUSED) " SET_BLKIO_PARAMETER ";\ blkiotune fv0", NULL }; const char *exp = set_blkio_parameters; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testIOThreadAdd(const void *data G_GNUC_UNUSED) @@ -357,7 +362,7 @@ static int testIOThreadAdd(const void *data G_GNUC_UNUSED) 4 0\n\ 6 0\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testIOThreadDel(const void *data G_GNUC_UNUSED) @@ -376,7 +381,7 @@ static int testIOThreadDel(const void *data G_GNUC_UNUSED) -----------------------------\n\ 4 0\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testIOThreadSet(const void *data G_GNUC_UNUSED) @@ -408,7 +413,7 @@ Domain: 'fc4'\n\ iothread.4.poll-max-ns" EQUAL "32768\n\ iothread.4.poll-grow" EQUAL "0\n\ iothread.4.poll-shrink" EQUAL "0\n\n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } static int testIOThreadPin(const void *data G_GNUC_UNUSED) @@ -429,7 +434,7 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) -----------------------------\n\ 2 0\n\ \n"; - return testCompareOutputLit(exp, NULL, argv); + return testCompareOutputLit(exp, "", NULL, argv); } struct testInfo { @@ -440,7 +445,7 @@ struct testInfo { static int testCompareEcho(const void *data) { const struct testInfo *info = data; - return testCompareOutputLit(info->result, NULL, info->argv); + return testCompareOutputLit(info->result, "", NULL, info->argv); } -- 2.33.1
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/tests/virshtest.c b/tests/virshtest.c index af2a70f5fb..8e5b397420 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -160,6 +160,8 @@ static char *custom_uri; "--connect", \ custom_uri +# define TEST_XML_PATH abs_top_builddir "/../examples/xml/test" + static int testCompareListDefault(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; @@ -437,6 +439,88 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, "", NULL, argv); } +static int testCompareDetachDevice(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevif.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevsound.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevhostdev.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevlease.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevcontroller.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevfs.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevrng.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevshmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevwatchdog.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevinput.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevvsock.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n"; + return testCompareOutputLit(exp, "", NULL, argv); +} + +static int testCompareDetachDeviceError(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5 --live\ + " TEST_XML_PATH "/testdevmemballoon.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevtpm.xml\n\ +error: device not found: matching tpm device not found\n\ +error: Failed to detach device from " TEST_XML_PATH "/testdevmemballoon.xml\n\ +error: Operation not supported: detach of device 'memballoon' on running domain is not supported\n"; + return testCompareOutputLit(exp, error_msg, NULL, argv); +} + +static int testCompareDetachDeviceAlias(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, + "detach-device-alias fc5 ua-testCD;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml", + NULL }; + const char *exp = "Device detach request sent successfully\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevdiskcdrom.xml\n\ +error: device not found: no target device hdb\n"; + return testCompareOutputLit(exp, error_msg, NULL, argv); +} + struct testInfo { const char *const *argv; const char *result; @@ -553,6 +637,18 @@ mymain(void) testIOThreadPin, NULL) != 0) ret = -1; + if (virTestRun("virsh detach-device", + testCompareDetachDevice, NULL) != 0) + ret = -1; + + if (virTestRun("virsh detach-device (with failure)", + testCompareDetachDeviceError, NULL) != 0) + ret = -1; + + if (virTestRun("virsh detach-device-alias", + testCompareDetachDeviceAlias, NULL) != 0) + ret = -1; + /* It's a bit awkward listing result before argument, but that's a * limitation of C99 vararg macros. */ # define DO_TEST(i, result, ...) \ -- 2.33.1
On Wed, Nov 10, 2021 at 10:24:31PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)
This makes the tests very fragile. It would be even easier to do the test using the internal APIs, similarly to how we do it with the qemuhotplugtest.
diff --git a/tests/virshtest.c b/tests/virshtest.c index af2a70f5fb..8e5b397420 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -160,6 +160,8 @@ static char *custom_uri; "--connect", \ custom_uri
+# define TEST_XML_PATH abs_top_builddir "/../examples/xml/test" + static int testCompareListDefault(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; @@ -437,6 +439,88 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, "", NULL, argv); }
+static int testCompareDetachDevice(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevif.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevsound.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevhostdev.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevlease.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevcontroller.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevfs.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevrng.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevshmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevwatchdog.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevinput.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevvsock.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n";
If any of these fails it will be a pain to figure out which one it was if the error message does not include the name. Splitting these is definitely the way to go.
+ return testCompareOutputLit(exp, "", NULL, argv); +} + +static int testCompareDetachDeviceError(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5 --live\ + " TEST_XML_PATH "/testdevmemballoon.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevtpm.xml\n\ +error: device not found: matching tpm device not found\n\ +error: Failed to detach device from " TEST_XML_PATH "/testdevmemballoon.xml\n\ +error: Operation not supported: detach of device 'memballoon' on running domain is not supported\n";
It'd be also nicer to read and write these tests if they did not rely on the output just like this and instead used the internal APIs.
On Fri, 2021-11-26 at 16:44 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:31PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)
This makes the tests very fragile. It would be even easier to do the test using the internal APIs, similarly to how we do it with the qemuhotplugtest.
Thanks for the explanation, I will look into how qemuhotplugtest is implemented.
diff --git a/tests/virshtest.c b/tests/virshtest.c index af2a70f5fb..8e5b397420 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -160,6 +160,8 @@ static char *custom_uri; "--connect", \ custom_uri
+# define TEST_XML_PATH abs_top_builddir "/../examples/xml/test" + static int testCompareListDefault(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; @@ -437,6 +439,88 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, "", NULL, argv); }
+static int testCompareDetachDevice(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevif.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevsound.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevhostdev.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevlease.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevcontroller.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevfs.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevrng.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevshmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevwatchdog.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevinput.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevvsock.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n";
If any of these fails it will be a pain to figure out which one it was if the error message does not include the name. Splitting these is definitely the way to go.
+ return testCompareOutputLit(exp, "", NULL, argv); +} + +static int testCompareDetachDeviceError(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5 --live\ + " TEST_XML_PATH "/testdevmemballoon.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevtpm.xml\n\ +error: device not found: matching tpm device not found\n\ +error: Failed to detach device from " TEST_XML_PATH "/testdevmemballoon.xml\n\ +error: Operation not supported: detach of device 'memballoon' on running domain is not supported\n";
It'd be also nicer to read and write these tests if they did not rely on the output just like this and instead used the internal APIs.
Should I create a new file for these tests? As this file is for `virsh` test but no for test using internal APIs? Thanks, Luke
On Mon, Nov 29, 2021 at 10:41:51PM +0800, Luke Yue wrote:
On Fri, 2021-11-26 at 16:44 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:31PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)
This makes the tests very fragile. It would be even easier to do the test using the internal APIs, similarly to how we do it with the qemuhotplugtest.
Thanks for the explanation, I will look into how qemuhotplugtest is implemented.
diff --git a/tests/virshtest.c b/tests/virshtest.c index af2a70f5fb..8e5b397420 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -160,6 +160,8 @@ static char *custom_uri; "--connect", \ custom_uri
+# define TEST_XML_PATH abs_top_builddir "/../examples/xml/test" + static int testCompareListDefault(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; @@ -437,6 +439,88 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, "", NULL, argv); }
+static int testCompareDetachDevice(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevif.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevsound.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevhostdev.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevlease.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevcontroller.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevfs.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevrng.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevshmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevwatchdog.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevinput.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevvsock.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n";
If any of these fails it will be a pain to figure out which one it was if the error message does not include the name. Splitting these is definitely the way to go.
+ return testCompareOutputLit(exp, "", NULL, argv); +} + +static int testCompareDetachDeviceError(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5 --live\ + " TEST_XML_PATH "/testdevmemballoon.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevtpm.xml\n\ +error: device not found: matching tpm device not found\n\ +error: Failed to detach device from " TEST_XML_PATH "/testdevmemballoon.xml\n\ +error: Operation not supported: detach of device 'memballoon' on running domain is not supported\n";
It'd be also nicer to read and write these tests if they did not rely on the output just like this and instead used the internal APIs.
Should I create a new file for these tests? As this file is for `virsh` test but no for test using internal APIs?
I would go with "generichotplugtest" or something like that.
Thanks, Luke
On Mon, 2021-11-29 at 15:50 +0100, Martin Kletzander wrote:
On Mon, Nov 29, 2021 at 10:41:51PM +0800, Luke Yue wrote:
On Fri, 2021-11-26 at 16:44 +0100, Martin Kletzander wrote:
On Wed, Nov 10, 2021 at 10:24:31PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- tests/virshtest.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)
This makes the tests very fragile. It would be even easier to do the test using the internal APIs, similarly to how we do it with the qemuhotplugtest.
Thanks for the explanation, I will look into how qemuhotplugtest is implemented.
diff --git a/tests/virshtest.c b/tests/virshtest.c index af2a70f5fb..8e5b397420 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -160,6 +160,8 @@ static char *custom_uri; "--connect", \ custom_uri
+# define TEST_XML_PATH abs_top_builddir "/../examples/xml/test" + static int testCompareListDefault(const void *data G_GNUC_UNUSED) { const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; @@ -437,6 +439,88 @@ static int testIOThreadPin(const void *data G_GNUC_UNUSED) return testCompareOutputLit(exp, "", NULL, argv); }
+static int testCompareDetachDevice(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevif.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevdiskcdrom.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevsound.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevhostdev.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevlease.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevcontroller.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevfs.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevrng.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevshmem.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevwatchdog.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevinput.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevvsock.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n\ +Device detached successfully\n\n";
If any of these fails it will be a pain to figure out which one it was if the error message does not include the name. Splitting these is definitely the way to go.
+ return testCompareOutputLit(exp, "", NULL, argv); +} + +static int testCompareDetachDeviceError(const void *data G_GNUC_UNUSED) +{ + const char *const argv[] = { VIRSH_CUSTOM, "detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5\ + " TEST_XML_PATH "/testdevtpm.xml;\ + detach-device fc5 --live\ + " TEST_XML_PATH "/testdevmemballoon.xml", + NULL }; + const char *exp = +"Device detached successfully\n\n\n\n"; + const char *error_msg = +"error: Failed to detach device from " TEST_XML_PATH "/testdevtpm.xml\n\ +error: device not found: matching tpm device not found\n\ +error: Failed to detach device from " TEST_XML_PATH "/testdevmemballoon.xml\n\ +error: Operation not supported: detach of device 'memballoon' on running domain is not supported\n";
It'd be also nicer to read and write these tests if they did not rely on the output just like this and instead used the internal APIs.
Should I create a new file for these tests? As this file is for `virsh` test but no for test using internal APIs?
I would go with "generichotplugtest" or something like that.
Thanks, will reimplement these tests with internal APIs in next version.
participants (2)
-
Luke Yue -
Martin Kletzander