[PATCH v3 03/12] test_driver: Implement virDomainDetachDeviceFlags

[Martin Kletzander]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211126/048e8d38/attachment-0001.sig>