11:48 a.m.
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
11:48 a.m.
New subject: [RFC PATCH 1/2] node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak
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(a)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
9:55 a.m.
New subject: [RFC PATCH 1/2] node_device_conf:
virNodeDeviceGetSCSITargetCaps: fix memory leak
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(a)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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
10:44 a.m.
New subject: [RFC PATCH 1/2] node_device_conf:
virNodeDeviceGetSCSITargetCaps: fix memory leak
On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko(a)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(a)lists.libvirt.org
>To unsubscribe send an email to devel-leave(a)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
2:50 a.m.
New subject: [RFC PATCH 1/2] node_device_conf:
virNodeDeviceGetSCSITargetCaps: fix memory leak
On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko(a)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
2:42 a.m.
New subject: [RFC PATCH 1/2] node_device_conf:
virNodeDeviceGetSCSITargetCaps: fix memory leak
On a Tuesday in 2024, Marc Hartmayer wrote:
On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko
<jtomko(a)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
3:26 a.m.
New subject: [RFC PATCH 1/2] node_device_conf:
virNodeDeviceGetSCSITargetCaps: fix memory leak
On Wed, Apr 10, 2024 at 09:42 AM +0200, Ján Tomko <jtomko(a)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(a)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
11:48 a.m.
New subject: [RFC PATCT 2/2] TODO virNodeDeviceUpdateCaps: checks missing?
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(a)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
10 a.m.
New subject: [RFC PATCT 2/2] TODO virNodeDeviceUpdateCaps: checks missing?
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(a)linux.ibm.com>
---
src/conf/node_device_conf.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
10:53 a.m.
New subject: [RFC PATCT 2/2] TODO virNodeDeviceUpdateCaps: checks missing?
On Thu, Mar 28, 2024 at 04:00 PM +0100, Ján Tomko <jtomko(a)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(a)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
3:17 a.m.
New subject: [RFC PATCT 2/2] TODO virNodeDeviceUpdateCaps: checks missing?
On Thu, Mar 28, 2024 at 04:53 PM +0100, "Marc Hartmayer"
<mhartmay(a)linux.ibm.com> wrote:
On Thu, Mar 28, 2024 at 04:00 PM +0100, Ján Tomko
<jtomko(a)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
39
days inactive
53
days old
10 comments
2 participants
participants (2)
-
Ján Tomko -
Marc Hartmayer