[libvirt PATCH 3/3] qemu: lookup node device against nodedev driver before getting XML

[Michal Prívozník]

On 10. 3. 2020 19:13, Daniel P. Berrangé wrote:
> Some of the node device APIs are a little odd because they accept a
> virNodeDevicePtr object but are still implemented by the virt drivers.
> The first thing the virt drivers need to do is get the XML config
> associated with the node device, and that means talking to the node
> device driver.
> 
> This worked previously because with monolithic libvirtd, both the
> virt driver and node device driver were in the same daemon and thus
> a single virConnectPtr can talk to both drivers.
> 
> With the split daemon world though, the virNodeDevicePtr passed into
> the APIs is associated with the QEMU driver virConnectPtr, which has
> no ability to invoke APIs against the node device driver. We must thus
> get a duplicate virNodeDevicePtr object which is associated with a
> virConnectPtr for the node device driver.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/libxl/libxl_driver.c | 63 ++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_driver.c   | 63 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 120 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index f2387e2a20..eca45da097 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5787,10 +5787,23 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
>      char *xml = NULL;
>      libxlDriverPrivatePtr driver = dev->conn->privateData;
>      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +    virConnectPtr nodeconn = NULL;
> +    virNodeDevicePtr nodedev = NULL;
>  
>      virCheckFlags(0, -1);
>  
> -    xml = virNodeDeviceGetXMLDesc(dev, 0);
> +    if (!(nodeconn = virGetConnectNodeDev()))
> +        goto cleanup;
> +
> +    /* 'dev' is associated with the QEMU virConnectPtr,
> +     * so for split daemons, we need to get a copy that
> +     * is associated with the virnodedevd daemon.
> +     */
> +    if (!(nodedev = virNodeDeviceLookupByName(
> +              nodeconn, virNodeDeviceGetName(dev))))

No need to split this line IMO. 85 characters long line is not that bad.

> +        goto cleanup;
> +
> +    xml = virNodeDeviceGetXMLDesc(nodedev, 0);
>      if (!xml)
>          goto cleanup;
>  
> @@ -5798,6 +5811,8 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
>      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;
>  
> @@ -5823,6 +5838,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
>   cleanup:
>      virPCIDeviceFree(pci);
>      virNodeDeviceDefFree(def);
> +    if (nodedev)
> +        virNodeDeviceFree(nodedev);

s/virNodeDeviceFree/virObjectUnref/ as we don't really need to call the
public API.

> +    if (nodeconn)
> +        virConnectClose(nodeconn);

And seems like we are preferring virObjectUnref() over virConnectClose()
too.

>      VIR_FREE(xml);
>      return ret;
>  }

Michal