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

[Ján Tomko]

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 at gmail.com>
>> > ---
>> > 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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210202/bee4b6f0/attachment-0001.sig>