[libvirt] [PATCH v1 23/31] qemu_domain: Introduce NVMe path getting helpers

[Peter Krempa]

On Thu, Jul 11, 2019 at 17:54:10 +0200, Michal Privoznik wrote:
> Couple of places in the QEMU driver will want to know what paths
> are associated with NVMe disks (for instance CGroup code or
> namespaces code). Introduce helpers which return desired paths
> (for instance /dev/vfio/vfio and /dev/vfio/N).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h |  6 ++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2a7f09ce24..949bbace88 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11723,6 +11723,50 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
>  }
>  
>  
> +char *
> +qemuDomainGetNVMeDiskPath(const virStorageSourceNVMeDef *nvme)

Name of this function is VERY misleading. It returns path of the IOMMU
group associated with the host portion of the NVMe device, but the name
implies smething completely different.

Include IOMMUGroup in the name and add a comment.

> +{
> +    VIR_AUTOPTR(virPCIDevice) pci = NULL;
> +
> +    /* All NVMe devices are VFIO PCI devices */
> +    if (!(pci = virPCIDeviceNew(nvme->pciAddr.domain,
> +                                nvme->pciAddr.bus,
> +                                nvme->pciAddr.slot,
> +                                nvme->pciAddr.function)))
> +        return NULL;
> +
> +    return virPCIDeviceGetIOMMUGroupDev(pci);
> +}
> +
> +
> +char **
> +qemuDomainGetDiskNVMePaths(const virDomainDef *def,

Also this name looks troubling.

> +                           const virStorageSource *src,
> +                           bool teardown)
> +{
> +    VIR_AUTOFREE(char *) iommuGroup = NULL;
> +    VIR_AUTOSTRINGLIST paths = NULL;
> +    bool includeVFIO = !teardown;
> +
> +    if (!(iommuGroup = qemuDomainGetNVMeDiskPath(src->nvme)))
> +        return NULL;
> +
> +    if (virStringListAdd(&paths, iommuGroup) < 0)
> +        return NULL;
> +
> +    if (teardown && def &&
> +        !virDomainDefHasNVMeDisk(def) &&
> +        !virDomainDefHasVFIOHostdev(def))
> +        includeVFIO = true;
> +
> +    if (includeVFIO &&
> +        virStringListAdd(&paths, QEMU_DEV_VFIO) < 0)
> +        return NULL;

I don't like this. It's hiding the stuff necessary to detach VFIO groups
in random function and the hostdev code will require exactly the same
treatment. Additionally any further possible VFIO based device would
require 3 places.

Can't we consolidate that somehow?
-------------- 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/20190716/ae2e378b/attachment-0001.sig>