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

On a Tuesday in 2021, Daniel P. Berrangé wrote: > On Tue, Feb 02, 2021 at 05:18:51PM +0100, Ján Tomko wrote: > > 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(a)gmail.com&gt; > > > --- > > > 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? > > Make the check-aclrules also validate this new source file ? > > Ah, I thought they also verify that each driver method has at least one ACL call, but we have plenty of methods that are just wrappers for MethodFlags(..., 0);