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@xxxxxxxxxx> >>>> --- >>>> >>>> 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@xxxxxxxxxx >>>> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list