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

[John Ferlan]

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

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