[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] storage: Move and rename getVhbaSCSIHostParent



On 18.11.2014 22:26, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1159180

Move the API from the backend to storage_conf and rename it to
virStoragePoolGetVhbaSCSIHostParent.  A future patch will need to
use this functionality from storage_conf

Signed-off-by: John Ferlan <jferlan redhat com>
---
  src/conf/storage_conf.c            | 66 ++++++++++++++++++++++++++++++++++++++
  src/conf/storage_conf.h            |  5 +++
  src/libvirt_private.syms           |  1 +
  src/storage/storage_backend_scsi.c | 66 ++------------------------------------
  4 files changed, 74 insertions(+), 64 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6ad37f8..751c0c0 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2078,6 +2078,72 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
      return ret;
  }

+/*
+ * virStoragePoolGetVhbaSCSIHostParent:
+ *
+ * Using the Node Device Driver, find the host# name found via wwnn/wwpn
+ * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get
+ * the parent 'scsi_host#'.
+ *
+ * @conn: Connection pointer (must be non-NULL on entry)
+ * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#")
+ *
+ * Returns a "scsi_host#" string of the parent of the vHBA
+ */
+char *
+virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn,
+                                    const char *name)
+{
+    char *nodedev_name = NULL;
+    virNodeDevicePtr device = NULL;
+    char *xml = NULL;
+    virNodeDeviceDefPtr def = NULL;
+    char *vhba_parent = NULL;
+    virErrorPtr savedError = NULL;
+
+    VIR_DEBUG("conn=%p, name=%s", conn, name);
+
+    /* We get passed "host#" from the return from virGetFCHostNameByWWN,
+     * so we need to adjust that to what the nodedev lookup expects
+     */
+    if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0)
+        goto cleanup;
+
+    /* Compare the scsi_host for the name with the provided parent
+     * if not the same, then fail
+     */
+    if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Cannot find '%s' in node device database"),
+                       nodedev_name);
+        goto cleanup;
+    }
+
+    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
+        goto cleanup;
+
+    if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
+        goto cleanup;
+
+    /* The caller checks whether the returned value is NULL or not
+     * before continuing
+     */
+    ignore_value(VIR_STRDUP(vhba_parent, def->parent));
+
+ cleanup:
+    if (!vhba_parent)
+        savedError = virSaveLastError();
+    VIR_FREE(nodedev_name);
+    virNodeDeviceDefFree(def);
+    VIR_FREE(xml);
+    virNodeDeviceFree(device);
+    if (savedError) {
+        virSetError(savedError);
+        virFreeError(savedError);
+    }
+    return vhba_parent;


While you're at this, would it make sense to:

1) s/virNodeDeviceFree/virObjectUnref/
2) drop savedError?

Firstly, virNodeDeviceFree() must be guarded with check for device != NULL (yep, our public API madness where free(NULL) is not accepted). Moreover, since it is a public API it resets the error object, which you don't want to and hence you save the error. But using the very same function that virNodeDeviceFree() is using, you will achieve the same goal and without any temporary variables and cleaner code.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]