nodedev: Rework virNodeDeviceGetParentHost

Rework the code to perform the various searches by parent, parent_wwnn/
parent_wwpn, parent_fabric_wwn, or vport capable in order to return the
'parent_host' number that is vHBA capable.

The former virNodeDeviceGetParentHost is renamed to add the ByParent
on it fixes an issue where if no parent was supplied in the XML to
create the vHBA, then virNodeDeviceFindByName was called with a NULL
second parameter which had bad results.

The reworked code will make the various calls to fetch the NPIV host
by the passed parameter options or if none are provided find a vport
capable NPIV HBA to perform the create. If the call is from the delete
path, then this option won't be allowed.

Each of virNodeDeviceGetParentHostBy* functions is now static, so
remove them external definitions.

A secondary benefit of this is the test_driver now can make use of
the new API to add some new tests to test the various creation options.

src/conf/node_device_conf.c

@@ -1975,17 +1975,15 @@ virNodeDeviceGetWWNs(virNodeDeviceDefPtr def,
  */
 /* virNodeDeviceFindFCParentHost:
  * @parent: Pointer to node device object
- * @parent_host: Pointer to return parent host number
  *
  * Search the capabilities for the device to find the FC capabilities
  * in order to set the parent_host value.
  *
  * Returns:
- *   0 on success with parent_host set, -1 otherwise;
+ *   parent_host value on success (>= 0), -1 otherwise.
  */
 static int
-virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent,
-                              int *parent_host)
+virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent)
 {
     virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(parent);
 
@@ -1997,16 +1995,14 @@ virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent,
         return -1;
     }
 
-    *parent_host = cap->data.scsi_host.host;
-    return 0;
+    return cap->data.scsi_host.host;
 }
 
 
-int
-virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs,
-                           const char *dev_name,
-                           const char *parent_name,
-                           int *parent_host)
+static int
+virNodeDeviceGetParentHostByParent(virNodeDeviceObjListPtr devs,
+                                   const char *dev_name,
+                                   const char *parent_name)
 {
     virNodeDeviceObjPtr parent = NULL;
     int ret;
@@ -2018,7 +2014,7 @@ virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs,
         return -1;
     }
 
-    ret = virNodeDeviceFindFCParentHost(parent, parent_host);
+    ret = virNodeDeviceFindFCParentHost(parent);
 
     virNodeDeviceObjUnlock(parent);
 
@@ -2026,12 +2022,11 @@ virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs,
 }
 
 
-int
+static int
 virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
                                  const char *dev_name,
                                  const char *parent_wwnn,
-                                 const char *parent_wwpn,
-                                 int *parent_host)
+                                 const char *parent_wwpn)
 {
     virNodeDeviceObjPtr parent = NULL;
     int ret;
@@ -2043,7 +2038,7 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
         return -1;
     }
 
-    ret = virNodeDeviceFindFCParentHost(parent, parent_host);
+    ret = virNodeDeviceFindFCParentHost(parent);
 
     virNodeDeviceObjUnlock(parent);
 
@@ -2051,11 +2046,10 @@ virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
 }
 
 
-int
+static int
 virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs,
                                       const char *dev_name,
-                                      const char *parent_fabric_wwn,
-                                      int *parent_host)
+                                      const char *parent_fabric_wwn)
 {
     virNodeDeviceObjPtr parent = NULL;
     int ret;
@@ -2067,7 +2061,7 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs,
         return -1;
     }
 
-    ret = virNodeDeviceFindFCParentHost(parent, parent_host);
+    ret = virNodeDeviceFindFCParentHost(parent);
 
     virNodeDeviceObjUnlock(parent);
 
@@ -2075,9 +2069,8 @@ virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs,
 }
 
 
-int
-virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs,
-                                 int *parent_host)
+static int
+virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs)
 {
     virNodeDeviceObjPtr parent = NULL;
     const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS);
@@ -2089,7 +2082,7 @@ virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs,
         return -1;
     }
 
-    ret = virNodeDeviceFindFCParentHost(parent, parent_host);
+    ret = virNodeDeviceFindFCParentHost(parent);
 
     virNodeDeviceObjUnlock(parent);
 
@@ -2097,6 +2090,33 @@ virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs,
 }
 
 
