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.

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index bc976d0e4719a50a4557d1ee8d230a6b5c31b7eb..31ca45646ca06b7ad9de8f28b12cb41e933bf4f7 100644 (file)
--- a/src/conf/node_device_conf.c
+++ b/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;

diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 6c43546926ed82bd43d51185207dd6f2751d0562..8213c27a1d7f023e011353fdfae3ae51ad7ec6e7 100644 (file)
--- a/src/conf/node_device_conf.h
+++ b/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);
 

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c90093a710347d4c233d7193d3f8cf98fd965504..e6ccd697d2074a3baf9594ce848c6fd3527b1959 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -702,10 +702,7 @@ virNodeDeviceDefParseNode;
 virNodeDeviceDefParseString;
 virNodeDeviceFindByName;
 virNodeDeviceFindBySysfsPath;
-virNodeDeviceFindVportParentHost;
 virNodeDeviceGetParentHost;
-virNodeDeviceGetParentHostByFabricWWN;
-virNodeDeviceGetParentHostByWWNs;
 virNodeDeviceGetParentName;
 virNodeDeviceGetWWNs;
 virNodeDeviceHasCap;

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 32022ebca90c0f4d577815e31dc590c99ebf98e5..5d57006f2cfcccc736f0f950b2f16eb5da814c31 100644 (file)
--- a/src/node_device/node_device_driver.c
+++ b/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;

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index acc4d340d9451dec6b4150a6e7454e1ea16a54df..5fef3f10b92848ece4b5b00638251399c32d6445 100644 (file)
--- a/src/test/test_driver.c
+++ b/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;
     }