From 08c0ea16fcd3a4bf119ba3675431acfe75f29849 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@redhat.com>
Date: Fri, 27 Jan 2017 18:50:57 -0500
Subject: [PATCH] conf: Return the vHBA name from virNodeDeviceCreateVport

Rather than returning true/false and having the caller check if the
vHBA was actually created, let's do that check within the CreateVport
function. That way the caller can faithfully assume success based
on a name start the thread looking for the LUNs. Prior to this change
it's possible that the vHBA wasn't really created (e.g if the call to
virVHBAGetHostByWWN returned NULL), we'd claim success, but in reality
there'd be no vHBA for the pool. This also fixes a second yet seen
issue that if the nodedev was present, but the parent by name wasn't
provided (perhaps parent by wwnn/wwpn or by fabric_name), then a failure
would be returned. For this path it shouldn't be an error - we should
just be happy that something else is managing the device and we don't
have to create/delete it.

The end result is that the createVport code can now just start the
refresh thread once it gets a non NULL name back.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/node_device_conf.c        | 20 ++++++++++++--------
 src/conf/node_device_conf.h        |  2 +-
 src/storage/storage_backend_scsi.c | 24 ++++++++++--------------
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 0cf4214d3b..ba727468e8 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1925,13 +1925,12 @@ checkParent(virConnectPtr conn,
  * wwnn/wwpn, or fabric_wwn (if provided). If no parent is provided, then
  * a vport capable scsi_host will be selected.
  *
- * Returns 0 on success, -1 on failure
+ * Returns vHBA name on success, NULL on failure with an error message set
  */
-int
+char *
 virNodeDeviceCreateVport(virConnectPtr conn,
                          virStorageAdapterFCHostPtr fchost)
 {
-    int ret = -1;
     unsigned int parent_host;
     char *name = NULL;
     char *parent_hoststr = NULL;
@@ -1948,9 +1947,9 @@ virNodeDeviceCreateVport(virConnectPtr conn,
         /* If a parent was provided, let's make sure the 'name' we've
          * retrieved has the same parent. If not this will cause failure. */
         if (fchost->parent && checkParent(conn, name, fchost->parent))
-            ret = 0;
+            VIR_FREE(name);
 
-        goto cleanup;
+        return name;
     }
 
     if (fchost->parent) {
@@ -2002,12 +2001,17 @@ virNodeDeviceCreateVport(virConnectPtr conn,
                            VPORT_CREATE) < 0)
         goto cleanup;
 
-    ret = 0;
+    /* Let's ensure the device was created */
+    virWaitForDevices();
+    if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+        ignore_value(virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
+                                        VPORT_DELETE));
+        goto cleanup;
+    }
 
  cleanup:
-    VIR_FREE(name);
     VIR_FREE(parent_hoststr);
-    return ret;
+    return name;
 }
 
 
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 82e988afa0..a5d5cdd2a5 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -357,7 +357,7 @@ char *
 virNodeDeviceGetParentName(virConnectPtr conn,
                            const char *nodedev_name);
 
-int
+char *
 virNodeDeviceCreateVport(virConnectPtr conn,
                          virStorageAdapterFCHostPtr fchost);
 
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index e406923eba..f7378d34b0 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -240,27 +240,23 @@ createVport(virConnectPtr conn,
         }
     }
 
-    if (virNodeDeviceCreateVport(conn, fchost) < 0)
+    if (!(name = virNodeDeviceCreateVport(conn, fchost)))
         goto cleanup;
 
-    virWaitForDevices();
-
     /* Creating our own VPORT didn't leave enough time to find any LUN's,
      * so, let's create a thread whose job it is to call the FindLU's with
      * retry logic set to true. If the thread isn't created, then no big
      * deal since it's still possible to refresh the pool later.
      */
-    if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
-        if (VIR_ALLOC(cbdata) == 0) {
-            memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN);
-            VIR_STEAL_PTR(cbdata->fchost_name, name);
-
-            if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread,
-                                cbdata) < 0) {
-                /* Oh well - at least someone can still refresh afterwards */
-                VIR_DEBUG("Failed to create FC Pool Refresh Thread");
-                virStoragePoolFCRefreshDataFree(cbdata);
-            }
+    if (VIR_ALLOC(cbdata) == 0) {
+        memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN);
+        VIR_STEAL_PTR(cbdata->fchost_name, name);
+
+        if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread,
+                            cbdata) < 0) {
+            /* Oh well - at least someone can still refresh afterwards */
+            VIR_DEBUG("Failed to create FC Pool Refresh Thread");
+            virStoragePoolFCRefreshDataFree(cbdata);
         }
     }
 
-- 
2.47.2