+int
+virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs,
+                           virNodeDeviceDefPtr def,
+                           int create)
+{
+    int parent_host = -1;
+
+    if (def->parent) {
+        parent_host = virNodeDeviceGetParentHostByParent(devs, def->name,
+                                                         def->parent);
+    } else if (def->parent_wwnn && def->parent_wwpn) {
+        parent_host = virNodeDeviceGetParentHostByWWNs(devs, def->name,
+                                                       def->parent_wwnn,
+                                                       def->parent_wwpn);
+    } else if (def->parent_fabric_wwn) {
+        parent_host =
+            virNodeDeviceGetParentHostByFabricWWN(devs, def->name,
+                                                  def->parent_fabric_wwn);
+    } else if (create == CREATE_DEVICE) {
+        /* Try to find a vport capable scsi_host when no parent supplied */
+        parent_host = virNodeDeviceFindVportParentHost(devs);
+    }
+
+    return parent_host;
+}
+
+
 void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
 {
     size_t i = 0;

src/conf/node_device_conf.h

@@ -299,23 +299,8 @@ int virNodeDeviceGetWWNs(virNodeDeviceDefPtr def,
                          char **wwpn);
 
 int virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs,
-                               const char *dev_name,
-                               const char *parent_name,
-                               int *parent_host);
-
-int virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs,
-                                     const char *dev_name,
-                                     const char *parent_wwnn,
-                                     const char *parent_wwpn,
-                                     int *parent_host);
-
-int virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs,
-                                          const char *dev_name,
-                                          const char *parent_fabric_wwn,
-                                          int *parent_host);
-
-int virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs,
-                                     int *parent_host);
+                               virNodeDeviceDefPtr def,
+                               int create);
 
 void virNodeDeviceDefFree(virNodeDeviceDefPtr def);
 

src/libvirt_private.syms

@@ -702,10 +702,7 @@ virNodeDeviceDefParseNode;
 virNodeDeviceDefParseString;
 virNodeDeviceFindByName;
 virNodeDeviceFindBySysfsPath;
-virNodeDeviceFindVportParentHost;
 virNodeDeviceGetParentHost;
-virNodeDeviceGetParentHostByFabricWWN;
-virNodeDeviceGetParentHostByWWNs;
 virNodeDeviceGetParentName;
 virNodeDeviceGetWWNs;
 virNodeDeviceHasCap;

src/node_device/node_device_driver.c

@@ -581,8 +581,7 @@ nodeDeviceCreateXML(virConnectPtr conn,
 
     nodeDeviceLock();
 
-    def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type);
-    if (def == NULL)
+    if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
         goto cleanup;
 
     if (virNodeDeviceCreateXMLEnsureACL(conn, def) < 0)
@@ -591,30 +590,9 @@ nodeDeviceCreateXML(virConnectPtr conn,
     if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
         goto cleanup;
 
-    if (def->parent) {
-        if (virNodeDeviceGetParentHost(&driver->devs,
-                                       def->name,
-                                       def->parent,
-                                       &parent_host) < 0)
-            goto cleanup;
-    } else if (def->parent_wwnn && def->parent_wwpn) {
-        if (virNodeDeviceGetParentHostByWWNs(&driver->devs,
-                                             def->name,
-                                             def->parent_wwnn,
-                                             def->parent_wwpn,
-                                             &parent_host) < 0)
-            goto cleanup;
-    } else if (def->parent_fabric_wwn) {
-        if (virNodeDeviceGetParentHostByFabricWWN(&driver->devs,
-                                                  def->name,
-                                                  def->parent_fabric_wwn,
-                                                  &parent_host) < 0)
-            goto cleanup;
-    } else {
-        /* Try to find a vport capable scsi_host when no parent supplied */
-        if (virNodeDeviceFindVportParentHost(&driver->devs, &parent_host) < 0)
-            goto cleanup;
-    }
+    if ((parent_host = virNodeDeviceGetParentHost(&driver->devs, def,
+                                                  CREATE_DEVICE)) < 0)
+        goto cleanup;
 
     if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_CREATE) < 0)
         goto cleanup;
