I'm not familiar with the code so I cannot decide if ignoring the return values
is a bug or not. At least, it looks awkward and should be annotated.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/conf/node_device_conf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index c146a9f0c881..2f9abf5d9938 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2646,11 +2646,13 @@ virNodeDeviceUpdateCaps(virNodeDeviceDef *def)
     while (cap) {
         switch (cap->data.type) {
         case VIR_NODE_DEV_CAP_SCSI_HOST:
-            virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
+            if (virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host) < 0)
+                return -1;
             break;
         case VIR_NODE_DEV_CAP_SCSI_TARGET:
-            virNodeDeviceGetSCSITargetCaps(def->sysfs_path,
-                                           &cap->data.scsi_target);
+            if (virNodeDeviceGetSCSITargetCaps(def->sysfs_path,
+                                               &cap->data.scsi_target) < 0)
+                return -1;
             break;
         case VIR_NODE_DEV_CAP_NET:
             if (virNetDevGetLinkInfo(cap->data.net.ifname,
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On a Wednesday in 2024, Marc Hartmayer wrote:
>I'm not familiar with the code so I cannot decide if ignoring the return values
>is a bug or not. At least, it looks awkward and should be annotated.
>

Adding error reporting after years of real-world usage can be tricky (as
evidenced by the VPD error reporting reverts by Peter).

I think virNodeDeviceGetSCSITargetCaps erroring out if (!virFCIsCapableRport(rport))
is incorrect - there were non-FC SCSI targets long before the FC code
was added.

Jano

>Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>---
> src/conf/node_device_conf.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Mar 28, 2024 at 04:00 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
> On a Wednesday in 2024, Marc Hartmayer wrote:
>>I'm not familiar with the code so I cannot decide if ignoring the return values
>>is a bug or not. At least, it looks awkward and should be annotated.
>>
>
> Adding error reporting after years of real-world usage can be tricky (as
> evidenced by the VPD error reporting reverts by Peter).

Ohh yes, we should understand the code first.

>
> I think virNodeDeviceGetSCSITargetCaps erroring out if (!virFCIsCapableRport(rport))
> is incorrect - there were non-FC SCSI targets long before the FC code
> was added.

Ok, but then we should fix the virNodeDeviceGetSCSITargetCaps and add
the proposed check, otherwise the code looks just odd.

IMO, a better name for

virNodeDeviceGetSCSITargetCaps

and for the other functions

s/Get/Update/g

e.g.

virNodeDeviceUpdateSCSITargetCaps

…and check all other functions, e.g. virNetDevGetLinkInfo, and all?
other functions called by `virNodeDeviceUpdateCaps` have no clear out
functionality.

>
> Jano
>
>>Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>---
>> src/conf/node_device_conf.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Mar 28, 2024 at 04:53 PM +0100, "Marc Hartmayer" <mhartmay@linux.ibm.com> wrote:
> On Thu, Mar 28, 2024 at 04:00 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
>> On a Wednesday in 2024, Marc Hartmayer wrote:
>>>I'm not familiar with the code so I cannot decide if ignoring the return values
>>>is a bug or not. At least, it looks awkward and should be annotated.
>>>
>>
>> Adding error reporting after years of real-world usage can be tricky (as
>> evidenced by the VPD error reporting reverts by Peter).
>
> Ohh yes, we should understand the code first.
>
>>
>> I think virNodeDeviceGetSCSITargetCaps erroring out if (!virFCIsCapableRport(rport))
>> is incorrect - there were non-FC SCSI targets long before the FC code
>> was added.
>
> Ok, but then we should fix the virNodeDeviceGetSCSITargetCaps and add
> the proposed check, otherwise the code looks just odd.
>
> IMO, a better name for
>
> virNodeDeviceGetSCSITargetCaps
>
> and for the other functions
>
> s/Get/Update/g
>
> e.g.
>
> virNodeDeviceUpdateSCSITargetCaps
>
> …and check all other functions, e.g. virNetDevGetLinkInfo, and all?
> other functions called by `virNodeDeviceUpdateCaps` have no clear out
> functionality.

@Jan, what do you think?

For the time being I’ll send a simple memory leak fix without changing
the functionality.

[…snip]

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org