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

2021-02-18 Thread 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 
Signed-off-by: Daniel Henrique Barboza 
---
  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).



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

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

2021-02-18 Thread 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 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  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:
>

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

2021-02-02 Thread Daniel Henrique Barboza 
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 
Signed-off-by: Daniel Henrique Barboza 
---
 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;
+}
+
 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);
-} 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:
-- 
2.26.2