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

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 ?

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|