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@xxxxxxxxx> > --- > 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); Jano
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 :|
Attachment:
signature.asc
Description: PGP signature
