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 <jfer...@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

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to