[PATCH 8/9] qemu, libxl, hypervisor: use virDomainDriverNodeDeviceDetachFlags() helper

[Ján Tomko]

On a Monday in 2021, Daniel Henrique Barboza wrote:
>libxlNodeDeviceDetachFlags() and qemuNodeDeviceDetachFlags() are mostly
>equal, aside from how the virHostdevmanager pointer is retrieved and
>the PCI stub driver used.
>
>Now that the PCI stub driver verification is done early in both functions,
>we can use the virDomainDriverNodeDeviceDetachFlags() helper to reduce
>code duplication between them. 'driverName' is checked inside the helper
>to set the appropriate stub driver.
>
>Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>---
> src/hypervisor/domain_driver.c | 60 ++++++++++++++++++++++++++++++++++
> src/hypervisor/domain_driver.h |  4 +++
> src/libvirt_private.syms       |  1 +
> src/libxl/libxl_driver.c       | 54 ++----------------------------
> src/qemu/qemu_driver.c         | 49 ++-------------------------
> 5 files changed, 71 insertions(+), 97 deletions(-)
>
>diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
>index ea4c3c9466..6ee74d6dff 100644
>--- a/src/hypervisor/domain_driver.c
>+++ b/src/hypervisor/domain_driver.c
>@@ -459,3 +459,63 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
>
>     return virHostdevPCINodeDeviceReAttach(hostdevMgr, pci);
> }
>+
>+int
>+virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev,
>+                                     virHostdevManagerPtr hostdevMgr,
>+                                     const char *driverName)

This helper does not even take flags, so the only reason to keep the
'Flags' in its name is to be consistent with the driver method it
implements...

>+{
>+    virPCIDevicePtr pci = NULL;
>+    virPCIDeviceAddress devAddr;
>+    int ret = -1;
>+    virNodeDeviceDefPtr def = NULL;
>+    g_autofree char *xml = NULL;
>+    virConnectPtr nodeconn = NULL;
>+    virNodeDevicePtr nodedev = NULL;
>+
>+    if (!driverName)
>+        return -1;
>+
>+    if (!(nodeconn = virGetConnectNodeDev()))
>+        goto cleanup;
>+
>+    /* 'dev' is associated with virConnectPtr, so for split
>+     * daemons, we need to get a copy that is associated with
>+     * the virnodedevd daemon. */
>+    if (!(nodedev = virNodeDeviceLookupByName(nodeconn,
>+                                              virNodeDeviceGetName(dev))))
>+        goto cleanup;
>+
>+    xml = virNodeDeviceGetXMLDesc(nodedev, 0);
>+    if (!xml)
>+        goto cleanup;
>+
>+    def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
>+    if (!def)
>+        goto cleanup;
>+
>+    /* ACL check must happen against original 'dev',
>+     * not the new 'nodedev' we acquired */
>+    if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
>+        goto cleanup;
>+

... and the ACL check required.

But moving the ACL checks into src/hypervisor means they are no longer
covered by the check-aclrules check.

I'd rather keep the ACL checks repetitive in each driver than risk the
chance of missing them, but that invalidates most of these refactors.

Any ideas?

Jano

>+    if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
>+        goto cleanup;
>+
>+    pci = virPCIDeviceNew(&devAddr);
>+    if (!pci)
>+        goto cleanup;
>+
>+    if (STREQ(driverName, "vfio"))
>+        virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
>+    else if (STREQ(driverName, "xen"))
>+        virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN);
>+
>+    ret = virHostdevPCINodeDeviceDetach(hostdevMgr, pci);
>+ cleanup:
>+    virPCIDeviceFree(pci);
>+    virNodeDeviceDefFree(def);
>+    virObjectUnref(nodedev);
>+    virObjectUnref(nodeconn);
>+    return ret;
>+}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210202/5928490d/attachment-0001.sig>