Commit 76f47889326c4 made qemuNodeDeviceDetachFlags() unusable due to an
'if then else if' chain that will always results in a 'return -1',
regardless of 'driverName' input. This slipped through review process
and Gitlab CI, making it clear now that there is no unit tests
exercising this code ATM.

LUckily John Ferlan caught this up with his Coverity scan and spared us
from an unneeded headache later on.

Fixes: 76f47889326c45d2732711bc6dd5751aaf6e5194
Reported-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/qemu/qemu_driver.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f1035a536e..b9bbdf8d48 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11998,7 +11998,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
                           unsigned int flags)
 {
     virQEMUDriverPtr driver = dev->conn->privateData;
-    bool vfio = qemuHostdevHostSupportsPassthroughVFIO();
     virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 
     virCheckFlags(0, -1);
@@ -12006,22 +12005,27 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
     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")) {
+    /* Only the 'vfio' driver is supported and a special error message for
+     * the previously supported 'kvm' driver is provided below. */
+    if (STRNEQ(driverName, "vfio") && STRNEQ(driverName, "kvm")) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("unknown driver name '%s'"), driverName);
+        return -1;
+    }
+
+    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 (!qemuHostdevHostSupportsPassthroughVFIO()) {
+        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                       _("VFIO device assignment is currently not "
+                         "supported on this system"));
+         return -1;
+    }
 
     /* virNodeDeviceDetachFlagsEnsureACL() is being called by
      * virDomainDriverNodeDeviceDetachFlags() */
-- 
2.29.2

On 2/18/21 7:33 AM, Daniel Henrique Barboza wrote:
> Commit 76f47889326c4 made qemuNodeDeviceDetachFlags() unusable due to an
> 'if then else if' chain that will always results in a 'return -1',
> regardless of 'driverName' input. This slipped through review process
> and Gitlab CI, making it clear now that there is no unit tests
> exercising this code ATM.
> 
> LUckily John Ferlan caught this up with his Coverity scan and spared us
> from an unneeded headache later on.
> 
> Fixes: 76f47889326c45d2732711bc6dd5751aaf6e5194
> Reported-by: John Ferlan <jferlan@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  src/qemu/qemu_driver.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 

I can vouch this does make Coverity happy again.  I do agree w/ Jano no
need to be too elaborate on slipping thru cracks.... And when I do
create patches (rarely) now for Coverity, I'll just attribute it to
"Found by Coverity" and be done with it. You already put the Reported-by
so that's probably sufficient!

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

On 2/18/21 9:52 AM, John Ferlan wrote:
> 
> 
> On 2/18/21 7:33 AM, Daniel Henrique Barboza wrote:
>> Commit 76f47889326c4 made qemuNodeDeviceDetachFlags() unusable due to an
>> 'if then else if' chain that will always results in a 'return -1',
>> regardless of 'driverName' input. This slipped through review process
>> and Gitlab CI, making it clear now that there is no unit tests
>> exercising this code ATM.
>>
>> LUckily John Ferlan caught this up with his Coverity scan and spared us
>> from an unneeded headache later on.
>>
>> Fixes: 76f47889326c45d2732711bc6dd5751aaf6e5194
>> Reported-by: John Ferlan <jferlan@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   src/qemu/qemu_driver.c | 26 +++++++++++++++-----------
>>   1 file changed, 15 insertions(+), 11 deletions(-)
>>
> 
> I can vouch this does make Coverity happy again.  I do agree w/ Jano no
> need to be too elaborate on slipping thru cracks.... And when I do
> create patches (rarely) now for Coverity, I'll just attribute it to
> "Found by Coverity" and be done with it. You already put the Reported-by
> so that's probably sufficient!

It was my attempt to save face by claiming that I failed because there were no
unit tests :)

But you gotta a good point though. I'll remove this except before pushing it.


Thanks,


DHB


> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 

On a Thursday in 2021, Daniel Henrique Barboza wrote:
>Commit 76f47889326c4 made qemuNodeDeviceDetachFlags() unusable due to an
>'if then else if' chain that will always results in a 'return -1',
>regardless of 'driverName' input. This slipped through review process
>and Gitlab CI, making it clear now that there is no unit tests
>exercising this code ATM.

There is no need to enumerate all the entities that missed the mistake,
especially not the reviewer! O:-)

>
>LUckily John Ferlan caught this up with his Coverity scan and spared us

s/LU/Lu/

>from an unneeded headache later on.
>
>Fixes: 76f47889326c45d2732711bc6dd5751aaf6e5194
>Reported-by: John Ferlan <jferlan@redhat.com>
>Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>---
> src/qemu/qemu_driver.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>

with the typo fixed:
Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano