[RFC PATCH 0/2] One memory leak fix and one question
Marc Hartmayer (2): node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak TODO virNodeDeviceUpdateCaps: checks missing? src/conf/node_device_conf.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) -- 2.34.1
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
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
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
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
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
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
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
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(-)
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
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
participants (2)
-
Ján Tomko -
Marc Hartmayer