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);
     }
 
     if (virIsCapableVport(NULL, d->scsi_host.host)) {
         d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS;
 
-        if (virReadFCHost(NULL,
-                          d->scsi_host.host,
-                          "max_npiv_vports",
-                          &max_vports) < 0) {
+        if (!(tmp = virReadFCHost(NULL, d->scsi_host.host,
+                                  "max_npiv_vports"))) {
             VIR_WARN("Failed to read max_npiv_vports for host%d",
                      d->scsi_host.host);
             goto cleanup;
         }
 
-         if (virReadFCHost(NULL,
-                          d->scsi_host.host,
-                          "npiv_vports_inuse",
-                          &vports) < 0) {
-            VIR_WARN("Failed to read npiv_vports_inuse for host%d",
-                     d->scsi_host.host);
+        if (virStrToLong_i(tmp, NULL, 10, &d->scsi_host.max_vports) < 0) {
+            VIR_WARN("Failed to parse value of max_npiv_vports '%s'", tmp);
             goto cleanup;
         }
 
-        if (virStrToLong_i(max_vports, NULL, 10,
-                           &d->scsi_host.max_vports) < 0) {
-            VIR_WARN("Failed to parse value of max_npiv_vports '%s'",
-                      max_vports);
+         if (!(tmp = virReadFCHost(NULL, d->scsi_host.host,
+                                   "npiv_vports_inuse"))) {
+            VIR_WARN("Failed to read npiv_vports_inuse for host%d",
+                     d->scsi_host.host);
             goto cleanup;
         }
 
-        if (virStrToLong_i(vports, NULL, 10,
-                           &d->scsi_host.vports) < 0) {
-            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'",
-                     vports);
+        if (virStrToLong_i(tmp, NULL, 10, &d->scsi_host.vports) < 0) {
+            VIR_WARN("Failed to parse value of npiv_vports_inuse '%s'", tmp);
             goto cleanup;
         }
     }
@@ -132,8 +120,7 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
         VIR_FREE(d->scsi_host.wwpn);
         VIR_FREE(d->scsi_host.fabric_wwn);
     }
-    VIR_FREE(max_vports);
-    VIR_FREE(vports);
+    VIR_FREE(tmp);
     return ret;
 }
 
diff --git a/src/util/virutil.c b/src/util/virutil.c
index b57a195..2459d2d 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2008,24 +2008,21 @@ virGetSCSIHostNameByParentaddr(unsigned int domain,
  * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
  * @host: Host number, E.g. 5 of "fc_host/host5"
  * @entry: Name of the sysfs entry to read
- * @result: Return the entry value as string
  *
  * Read the value of sysfs "fc_host" entry.
  *
- * Returns 0 on success, and @result is filled with the entry value.
- * as string, Otherwise returns -1. Caller must free @result after
- * use.
+ * Returns result as a stringon success, caller must free @result after
+ * Otherwise returns NULL.
  */
-int
+char *
 virReadFCHost(const char *sysfs_prefix,
               int host,
-              const char *entry,
-              char **result)
+              const char *entry)
 {
     char *sysfs_path = NULL;
     char *p = NULL;
-    int ret = -1;
     char *buf = NULL;
+    char *result = NULL;
 
     if (virAsprintf(&sysfs_path, "%s/host%d/%s",
                     sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH,
@@ -2043,14 +2040,12 @@ virReadFCHost(const char *sysfs_prefix,
     else
         p = buf;
 
-    if (VIR_STRDUP(*result, p) < 0)
-        goto cleanup;
+    ignore_value(VIR_STRDUP(result, p));
 
-    ret = 0;
  cleanup:
     VIR_FREE(sysfs_path);
     VIR_FREE(buf);
-    return ret;
+    return result;
 }
 
 bool
@@ -2171,7 +2166,7 @@ virManageVport(const int parent_host,
     return ret;
 }
 
-/* virGetHostNameByWWN:
+/* virGetFCHostNameByWWN:
  *
  * 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

Reply via email to