Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak
On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote:
On 10/12/2016 06:40 AM, Erik Skultety wrote:
On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1357416
Rather than return a 0 or -1 and the *result string, return just the result string to the caller. Alter all the callers to handle the different return.
As a side effect or result of this, it's much clearer that we cannot just assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn fields - rather we should fetch a temporary string, then as long as our fetch was good, VIR_FREE what may have been there, and STEAL what we just got. This fixes a memory leak in the virNodeDeviceCreateXML code path through find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found in the device object capabilities.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I suppose I could have made two patches out of this, but it felt like overkill once I realized what the issue was. OK a third one line patch could have been added to change the virGetFCHostNameByWWN comment as well, but that'd really be overkill.
src/node_device/node_device_linux_sysfs.c | 55 ++++++++++++------------------- src/util/virutil.c | 34 ++++++++----------- src/util/virutil.h | 9 +++-- tests/fchosttest.c | 30 ++++++----------- 4 files changed, 49 insertions(+), 79 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 549d32c..be99c41 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) { - char *max_vports = NULL; - char *vports = NULL; + char *tmp = NULL; int ret = -1;
if (virReadSCSIUniqueId(NULL, d->scsi_host.host, @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) if (virIsCapableFCHost(NULL, d->scsi_host.host)) { d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
- if (virReadFCHost(NULL, - d->scsi_host.host, - "port_name", - &d->scsi_host.wwpn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) { VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.wwpn); + VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
- if (virReadFCHost(NULL, - d->scsi_host.host, - "node_name", - &d->scsi_host.wwnn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) { VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.wwnn); + VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
- if (virReadFCHost(NULL, - d->scsi_host.host, - "fabric_name", - &d->scsi_host.fabric_wwn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) { VIR_WARN("Failed to read fabric WWN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.fabric_wwn); + VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp); }
So if I understand correctly, the problem is basically that we did not call VIR_FREE on the data that had previously been filled to the virNodeDevCapData structure during replacing it with new data. So, we could just keep the signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR to the function, thus there won't be any need to use VIR_FREE+STEAL repeatedly in here.
True - we're overwriting the &d->scsi_host.{wwnn|wwpn|fabric_wwn} fields and that's the primary issue (e.g. mem leak).
While I understand your point, I'm not sure adding a VIR_FREE(*result) to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a proper solution since virReadFCHost does not "own" that memory. It doesn't state that if *result has something in it, the code will perform a VIR_FREE (although that's a nice convenience in this case). Sure it's simple enough to add that comment, but then I believe that the
I don't see a major problem here...
VIR_FREE() would have to be done at the top of the function; otherwise, how does the caller distinguish which error occurred when -1 gets returned and whether it should VIR_FREE itself?
Well, I have to admin that this^^ is a fair argument because there are 3 different spots where the function can fail, not that the caller could not check result for NULL but the fact that a function touched caller's argument and then failed would be just weird. So, yeah, good point.
Also, what if the caller didn't want the *result wiped out? What if it was using it to compare what it found "this" time vs. what was present in the data/file a previous time? That caller would then need to be adjusted to pass a temporary variable anyway.
Exactly, if a caller wanted to just compare 2 values - old and new one - they would use a temporary variable, that would IMHO be expected correct behaviour. Anyway, it's irrelevant now that I agree with your point above.
I think for me most compelling reason to alter virReadFCHost is that it follows the mindset of not returning two values from a function when one is perfectly reasonable.
John
BTW: The OCD would still kick in and want to change the comment below to match the function name...
Yeah, since the patch is going to be relatively beefy, this change will blend in fairly easily, so go ahead, ACK. Erik NOTE: I've recently switched to mutt and it's still quite new, so I forgot to hit 'g' for group reply, therefore most of this conversation 'officially' never happened, so CC'ng libvir-list at least for the last bit, I suppose the whole 'thread' is going to look odd, sigh....
-/* virGetHostNameByWWN: +/* virGetFCHostNameByWWN:
Normally, I'd say sure, you're absolutely right about not creating a separate patch for this kind of change, but once you make the adjustment I mentioned above none of the below changes will be necessary and thus this rename change would look sort of unrelated imho.
ACK with the adjustment.
Erik
* * Iterate over the sysfs tree to get FC host name (e.g. host5) * by the provided "wwnn,wwpn" pair. @@ -2298,7 +2293,7 @@ virFindFCHostCapableVport(const char *sysfs_prefix) if (!virIsCapableVport(prefix, host)) continue;
- if (virReadFCHost(prefix, host, "port_state", &state) < 0) { + if (!(state = virReadFCHost(prefix, host, "port_state"))) { VIR_DEBUG("Failed to read port_state for host%d", host); continue; } @@ -2310,12 +2305,12 @@ virFindFCHostCapableVport(const char *sysfs_prefix) } VIR_FREE(state);
- if (virReadFCHost(prefix, host, "max_npiv_vports", &max_vports) < 0) { + if (!(max_vports = virReadFCHost(prefix, host, "max_npiv_vports"))) { VIR_DEBUG("Failed to read max_npiv_vports for host%d", host); continue; }
- if (virReadFCHost(prefix, host, "npiv_vports_inuse", &vports) < 0) { + if (!(vports = virReadFCHost(prefix, host, "npiv_vports_inuse"))) { VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); VIR_FREE(max_vports); continue; @@ -2379,14 +2374,13 @@ virGetSCSIHostNameByParentaddr(unsigned int domain ATTRIBUTE_UNUSED, return NULL; }
-int +char * virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, int host ATTRIBUTE_UNUSED, - const char *entry ATTRIBUTE_UNUSED, - char **result ATTRIBUTE_UNUSED) + const char *entry ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); - return -1; + return NULL; }
bool diff --git a/src/util/virutil.h b/src/util/virutil.h index 703ec53..8c0d83c 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -182,11 +182,10 @@ virGetSCSIHostNameByParentaddr(unsigned int domain, unsigned int slot, unsigned int function, unsigned int unique_id); -int virReadFCHost(const char *sysfs_prefix, - int host, - const char *entry, - char **result) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +char *virReadFCHost(const char *sysfs_prefix, + int host, + const char *entry) + ATTRIBUTE_NONNULL(3);
bool virIsCapableFCHost(const char *sysfs_prefix, int host); bool virIsCapableVport(const char *sysfs_prefix, int host); diff --git a/tests/fchosttest.c b/tests/fchosttest.c index e9b89a7..a08a2e8 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -68,35 +68,25 @@ test3(const void *data ATTRIBUTE_UNUSED) char *vports = NULL; int ret = -1;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "node_name", - &wwnn) < 0) + if (!(wwnn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "node_name"))) return -1;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "port_name", - &wwpn) < 0) + if (!(wwpn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "port_name"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "fabric_name", - &fabric_wwn) < 0) + if (!(fabric_wwn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "fabric_name"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "max_npiv_vports", - &max_vports) < 0) + if (!(max_vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "max_npiv_vports"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "npiv_vports_inuse", - &vports) < 0) + if (!(vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "npiv_vports_inuse"))) goto cleanup;
if (STRNEQ(expect_wwnn, wwnn) || -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/12/2016 10:49 AM, Erik Skultety wrote:
On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote:
On 10/12/2016 06:40 AM, Erik Skultety wrote:
On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1357416
Rather than return a 0 or -1 and the *result string, return just the result string to the caller. Alter all the callers to handle the different return.
As a side effect or result of this, it's much clearer that we cannot just assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn fields - rather we should fetch a temporary string, then as long as our fetch was good, VIR_FREE what may have been there, and STEAL what we just got. This fixes a memory leak in the virNodeDeviceCreateXML code path through find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found in the device object capabilities.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I suppose I could have made two patches out of this, but it felt like overkill once I realized what the issue was. OK a third one line patch could have been added to change the virGetFCHostNameByWWN comment as well, but that'd really be overkill.
src/node_device/node_device_linux_sysfs.c | 55 ++++++++++++------------------- src/util/virutil.c | 34 ++++++++----------- src/util/virutil.h | 9 +++-- tests/fchosttest.c | 30 ++++++----------- 4 files changed, 49 insertions(+), 79 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 549d32c..be99c41 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) { - char *max_vports = NULL; - char *vports = NULL; + char *tmp = NULL; int ret = -1;
if (virReadSCSIUniqueId(NULL, d->scsi_host.host, @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) if (virIsCapableFCHost(NULL, d->scsi_host.host)) { d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
- if (virReadFCHost(NULL, - d->scsi_host.host, - "port_name", - &d->scsi_host.wwpn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) { VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.wwpn); + VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
- if (virReadFCHost(NULL, - d->scsi_host.host, - "node_name", - &d->scsi_host.wwnn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) { VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.wwnn); + VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
- if (virReadFCHost(NULL, - d->scsi_host.host, - "fabric_name", - &d->scsi_host.fabric_wwn) < 0) { + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) { VIR_WARN("Failed to read fabric WWN for host%d", d->scsi_host.host); goto cleanup; } + VIR_FREE(d->scsi_host.fabric_wwn); + VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp); }
So if I understand correctly, the problem is basically that we did not call VIR_FREE on the data that had previously been filled to the virNodeDevCapData structure during replacing it with new data. So, we could just keep the signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR to the function, thus there won't be any need to use VIR_FREE+STEAL repeatedly in here.
True - we're overwriting the &d->scsi_host.{wwnn|wwpn|fabric_wwn} fields and that's the primary issue (e.g. mem leak).
While I understand your point, I'm not sure adding a VIR_FREE(*result) to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a proper solution since virReadFCHost does not "own" that memory. It doesn't state that if *result has something in it, the code will perform a VIR_FREE (although that's a nice convenience in this case). Sure it's simple enough to add that comment, but then I believe that the
I don't see a major problem here...
VIR_FREE() would have to be done at the top of the function; otherwise, how does the caller distinguish which error occurred when -1 gets returned and whether it should VIR_FREE itself?
Well, I have to admin that this^^ is a fair argument because there are 3 different spots where the function can fail, not that the caller could not check result for NULL but the fact that a function touched caller's argument and then failed would be just weird. So, yeah, good point.
Also, what if the caller didn't want the *result wiped out? What if it was using it to compare what it found "this" time vs. what was present in the data/file a previous time? That caller would then need to be adjusted to pass a temporary variable anyway.
Exactly, if a caller wanted to just compare 2 values - old and new one - they would use a temporary variable, that would IMHO be expected correct behaviour. Anyway, it's irrelevant now that I agree with your point above.
I think for me most compelling reason to alter virReadFCHost is that it follows the mindset of not returning two values from a function when one is perfectly reasonable.
I actually thought this was the "more compelling" reason, but seeing as there's no other feedback - I'll make the simple patch for having the VIR_FREE() in virReadFCHost, adjust the comments, and move on. John
John
BTW: The OCD would still kick in and want to change the comment below to match the function name...
Yeah, since the patch is going to be relatively beefy, this change will blend in fairly easily, so go ahead, ACK.
Erik
NOTE: I've recently switched to mutt and it's still quite new, so I forgot to hit 'g' for group reply, therefore most of this conversation 'officially' never happened, so CC'ng libvir-list at least for the last bit, I suppose the whole 'thread' is going to look odd, sigh....
-/* virGetHostNameByWWN: +/* virGetFCHostNameByWWN:
Normally, I'd say sure, you're absolutely right about not creating a separate patch for this kind of change, but once you make the adjustment I mentioned above none of the below changes will be necessary and thus this rename change would look sort of unrelated imho.
ACK with the adjustment.
Erik
* * Iterate over the sysfs tree to get FC host name (e.g. host5) * by the provided "wwnn,wwpn" pair. @@ -2298,7 +2293,7 @@ virFindFCHostCapableVport(const char *sysfs_prefix) if (!virIsCapableVport(prefix, host)) continue;
- if (virReadFCHost(prefix, host, "port_state", &state) < 0) { + if (!(state = virReadFCHost(prefix, host, "port_state"))) { VIR_DEBUG("Failed to read port_state for host%d", host); continue; } @@ -2310,12 +2305,12 @@ virFindFCHostCapableVport(const char *sysfs_prefix) } VIR_FREE(state);
- if (virReadFCHost(prefix, host, "max_npiv_vports", &max_vports) < 0) { + if (!(max_vports = virReadFCHost(prefix, host, "max_npiv_vports"))) { VIR_DEBUG("Failed to read max_npiv_vports for host%d", host); continue; }
- if (virReadFCHost(prefix, host, "npiv_vports_inuse", &vports) < 0) { + if (!(vports = virReadFCHost(prefix, host, "npiv_vports_inuse"))) { VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); VIR_FREE(max_vports); continue; @@ -2379,14 +2374,13 @@ virGetSCSIHostNameByParentaddr(unsigned int domain ATTRIBUTE_UNUSED, return NULL; }
-int +char * virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, int host ATTRIBUTE_UNUSED, - const char *entry ATTRIBUTE_UNUSED, - char **result ATTRIBUTE_UNUSED) + const char *entry ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); - return -1; + return NULL; }
bool diff --git a/src/util/virutil.h b/src/util/virutil.h index 703ec53..8c0d83c 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -182,11 +182,10 @@ virGetSCSIHostNameByParentaddr(unsigned int domain, unsigned int slot, unsigned int function, unsigned int unique_id); -int virReadFCHost(const char *sysfs_prefix, - int host, - const char *entry, - char **result) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +char *virReadFCHost(const char *sysfs_prefix, + int host, + const char *entry) + ATTRIBUTE_NONNULL(3);
bool virIsCapableFCHost(const char *sysfs_prefix, int host); bool virIsCapableVport(const char *sysfs_prefix, int host); diff --git a/tests/fchosttest.c b/tests/fchosttest.c index e9b89a7..a08a2e8 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -68,35 +68,25 @@ test3(const void *data ATTRIBUTE_UNUSED) char *vports = NULL; int ret = -1;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "node_name", - &wwnn) < 0) + if (!(wwnn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "node_name"))) return -1;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "port_name", - &wwpn) < 0) + if (!(wwpn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "port_name"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "fabric_name", - &fabric_wwn) < 0) + if (!(fabric_wwn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "fabric_name"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "max_npiv_vports", - &max_vports) < 0) + if (!(max_vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "max_npiv_vports"))) goto cleanup;
- if (virReadFCHost(TEST_FC_HOST_PREFIX, - TEST_FC_HOST_NUM, - "npiv_vports_inuse", - &vports) < 0) + if (!(vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, + "npiv_vports_inuse"))) goto cleanup;
if (STRNEQ(expect_wwnn, wwnn) || -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
VIR_FREE() would have to be done at the top of the function; otherwise, how does the caller distinguish which error occurred when -1 gets returned and whether it should VIR_FREE itself?
Well, I have to admin that this^^ is a fair argument because there are 3 different spots where the function can fail, not that the caller could not check result for NULL but the fact that a function touched caller's argument and then failed would be just weird. So, yeah, good point.
I actually thought this was the "more compelling" reason, but seeing as there's no other feedback - I'll make the simple patch for having the VIR_FREE() in virReadFCHost, adjust the comments, and move on.
John
Hi John, I think I admitted that you had a very good point (the one on top) so I thought you would actually push your original version, I'm sorry if I wasn't clear enough with my statement. Erik
participants (2)
-
Erik Skultety -
John Ferlan