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

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(a)redhat.com&gt; > Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > --- > 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: >