Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
While at it, simplify the code.
==9104== 38 bytes in 2 blocks are definitely lost in loss record 1,943 of 3,250
==9104== at 0x483B8C0: malloc (vg_replace_malloc.c:442)
==9104== by 0x4DFB69B: g_malloc (gmem.c:130)
==9104== by 0x4E1921D: g_strdup (gstrfuncs.c:363)
==9104== by 0x495D60B: g_strdup_inline (gstrfuncs.h:321)
==9104== by 0x495D60B: virFCReadRportValue (virfcp.c:62)
==9104== by 0x4A5F5CB: virNodeDeviceGetSCSITargetCaps (node_device_conf.c:2914)
==9104== by 0xBF62529: udevProcessSCSITarget (node_device_udev.c:657)
==9104== by 0xBF62529: udevGetDeviceDetails (node_device_udev.c:1406)
==9104== by 0xBF62529: udevAddOneDevice (node_device_udev.c:1563)
==9104== by 0xBF639B5: udevProcessDeviceListEntry (node_device_udev.c:1637)
==9104== by 0xBF639B5: udevEnumerateDevices (node_device_udev.c:1691)
==9104== by 0xBF639B5: nodeStateInitializeEnumerate (node_device_udev.c:2009)
==9104== by 0x49BDBFD: virThreadHelper (virthread.c:256)
==9104== by 0x5242069: start_thread (in /usr/lib64/libc.so.6)
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
src/conf/node_device_conf.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 48140b17baa5..c146a9f0c881 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2894,9 +2894,9 @@ int
virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
virNodeDevCapSCSITarget *scsi_target)
{
- int ret = -1;
g_autofree char *dir = NULL;
g_autofree char *rport = NULL;
+ g_autofree char *wwpn = NULL;
VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
@@ -2906,28 +2906,21 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
rport = g_path_get_basename(dir);
if (!virFCIsCapableRport(rport))
- goto cleanup;
+ return -1;
+
+ if (virFCReadRportValue(rport, "port_name",
+ &wwpn) < 0) {
+ VIR_WARN("Failed to read port_name for '%s'", rport);
+ return -1;
+ }
VIR_FREE(scsi_target->rport);
scsi_target->rport = g_steal_pointer(&rport);
- if (virFCReadRportValue(scsi_target->rport, "port_name",
- &scsi_target->wwpn) < 0) {
- VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport);
- goto cleanup;
- }
-
+ VIR_FREE(scsi_target->wwpn);
+ scsi_target->wwpn = g_steal_pointer(&wwpn);
scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
- ret = 0;
-
- cleanup:
- if (ret < 0) {
- VIR_FREE(scsi_target->rport);
- VIR_FREE(scsi_target->wwpn);
- scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
- }
-
- return ret;
+ return 0;
}
--
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:
>Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
>While at it, simplify the code.
>
"While at it" usually means it should have been a separate commit,
especially if the simplification changes the behavior of the code.
>==9104== 38 bytes in 2 blocks are definitely lost in loss record 1,943 of 3,250
>==9104== at 0x483B8C0: malloc (vg_replace_malloc.c:442)
>==9104== by 0x4DFB69B: g_malloc (gmem.c:130)
>==9104== by 0x4E1921D: g_strdup (gstrfuncs.c:363)
>==9104== by 0x495D60B: g_strdup_inline (gstrfuncs.h:321)
>==9104== by 0x495D60B: virFCReadRportValue (virfcp.c:62)
>==9104== by 0x4A5F5CB: virNodeDeviceGetSCSITargetCaps (node_device_conf.c:2914)
>==9104== by 0xBF62529: udevProcessSCSITarget (node_device_udev.c:657)
>==9104== by 0xBF62529: udevGetDeviceDetails (node_device_udev.c:1406)
>==9104== by 0xBF62529: udevAddOneDevice (node_device_udev.c:1563)
>==9104== by 0xBF639B5: udevProcessDeviceListEntry (node_device_udev.c:1637)
>==9104== by 0xBF639B5: udevEnumerateDevices (node_device_udev.c:1691)
>==9104== by 0xBF639B5: nodeStateInitializeEnumerate (node_device_udev.c:2009)
>==9104== by 0x49BDBFD: virThreadHelper (virthread.c:256)
>==9104== by 0x5242069: start_thread (in /usr/lib64/libc.so.6)
>
>Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>---
> src/conf/node_device_conf.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
>diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>index 48140b17baa5..c146a9f0c881 100644
>--- a/src/conf/node_device_conf.c
>+++ b/src/conf/node_device_conf.c
>@@ -2894,9 +2894,9 @@ int
> virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
> virNodeDevCapSCSITarget *scsi_target)
> {
>- int ret = -1;
> g_autofree char *dir = NULL;
> g_autofree char *rport = NULL;
>+ g_autofree char *wwpn = NULL;
>
> VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
>
>@@ -2906,28 +2906,21 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
> rport = g_path_get_basename(dir);
>
> if (!virFCIsCapableRport(rport))
>- goto cleanup;
>+ return -1;
>+
>+ if (virFCReadRportValue(rport, "port_name",
>+ &wwpn) < 0) {
>+ VIR_WARN("Failed to read port_name for '%s'", rport);
>+ return -1;
>+ }
>
> VIR_FREE(scsi_target->rport);
> scsi_target->rport = g_steal_pointer(&rport);
>
>- if (virFCReadRportValue(scsi_target->rport, "port_name",
>- &scsi_target->wwpn) < 0) {
>- VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport);
>- goto cleanup;
>- }
>-
>+ VIR_FREE(scsi_target->wwpn);
>+ scsi_target->wwpn = g_steal_pointer(&wwpn);
> scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
>- ret = 0;
>-
>- cleanup:
>- if (ret < 0) {
>- VIR_FREE(scsi_target->rport);
>- VIR_FREE(scsi_target->wwpn);
>- scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
Before, we cleared out the info if we were not able to refresh it.
Is there a possibility that would lead to stale information?
Jano
>- }
>-
>- return ret;
>+ return 0;
> }
>
>
>--
>2.34.1
>_______________________________________________
>Devel mailing list -- devel@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
> On a Wednesday in 2024, Marc Hartmayer wrote:
>>Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
>>While at it, simplify the code.
>>
>
> "While at it" usually means it should have been a separate commit,
> especially if the simplification changes the behavior of the code.
>
Looks like this patch has been merged:
5138dd247870 ("node_device_conf: virNodeDeviceGetSCSITargetCaps: fix
memory leak")
Shall we revert it and apply the patch in the mail “[PATCH v1]
node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak”
instead?
[…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
On a Tuesday in 2024, Marc Hartmayer wrote:
>On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
>> On a Wednesday in 2024, Marc Hartmayer wrote:
>>>Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
>>>While at it, simplify the code.
>>>
>>
>> "While at it" usually means it should have been a separate commit,
>> especially if the simplification changes the behavior of the code.
>>
>
>Looks like this patch has been merged:
>
>5138dd247870 ("node_device_conf: virNodeDeviceGetSCSITargetCaps: fix
>memory leak")
>
Oh, I haven't seen Michal's R-b. Maybe there was a mailing list outage?
>Shall we revert it and apply the patch in the mail “[PATCH v1]
>node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak”
>instead?
>
I think that if you have to ask, it's not worth the hassle.
Jano
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Wed, Apr 10, 2024 at 09:42 AM +0200, Ján Tomko <jtomko@redhat.com> wrote:
> On a Tuesday in 2024, Marc Hartmayer wrote:
>>On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
>>> On a Wednesday in 2024, Marc Hartmayer wrote:
>>>>Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
>>>>While at it, simplify the code.
>>>>
>>>
>>> "While at it" usually means it should have been a separate commit,
>>> especially if the simplification changes the behavior of the code.
>>>
>>
>>Looks like this patch has been merged:
>>
>>5138dd247870 ("node_device_conf: virNodeDeviceGetSCSITargetCaps: fix
>>memory leak")
>>
>
> Oh, I haven't seen Michal's R-b. Maybe there was a mailing list
> outage?
No, it was my fault… I’ve sent the first version to the old libvirt ML +
Michal on CC…
>
>>Shall we revert it and apply the patch in the mail “[PATCH v1]
>>node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak”
>>instead?
>>
>
> I think that if you have to ask, it's not worth the hassle.
>
> Jano
--
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 03:55 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
> On a Wednesday in 2024, Marc Hartmayer wrote:
>>Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
>>While at it, simplify the code.
>>
>
> "While at it" usually means it should have been a separate commit,
> especially if the simplification changes the behavior of the code.
:)
[…snip…]
>>
>>diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>>index 48140b17baa5..c146a9f0c881 100644
>>--- a/src/conf/node_device_conf.c
>>+++ b/src/conf/node_device_conf.c
>>@@ -2894,9 +2894,9 @@ int
>> virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
>> virNodeDevCapSCSITarget *scsi_target)
>> {
>>- int ret = -1;
>> g_autofree char *dir = NULL;
>> g_autofree char *rport = NULL;
>>+ g_autofree char *wwpn = NULL;
>>
>> VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
>>
>>@@ -2906,28 +2906,21 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
>> rport = g_path_get_basename(dir);
>>
>> if (!virFCIsCapableRport(rport))
>>- goto cleanup;
>>+ return -1;
>>+
>>+ if (virFCReadRportValue(rport, "port_name",
>>+ &wwpn) < 0) {
>>+ VIR_WARN("Failed to read port_name for '%s'", rport);
>>+ return -1;
>>+ }
>>
>> VIR_FREE(scsi_target->rport);
>> scsi_target->rport = g_steal_pointer(&rport);
>>
>>- if (virFCReadRportValue(scsi_target->rport, "port_name",
>>- &scsi_target->wwpn) < 0) {
>>- VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport);
>>- goto cleanup;
>>- }
>>-
>>+ VIR_FREE(scsi_target->wwpn);
>>+ scsi_target->wwpn = g_steal_pointer(&wwpn);
>> scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
>>- ret = 0;
>>-
>>- cleanup:
>>- if (ret < 0) {
>>- VIR_FREE(scsi_target->rport);
>>- VIR_FREE(scsi_target->wwpn);
>>- scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
>
> Before, we cleared out the info if we were not able to refresh it.
Yep, that was the reason why have looked at other functions like
`virNodeDeviceGetSCSIHostCaps` to understand if that “clear-out” is by
intention or just a side-effect of the “cleanup” pattern here.
IMO, it’s an unwanted side-effect of the cleanup pattern, but yes, I
agree, we have to double check if that’s really the case.
> Is there a possibility that would lead to stale information?
With this change: If the function fails with -1 no values are changed
@scsi_target and that is what I would expect…
But if it’s a valid use case to clear out previous information in the
cleanup path and to return -1 hmm… why do we then not return 0 if it’s
no error case but something expected?
Thanks for the feedback.
>
> Jano
>
>>- }
>>-
>>- return ret;
>>+ return 0;
>> }
>>
>>
>>--
>>2.34.1
>>_______________________________________________
>>Devel mailing list -- devel@lists.libvirt.org
>>To unsubscribe send an email to devel-leave@lists.libvirt.org
--
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
© 2016 - 2025 Red Hat, Inc.