[PATCH v2 07/10] qemu_driver.c: validate 'driverName' earlier in qemuNodeDeviceDetachFlags()

[Daniel Henrique Barboza]

On 2/18/21 8:30 AM, John Ferlan wrote:
> 
> 
> On 2/2/21 8:06 PM, Daniel Henrique Barboza wrote:
>> The validation of 'driverName' does not depend on any other state and can be
>> done right on the start of the function. We can fail earlier while avoiding
>> a cleanup jump.
>>
>> Reviewed-by: Ján Tomko <jtomko at redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/qemu/qemu_driver.c | 41 ++++++++++++++++++++---------------------
>>   1 file changed, 20 insertions(+), 21 deletions(-)
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 64ae8fafc0..c6ba33e4ad 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -11982,6 +11982,25 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>>   
>>       virCheckFlags(0, -1);
>>   
>> +    if (!driverName)
>> +        driverName = "vfio";
>> +
>> +    if (STREQ(driverName, "vfio") && !vfio) {
>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                       _("VFIO device assignment is currently not "
>> +                         "supported on this system"));
>> +         return -1;
>> +    } else if (STREQ(driverName, "kvm")) {
>> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                       _("KVM device assignment is no longer "
>> +                         "supported on this system"));
>> +        return -1;
>> +    } else {
>> +        virReportError(VIR_ERR_INVALID_ARG,
>> +                       _("unknown driver name '%s'"), driverName);
>> +        return -1;
>> +    }
>> +
> 
> Coverity points out the rest of this is unreachable now (even with the
> subsequent patch).


oooffff. I'll fix it. Thanks for the heads up John!

> 
>>       if (!(nodeconn = virGetConnectNodeDev()))
>>           goto cleanup;
>>   
>> @@ -12013,27 +12032,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
>>       if (!pci)
>>           goto cleanup;
>>   
>> -    if (!driverName)
>> -        driverName = "vfio";
>> -
>> -    if (STREQ(driverName, "vfio")) {
>> -        if (!vfio) {
>> -            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> -                           _("VFIO device assignment is currently not "
>> -                             "supported on this system"));
>> -            goto cleanup;
>> -        }
>> -        virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
> 
> This section seems to be more than just code motion...  it used to pass
> thru here when vfio = true, but with the movement it would fall into the
> catch all else.
> 
> John
> 
>> -    } else if (STREQ(driverName, "kvm")) {
>> -        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> -                       _("KVM device assignment is no longer "
>> -                         "supported on this system"));
>> -        goto cleanup;
>> -    } else {
>> -        virReportError(VIR_ERR_INVALID_ARG,
>> -                       _("unknown driver name '%s'"), driverName);
>> -        goto cleanup;
>> -    }
>> +    virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
>>   
>>       ret = virHostdevPCINodeDeviceDetach(hostdev_mgr, pci);
>>    cleanup:
>>
>