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@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);
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
--
|: 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 :|