@@ -642,7 +620,8 @@ nodeDeviceDestroy(virNodeDevicePtr dev)
 {
     int ret = -1;
     virNodeDeviceObjPtr obj = NULL;
-    char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL;
+    virNodeDeviceDefPtr def;
+    char *wwnn = NULL, *wwpn = NULL;
     int parent_host = -1;
 
     nodeDeviceLock();
@@ -659,32 +638,26 @@ nodeDeviceDestroy(virNodeDevicePtr dev)
     if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) < 0)
         goto cleanup;
 
-
-    /* virNodeDeviceGetParentHost will cause the device object's lock to be
-     * taken, so we have to dup the parent's name and drop the lock
-     * before calling it.  We don't need the reference to the object
-     * any more once we have the parent's name.  */
-    if (VIR_STRDUP(parent_name, obj->def->parent) < 0) {
-        virNodeDeviceObjUnlock(obj);
-        obj = NULL;
-        goto cleanup;
-    }
+    /* virNodeDeviceGetParentHost will cause the device object's lock
+     * to be taken, so grab the object def which will have the various
+     * fields used to search (name, parent, parent_wwnn, parent_wwpn,
+     * or parent_fabric_wwn) and drop the object lock. */
+    def = obj->def;
     virNodeDeviceObjUnlock(obj);
     obj = NULL;
-
-    if (virNodeDeviceGetParentHost(&driver->devs, dev->name, parent_name,
-                                   &parent_host) < 0)
+    if ((parent_host = virNodeDeviceGetParentHost(&driver->devs, def,
+                                                  EXISTING_DEVICE)) < 0)
         goto cleanup;
 
     if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0)
         goto cleanup;
 
     ret = 0;
+
  cleanup:
     nodeDeviceUnlock();
     if (obj)
         virNodeDeviceObjUnlock(obj);
-    VIR_FREE(parent_name);
     VIR_FREE(wwnn);
     VIR_FREE(wwpn);
     return ret;

src/test/test_driver.c

@@ -5712,7 +5712,6 @@ testNodeDeviceCreateXML(virConnectPtr conn,
     testDriverPtr driver = conn->privateData;
     virNodeDeviceDefPtr def = NULL;
     char *wwnn = NULL, *wwpn = NULL;
-    int parent_host = -1;
     virNodeDevicePtr dev = NULL;
     virNodeDeviceDefPtr newdef = NULL;
 
@@ -5723,15 +5722,16 @@ testNodeDeviceCreateXML(virConnectPtr conn,
     if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, NULL)))
         goto cleanup;
 
-    /* We run these next two simply for validation - they are essentially
-     * 'validating' that the input XML either has a wwnn/wwpn or the
-     * virNodeDevCapSCSIHostParseXML generated a wwnn/wwpn and that the
-     * input XML has a parent host defined. */
+    /* We run this simply for validation - it essentially validates that
+     * the input XML either has a wwnn/wwpn or virNodeDevCapSCSIHostParseXML
+     * generated a wwnn/wwpn */
     if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0)
         goto cleanup;
 
-    if (virNodeDeviceGetParentHost(&driver->devs, def->name,
-                                   def->parent, &parent_host) < 0)
+    /* Unlike the "real" code we don't need the parent_host in order to
+     * call virVHBAManageVport, but still let's make sure the code finds
+     * something valid and no one messed up the mock environment. */
+    if (virNodeDeviceGetParentHost(&driver->devs, def, CREATE_DEVICE) < 0)
         goto cleanup;
 
     /* In the real code, we'd call virVHBAManageVport followed by
@@ -5762,7 +5762,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     testDriverPtr driver = dev->conn->privateData;
     virNodeDeviceObjPtr obj = NULL;
     char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL;
-    int parent_host = -1;
     virObjectEventPtr event = NULL;
 
     testDriverLock(driver);
@@ -5788,11 +5787,10 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
      * any more once we have the parent's name.  */
     virNodeDeviceObjUnlock(obj);
 
-    /* We do this just for basic validation */
-    if (virNodeDeviceGetParentHost(&driver->devs,
-                                   dev->name,
-                                   parent_name,
-                                   &parent_host) == -1) {
+    /* We do this just for basic validation, but also avoid finding a
+     * vport capable HBA if for some reason our vHBA doesn't exist */
+    if (virNodeDeviceGetParentHost(&driver->devs, obj->def,
+                                   EXISTING_DEVICE) < 0) {
         obj = NULL;
         goto out;
     }