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

On Tue, Feb 02, 2021 at 05:32:14PM +0100, Ján Tomko wrote: > 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); The check-aclrules.py script has some logic which looks to see if the method contains a function call that resolves to another public API entrypoint, as a special case. So basically we'll need to process the hypervisor_driver.c file to extract a list of public API entrpoints with ACL checks, and then in special case the real drivers if they call one of these shared functions. Regards, Daniel