[libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak
John Ferlan
jferlan at redhat.com
Fri Oct 14 02:25:27 UTC 2016
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 at 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 at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list