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

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(a)redhat.com&gt; > --- > 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?