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

[Daniel Henrique Barboza]

On 2/2/21 1:35 PM, Daniel P. Berrangé wrote:
> 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 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...

Fair point. I can remove the 'Flags' name of the helper since the flags
are now being considered by the callers.


>>>>
>>>>> +{
>>>>> +    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 ?


I appreciate any pointers in doing that. I haven't found any 'not horrible' way
(e.g. creating a new condition to add domain_driver.c as a valid driver implementation
file) of doing it in check-aclrules.py.




Thanks,


DHB

>>>
>>>
>>
>> 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
>