On 8/23/23 3:52 AM, Michal Prívozník wrote:
On 8/21/23 21:32, Laine Stump wrote:In the past, the only allowable values for the "driver" field of virNodeDeviceDetachFlags() were "kvm" or "vfio" for the QEMU driver, and "xen" for the libxl driver. Then "kvm" was deprecated and removed, so the driver name became essentially irrelevant (because it is always called via a particular hypervisor driver, and so the "xen" or "vfio" can be (and almost always is) implied. With the advent of VFIO variant drivers, the ability to explicitly specify a driver name once again becomes useful - it can be used to name the exact VFIO driver that we want bound to the device in place of vfio-pci, so this patch allows those other names to be passed down the call chain, where the code in virpci.c can make use of them. The names "vfio", "kvm", and "xen" retain their special meaning, though: 1) because there may be some application or configuration that still calls virNodeDeviceDetachFlags() with driverName="vfio", this single value is substituted with the synonym of NULL, which means "bind the default driver for this device and hypervisor". This will currently result in the vfio-pci driver being bound to the device. 2) in the case of the libxl driver, "xen" means to use the standard driver used in the case of Xen ("pciback"). 3) "kvm" as a driver name always results in an error, as legacy KVM device assignment was removed from the kernel around 10 years ago. Signed-off-by: Laine Stump <laine@xxxxxxxxxx> --- src/hypervisor/domain_driver.c | 9 ++++----- src/hypervisor/domain_driver.h | 2 ++ src/libxl/libxl_driver.c | 3 ++- src/qemu/qemu_driver.c | 33 +++++++++++++++++++-------------- 4 files changed, 27 insertions(+), 20 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index a70f75f3ae..429784292a 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -462,6 +462,7 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, int virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, virHostdevManager *hostdevMgr, + virPCIStubDriver driverType, const char *driverName) { g_autoptr(virPCIDevice) pci = NULL; @@ -471,7 +472,7 @@ virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, g_autoptr(virConnect) nodeconn = NULL; g_autoptr(virNodeDevice) nodedev = NULL;- if (!driverName)+ if (driverType == VIR_PCI_STUB_DRIVER_NONE) return -1;Even though, we are very unlikely to hit this check, the rest of the function reports an error on error paths. Please virReportError() here too.
Nice catch! Although 1) this is pre-existing (added all the way back in commit 887dd0d33), and 2) with the current code it would be impossible for this error to occur (since both callers set driverType to something other than NONE), still there is no point in having the check there if it's just going to fail silently on that fateful day in the future when someone modifies the code such that all the callers *don't* set driverType != NONE.
I'll add an error log before I push.
