Move the bulk of the code to the node_device_conf and rename to
virNodeDevice{Create|Delete}Vport.
Alter the create algorithm slightly in order to return the name of
the scsi_host vHBA that was created. The existing code would fetch
the name and if it exists would start a thread looking for the LUNs;
however, in no way did it validate the device was created for the
storage pool even though it would be used thereafter.
This slight change allows the code that checks if the node device
by wwnn/wwpn already exists and return that name. This 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 startVport code can now just start the
thread once it gets a non NULL name back.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
src/conf/node_device_conf.h | 9 ++
src/libvirt_private.syms | 2 +
src/storage/storage_backend_scsi.c | 192 +++------------------------------
4 files changed, 235 insertions(+), 179 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 3565aec..66cb78d 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1870,3 +1870,214 @@ virNodeDeviceGetParentName(virConnectPtr conn,
return parent;
}
+
+
+/*
+ * Using the host# name found via wwnn/wwpn lookup in the fc_host
+ * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
+ */
+static bool
+nodeDeviceCheckParent(virConnectPtr conn,
+ const char *name,
+ const char *parent_name)
+{
+ char *scsi_host_name = NULL;
+ char *vhba_parent = NULL;
+ bool retval = false;
+
+ VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
+
+ /* autostarted pool - assume we're OK */
+ if (!conn)
+ return true;
+
+ if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ goto cleanup;
+
+ if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+ goto cleanup;
+
+ if (STRNEQ(parent_name, vhba_parent)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Parent attribute '%s' does not match parent '%s' "
+ "determined for the '%s' wwnn/wwpn lookup."),
+ parent_name, vhba_parent, name);
+ goto cleanup;
+ }
+
+ retval = true;
+
+ cleanup:
+ VIR_FREE(vhba_parent);
+ VIR_FREE(scsi_host_name);
+ return retval;
+}
+
+
+/**
+ * @conn: Connection pointer
+ * @fchost: Pointer to vHBA adapter
+ *
+ * Create a vHBA for Storage. This code accomplishes this via searching
+ * through the sysfs for scsi_host/fc_host in order to first ensure some
+ * vHBA doesn't already exist for the requested wwnn/wwpn (e.g. an unmanaged
+ * vHBA) and to search for the parent vport capable scsi_host by name,
+ * wwnn/wwpn, or fabric_wwn (if provided). If no parent is provided, then
+ * a vport capable scsi_host will be selected.
+ *
+ * Returns vHBA name on success, NULL on failure with an error message set
+ */
+char *
+virNodeDeviceCreateVport(virConnectPtr conn,
+ virStorageAdapterFCHostPtr fchost)
+{
+ unsigned int parent_host;
+ char *name = NULL;
+ char *parent_hoststr = NULL;
+ bool skip_capable_check = false;
+
+ VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'",
+ conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
+
+ /* If we find an existing HBA/vHBA within the fc_host sysfs
+ * using the wwnn/wwpn, then a nodedev is already created for
+ * this pool and we don't have to create the vHBA
+ */
+ if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+ /* 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 && nodeDeviceCheckParent(conn, name, fchost->parent))
+ VIR_FREE(name);
+
+ return name;
+ }
+
+ if (fchost->parent) {
+ if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
+ goto cleanup;
+ } else if (fchost->parent_wwnn && fchost->parent_wwpn) {
+ if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn,
+ fchost->parent_wwpn))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("cannot find parent using provided wwnn/wwpn"));
+ goto cleanup;
+ }
+ } else if (fchost->parent_fabric_wwn) {
+ if (!(parent_hoststr =
+ virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("cannot find parent using provided fabric_wwn"));
+ goto cleanup;
+ }
+ } else {
+ if (!(parent_hoststr = virVHBAFindVportHost(NULL))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'parent' for vHBA not specified, and "
+ "cannot find one on this host"));
+ goto cleanup;
+ }
+ skip_capable_check = true;
+ }
+
+ if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0)
+ goto cleanup;
+
+ /* NOTE:
+ * We do not save the parent_hoststr in fchost->parent since
+ * we could be writing out the 'def' to the saved XML config.
+ * If we wrote out the name in the XML, then future starts would
+ * always use the same parent rather than finding the "best available"
+ * parent. Besides we have a way to determine the parent based on
+ * the 'name' field.
+ */
+ if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' specified for vHBA does not exist"),
+ parent_hoststr);
+ goto cleanup;
+ }
+
+ if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
+ VPORT_CREATE) < 0)
+ goto cleanup;
+
+ /* 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(parent_hoststr);
+ return name;
+}
+
+
+/**
+ * @conn: Connection pointer
+ * @fchost: Pointer to vHBA adapter
+ *
+ * As long as the vHBA is being managed, search for the scsi_host via the
+ * provided wwnn/wwpn and then find the corresponding parent scsi_host in
+ * order to send the delete request.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virNodeDeviceDeleteVport(virConnectPtr conn,
+ virStorageAdapterFCHostPtr fchost)
+{
+ char *name = NULL;
+ char *scsi_host_name = NULL;
+ unsigned int parent_host;
+ char *vhba_parent = NULL;
+ int ret = -1;
+
+ VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
+ conn, NULLSTR(fchost->parent), fchost->managed,
+ fchost->wwnn, fchost->wwpn);
+
+ /* If we're not managing the deletion of the vHBA, then just return */
+ if (fchost->managed != VIR_TRISTATE_BOOL_YES)
+ return 0;
+
+ /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
+ if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"),
+ fchost->wwnn, fchost->wwpn);
+ goto cleanup;
+ }
+
+ /* If at startup time we provided a parent, then use that to
+ * get the parent_host value; otherwise, we have to determine
+ * the parent scsi_host which we did not save at startup time
+ */
+ if (fchost->parent) {
+ if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0)
+ goto cleanup;
+ } else {
+ if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ goto cleanup;
+
+ if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+ goto cleanup;
+
+ if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0)
+ goto cleanup;
+ }
+
+ if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
+ VPORT_DELETE) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(name);
+ VIR_FREE(vhba_parent);
+ VIR_FREE(scsi_host_name);
+ return ret;
+}
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index cf51c69..15e2eac 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -28,8 +28,11 @@
# include "internal.h"
# include "virbitmap.h"
# include "virutil.h"
+# include "virscsihost.h"
# include "virpci.h"
+# include "virvhba.h"
# include "device_conf.h"
+# include "storage_adapter_conf.h"
# include <libxml/tree.h>
@@ -354,4 +357,10 @@ char *
virNodeDeviceGetParentName(virConnectPtr conn,
const char *nodedev_name);
+char *virNodeDeviceCreateVport(virConnectPtr conn,
+ virStorageAdapterFCHostPtr fchost);
+
+int virNodeDeviceDeleteVport(virConnectPtr conn,
+ virStorageAdapterFCHostPtr fchost);
+
#endif /* __VIR_NODE_DEVICE_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7bdcde7..cacc7aa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -686,11 +686,13 @@ virNetDevIPRouteParseXML;
virNodeDevCapsDefFree;
virNodeDevCapTypeFromString;
virNodeDevCapTypeToString;
+virNodeDeviceCreateVport;
virNodeDeviceDefFormat;
virNodeDeviceDefFree;
virNodeDeviceDefParseFile;
virNodeDeviceDefParseNode;
virNodeDeviceDefParseString;
+virNodeDeviceDeleteVport;
virNodeDeviceGetParentName;
virNodeDeviceGetWWNs;
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index abbd38d..666e473 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -33,9 +33,7 @@
#include "virlog.h"
#include "virfile.h"
#include "vircommand.h"
-#include "virscsihost.h"
#include "virstring.h"
-#include "virvhba.h"
#include "storage_util.h"
#include "node_device_conf.h"
@@ -214,57 +212,13 @@ getAdapterName(virStorageAdapterPtr adapter)
return name;
}
-/*
- * Using the host# name found via wwnn/wwpn lookup in the fc_host
- * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
- */
-static bool
-checkParent(virConnectPtr conn,
- const char *name,
- const char *parent_name)
-{
- char *scsi_host_name = NULL;
- char *vhba_parent = NULL;
- bool retval = false;
-
- VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
-
- /* autostarted pool - assume we're OK */
- if (!conn)
- return true;
-
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
- goto cleanup;
-
- if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
- goto cleanup;
-
- if (STRNEQ(parent_name, vhba_parent)) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Parent attribute '%s' does not match parent '%s' "
- "determined for the '%s' wwnn/wwpn lookup."),
- parent_name, vhba_parent, name);
- goto cleanup;
- }
-
- retval = true;
-
- cleanup:
- VIR_FREE(vhba_parent);
- VIR_FREE(scsi_host_name);
- return retval;
-}
-
static int
createVport(virConnectPtr conn,
virStoragePoolDefPtr def,
const char *configFile,
virStorageAdapterFCHostPtr fchost)
{
- unsigned int parent_host;
char *name = NULL;
- char *parent_hoststr = NULL;
- bool skip_capable_check = false;
virStoragePoolFCRefreshInfoPtr cbdata = NULL;
virThread thread;
int ret = -1;
@@ -273,66 +227,11 @@ createVport(virConnectPtr conn,
conn, NULLSTR(configFile), NULLSTR(fchost->parent),
fchost->wwnn, fchost->wwpn);
- /* If we find an existing HBA/vHBA within the fc_host sysfs
- * using the wwnn/wwpn, then a nodedev is already created for
- * this pool and we don't have to create the vHBA
- */
- if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
- /* If a parent was provided, let's make sure the 'name' we've
- * retrieved has the same parent
- */
- if (fchost->parent && checkParent(conn, name, fchost->parent))
- ret = 0;
+ if (!(name = virNodeDeviceCreateVport(conn, fchost)))
goto cleanup;
- }
- if (fchost->parent) {
- if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
- goto cleanup;
- } else if (fchost->parent_wwnn && fchost->parent_wwpn) {
- if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn,
- fchost->parent_wwpn))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("cannot find parent using provided wwnn/wwpn"));
- goto cleanup;
- }
- } else if (fchost->parent_fabric_wwn) {
- if (!(parent_hoststr =
- virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("cannot find parent using provided fabric_wwn"));
- goto cleanup;
- }
- } else {
- if (!(parent_hoststr = virVHBAFindVportHost(NULL))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'parent' for vHBA not specified, and "
- "cannot find one on this host"));
- goto cleanup;
- }
- skip_capable_check = true;
- }
-
- if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0)
- goto cleanup;
-
- /* NOTE:
- * We do not save the parent_hoststr in fchost->parent since
- * we could be writing out the 'def' to the saved XML config.
- * If we wrote out the name in the XML, then future starts would
- * always use the same parent rather than finding the "best available"
- * parent. Besides we have a way to determine the parent based on
- * the 'name' field.
- */
- if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) {
- virReportError(VIR_ERR_XML_ERROR,
- _("parent '%s' specified for vHBA does not exist"),
- parent_hoststr);
- goto cleanup;
- }
-
- /* Since we're creating the vHBA, then we need to manage removing it
+ /* Since we've created the vHBA, then we need to manage removing it
* as well. Since we need this setting to "live" through a libvirtd
* restart, we need to save the persistent configuration. So if not
* already defined as YES, then force the issue.
@@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
}
}
- if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
- VPORT_CREATE) < 0)
- 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);
}
}
@@ -374,64 +265,6 @@ createVport(virConnectPtr conn,
cleanup:
VIR_FREE(name);
- VIR_FREE(parent_hoststr);
- return ret;
-}
-
-
-static int
-deleteVport(virConnectPtr conn,
- virStorageAdapterFCHostPtr fchost)
-{
- unsigned int parent_host;
- char *name = NULL;
- char *scsi_host_name = NULL;
- char *vhba_parent = NULL;
- int ret = -1;
-
- VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
- conn, NULLSTR(fchost->parent), fchost->managed,
- fchost->wwnn, fchost->wwpn);
-
- /* If we're not managing the deletion of the vHBA, then just return */
- if (fchost->managed != VIR_TRISTATE_BOOL_YES)
- return 0;
-
- /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
- if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"),
- fchost->wwnn, fchost->wwpn);
- goto cleanup;
- }
-
- /* If at startup time we provided a parent, then use that to
- * get the parent_host value; otherwise, we have to determine
- * the parent scsi_host which we did not save at startup time
- */
- if (fchost->parent) {
- if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0)
- goto cleanup;
- } else {
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
- goto cleanup;
-
- if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
- goto cleanup;
-
- if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0)
- goto cleanup;
- }
-
- if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
- VPORT_DELETE) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- VIR_FREE(name);
- VIR_FREE(vhba_parent);
- VIR_FREE(scsi_host_name);
return ret;
}
@@ -525,7 +358,8 @@ virStorageBackendSCSIStopPool(virConnectPtr conn,
virStoragePoolObjPtr pool)
{
if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST)
- return deleteVport(conn, &pool->def->source.adapter.data.fchost);
+ return virNodeDeviceDeleteVport(conn,
+ &pool->def->source.adapter.data.fchost);
return 0;
}
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/10/2017 04:10 PM, John Ferlan wrote:
> Move the bulk of the code to the node_device_conf and rename to
> virNodeDevice{Create|Delete}Vport.
>
> Alter the create algorithm slightly in order to return the name of
> the scsi_host vHBA that was created. The existing code would fetch
> the name and if it exists would start a thread looking for the LUNs;
> however, in no way did it validate the device was created for the
> storage pool even though it would be used thereafter.
>
> This slight change allows the code that checks if the node device
> by wwnn/wwpn already exists and return that name. This 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 startVport code can now just start the
> thread once it gets a non NULL name back.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
> src/conf/node_device_conf.h | 9 ++
> src/libvirt_private.syms | 2 +
> src/storage/storage_backend_scsi.c | 192 +++------------------------------
> 4 files changed, 235 insertions(+), 179 deletions(-)
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 3565aec..66cb78d 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -1870,3 +1870,214 @@ virNodeDeviceGetParentName(virConnectPtr conn,
>
> return parent;
> }
> +
> +
> +/*
> + * Using the host# name found via wwnn/wwpn lookup in the fc_host
> + * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
> + */
> +static bool
> +nodeDeviceCheckParent(virConnectPtr conn,
> + const char *name,
> + const char *parent_name)
> +{
> + char *scsi_host_name = NULL;
> + char *vhba_parent = NULL;
> + bool retval = false;
> +
> + VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
> +
> + /* autostarted pool - assume we're OK */
> + if (!conn)
> + return true;
> +
> + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + goto cleanup;
> +
> + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> + goto cleanup;
> +
> + if (STRNEQ(parent_name, vhba_parent)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Parent attribute '%s' does not match parent '%s' "
> + "determined for the '%s' wwnn/wwpn lookup."),
> + parent_name, vhba_parent, name);
> + goto cleanup;
> + }
> +
> + retval = true;
> +
> + cleanup:
> + VIR_FREE(vhba_parent);
> + VIR_FREE(scsi_host_name);
> + return retval;
> +}
> +
> +
> +/**
> + * @conn: Connection pointer
> + * @fchost: Pointer to vHBA adapter
> + *
> + * Create a vHBA for Storage. This code accomplishes this via searching
> + * through the sysfs for scsi_host/fc_host in order to first ensure some
> + * vHBA doesn't already exist for the requested wwnn/wwpn (e.g. an unmanaged
> + * vHBA) and to search for the parent vport capable scsi_host by name,
> + * wwnn/wwpn, or fabric_wwn (if provided). If no parent is provided, then
> + * a vport capable scsi_host will be selected.
> + *
> + * Returns vHBA name on success, NULL on failure with an error message set
> + */
> +char *
> +virNodeDeviceCreateVport(virConnectPtr conn,
> + virStorageAdapterFCHostPtr fchost)
> +{
> + unsigned int parent_host;
> + char *name = NULL;
> + char *parent_hoststr = NULL;
> + bool skip_capable_check = false;
> +
> + VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'",
> + conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
> +
> + /* If we find an existing HBA/vHBA within the fc_host sysfs
> + * using the wwnn/wwpn, then a nodedev is already created for
> + * this pool and we don't have to create the vHBA
> + */
> + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> + /* 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 && nodeDeviceCheckParent(conn, name, fchost->parent))
> + VIR_FREE(name);
> +
> + return name;
> + }
> +
> + if (fchost->parent) {
> + if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
> + goto cleanup;
> + } else if (fchost->parent_wwnn && fchost->parent_wwpn) {
> + if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn,
> + fchost->parent_wwpn))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("cannot find parent using provided wwnn/wwpn"));
> + goto cleanup;
> + }
> + } else if (fchost->parent_fabric_wwn) {
> + if (!(parent_hoststr =
> + virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("cannot find parent using provided fabric_wwn"));
> + goto cleanup;
> + }
> + } else {
> + if (!(parent_hoststr = virVHBAFindVportHost(NULL))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("'parent' for vHBA not specified, and "
> + "cannot find one on this host"));
> + goto cleanup;
> + }
> + skip_capable_check = true;
> + }
> +
> + if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0)
> + goto cleanup;
> +
> + /* NOTE:
> + * We do not save the parent_hoststr in fchost->parent since
> + * we could be writing out the 'def' to the saved XML config.
> + * If we wrote out the name in the XML, then future starts would
> + * always use the same parent rather than finding the "best available"
> + * parent. Besides we have a way to determine the parent based on
> + * the 'name' field.
> + */
> + if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("parent '%s' specified for vHBA does not exist"),
> + parent_hoststr);
> + goto cleanup;
> + }
> +
> + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
> + VPORT_CREATE) < 0)
> + goto cleanup;
> +
> + /* 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(parent_hoststr);
> + return name;
> +}
> +
> +
> +/**
> + * @conn: Connection pointer
> + * @fchost: Pointer to vHBA adapter
> + *
> + * As long as the vHBA is being managed, search for the scsi_host via the
> + * provided wwnn/wwpn and then find the corresponding parent scsi_host in
> + * order to send the delete request.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virNodeDeviceDeleteVport(virConnectPtr conn,
> + virStorageAdapterFCHostPtr fchost)
> +{
> + char *name = NULL;
> + char *scsi_host_name = NULL;
> + unsigned int parent_host;
> + char *vhba_parent = NULL;
> + int ret = -1;
> +
> + VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
> + conn, NULLSTR(fchost->parent), fchost->managed,
> + fchost->wwnn, fchost->wwpn);
> +
> + /* If we're not managing the deletion of the vHBA, then just return */
> + if (fchost->managed != VIR_TRISTATE_BOOL_YES)
> + return 0;
> +
> + /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
> + if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"),
> + fchost->wwnn, fchost->wwpn);
> + goto cleanup;
> + }
> +
> + /* If at startup time we provided a parent, then use that to
> + * get the parent_host value; otherwise, we have to determine
> + * the parent scsi_host which we did not save at startup time
> + */
> + if (fchost->parent) {
> + if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0)
> + goto cleanup;
> + } else {
> + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + goto cleanup;
> +
> + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> + goto cleanup;
> +
> + if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0)
> + goto cleanup;
> + }
> +
> + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
> + VPORT_DELETE) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(name);
> + VIR_FREE(vhba_parent);
> + VIR_FREE(scsi_host_name);
> + return ret;
> +}
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index cf51c69..15e2eac 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -28,8 +28,11 @@
> # include "internal.h"
> # include "virbitmap.h"
> # include "virutil.h"
> +# include "virscsihost.h"
> # include "virpci.h"
> +# include "virvhba.h"
> # include "device_conf.h"
> +# include "storage_adapter_conf.h"
>
> # include <libxml/tree.h>
>
> @@ -354,4 +357,10 @@ char *
> virNodeDeviceGetParentName(virConnectPtr conn,
> const char *nodedev_name);
>
> +char *virNodeDeviceCreateVport(virConnectPtr conn,
> + virStorageAdapterFCHostPtr fchost);
> +
> +int virNodeDeviceDeleteVport(virConnectPtr conn,
> + virStorageAdapterFCHostPtr fchost);
> +
> #endif /* __VIR_NODE_DEVICE_CONF_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7bdcde7..cacc7aa 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -686,11 +686,13 @@ virNetDevIPRouteParseXML;
> virNodeDevCapsDefFree;
> virNodeDevCapTypeFromString;
> virNodeDevCapTypeToString;
> +virNodeDeviceCreateVport;
> virNodeDeviceDefFormat;
> virNodeDeviceDefFree;
> virNodeDeviceDefParseFile;
> virNodeDeviceDefParseNode;
> virNodeDeviceDefParseString;
> +virNodeDeviceDeleteVport;
> virNodeDeviceGetParentName;
> virNodeDeviceGetWWNs;
>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index abbd38d..666e473 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -33,9 +33,7 @@
> #include "virlog.h"
> #include "virfile.h"
> #include "vircommand.h"
> -#include "virscsihost.h"
> #include "virstring.h"
> -#include "virvhba.h"
> #include "storage_util.h"
> #include "node_device_conf.h"
>
> @@ -214,57 +212,13 @@ getAdapterName(virStorageAdapterPtr adapter)
> return name;
> }
>
> -/*
> - * Using the host# name found via wwnn/wwpn lookup in the fc_host
> - * sysfs tree to get the parent 'scsi_host#' to ensure it matches.
> - */
> -static bool
> -checkParent(virConnectPtr conn,
> - const char *name,
> - const char *parent_name)
> -{
> - char *scsi_host_name = NULL;
> - char *vhba_parent = NULL;
> - bool retval = false;
> -
> - VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
> -
> - /* autostarted pool - assume we're OK */
> - if (!conn)
> - return true;
> -
> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> - goto cleanup;
> -
> - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> - goto cleanup;
> -
> - if (STRNEQ(parent_name, vhba_parent)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Parent attribute '%s' does not match parent '%s' "
> - "determined for the '%s' wwnn/wwpn lookup."),
> - parent_name, vhba_parent, name);
> - goto cleanup;
> - }
> -
> - retval = true;
> -
> - cleanup:
> - VIR_FREE(vhba_parent);
> - VIR_FREE(scsi_host_name);
> - return retval;
> -}
> -
> static int
> createVport(virConnectPtr conn,
> virStoragePoolDefPtr def,
> const char *configFile,
> virStorageAdapterFCHostPtr fchost)
> {
> - unsigned int parent_host;
> char *name = NULL;
> - char *parent_hoststr = NULL;
> - bool skip_capable_check = false;
> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
> virThread thread;
> int ret = -1;
> @@ -273,66 +227,11 @@ createVport(virConnectPtr conn,
> conn, NULLSTR(configFile), NULLSTR(fchost->parent),
> fchost->wwnn, fchost->wwpn);
>
> - /* If we find an existing HBA/vHBA within the fc_host sysfs
> - * using the wwnn/wwpn, then a nodedev is already created for
> - * this pool and we don't have to create the vHBA
> - */
> - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> - /* If a parent was provided, let's make sure the 'name' we've
> - * retrieved has the same parent
> - */
> - if (fchost->parent && checkParent(conn, name, fchost->parent))
> - ret = 0;
>
> + if (!(name = virNodeDeviceCreateVport(conn, fchost)))
> goto cleanup;
> - }
>
> - if (fchost->parent) {
> - if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
> - goto cleanup;
> - } else if (fchost->parent_wwnn && fchost->parent_wwpn) {
> - if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn,
> - fchost->parent_wwpn))) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("cannot find parent using provided wwnn/wwpn"));
> - goto cleanup;
> - }
> - } else if (fchost->parent_fabric_wwn) {
> - if (!(parent_hoststr =
> - virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("cannot find parent using provided fabric_wwn"));
> - goto cleanup;
> - }
> - } else {
> - if (!(parent_hoststr = virVHBAFindVportHost(NULL))) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("'parent' for vHBA not specified, and "
> - "cannot find one on this host"));
> - goto cleanup;
> - }
> - skip_capable_check = true;
> - }
> -
> - if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0)
> - goto cleanup;
> -
> - /* NOTE:
> - * We do not save the parent_hoststr in fchost->parent since
> - * we could be writing out the 'def' to the saved XML config.
> - * If we wrote out the name in the XML, then future starts would
> - * always use the same parent rather than finding the "best available"
> - * parent. Besides we have a way to determine the parent based on
> - * the 'name' field.
> - */
> - if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("parent '%s' specified for vHBA does not exist"),
> - parent_hoststr);
> - goto cleanup;
> - }
> -
> - /* Since we're creating the vHBA, then we need to manage removing it
> + /* Since we've created the vHBA, then we need to manage removing it
> * as well. Since we need this setting to "live" through a libvirtd
> * restart, we need to save the persistent configuration. So if not
> * already defined as YES, then force the issue.
> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
Right in this space the old code would set managed = YES and call virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever *that* does) and a few other things from post-saveconfig that have been moed into virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering won't cause a problem...
> }
> }
>
> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
> - VPORT_CREATE) < 0)
> - 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);
> - }
Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would have resulted in an early return, it's okay to move all of this outside the conditional (which no longer exists!)
Everything looks like a simple hatchet and stitch job to me - ACK.
> + 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);
> }
> }
>
> @@ -374,64 +265,6 @@ createVport(virConnectPtr conn,
>
> cleanup:
> VIR_FREE(name);
> - VIR_FREE(parent_hoststr);
> - return ret;
> -}
> -
> -
> -static int
> -deleteVport(virConnectPtr conn,
> - virStorageAdapterFCHostPtr fchost)
> -{
> - unsigned int parent_host;
> - char *name = NULL;
> - char *scsi_host_name = NULL;
> - char *vhba_parent = NULL;
> - int ret = -1;
> -
> - VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
> - conn, NULLSTR(fchost->parent), fchost->managed,
> - fchost->wwnn, fchost->wwpn);
> -
> - /* If we're not managing the deletion of the vHBA, then just return */
> - if (fchost->managed != VIR_TRISTATE_BOOL_YES)
> - return 0;
> -
> - /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
> - if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"),
> - fchost->wwnn, fchost->wwpn);
> - goto cleanup;
> - }
> -
> - /* If at startup time we provided a parent, then use that to
> - * get the parent_host value; otherwise, we have to determine
> - * the parent scsi_host which we did not save at startup time
> - */
> - if (fchost->parent) {
> - if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0)
> - goto cleanup;
> - } else {
> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> - goto cleanup;
> -
> - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> - goto cleanup;
> -
> - if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0)
> - goto cleanup;
> - }
> -
> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
> - VPORT_DELETE) < 0)
> - goto cleanup;
> -
> - ret = 0;
> - cleanup:
> - VIR_FREE(name);
> - VIR_FREE(vhba_parent);
> - VIR_FREE(scsi_host_name);
> return ret;
> }
>
> @@ -525,7 +358,8 @@ virStorageBackendSCSIStopPool(virConnectPtr conn,
> virStoragePoolObjPtr pool)
> {
> if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST)
> - return deleteVport(conn, &pool->def->source.adapter.data.fchost);
> + return virNodeDeviceDeleteVport(conn,
> + &pool->def->source.adapter.data.fchost);
>
> return 0;
> }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/12/2017 06:35 PM, Laine Stump wrote:
> On 03/10/2017 04:10 PM, John Ferlan wrote:
>> Move the bulk of the code to the node_device_conf and rename to
>> virNodeDevice{Create|Delete}Vport.
>>
>> Alter the create algorithm slightly in order to return the name of
>> the scsi_host vHBA that was created. The existing code would fetch
>> the name and if it exists would start a thread looking for the LUNs;
>> however, in no way did it validate the device was created for the
>> storage pool even though it would be used thereafter.
>>
>> This slight change allows the code that checks if the node device
>> by wwnn/wwpn already exists and return that name. This 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 startVport code can now just start the
>> thread once it gets a non NULL name back.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
>> src/conf/node_device_conf.h | 9 ++
>> src/libvirt_private.syms | 2 +
>> src/storage/storage_backend_scsi.c | 192 +++------------------------------
>> 4 files changed, 235 insertions(+), 179 deletions(-)
>>
[...]
>> -
>> - /* Since we're creating the vHBA, then we need to manage removing it
>> + /* Since we've created the vHBA, then we need to manage removing it
>> * as well. Since we need this setting to "live" through a libvirtd
>> * restart, we need to save the persistent configuration. So if not
>> * already defined as YES, then force the issue.
>> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
>
>
> Right in this space the old code would set managed = YES and call virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever *that* does) and a few other things from post-saveconfig that have been moed into virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering won't cause a problem...
>
Right it was intentional and (famous last words) I don't think there's
going to be a problem in the change of ordering...
One of the things virNodeDeviceCreateVport now does is ensure that the
vHBA was created (whether via using virNodeDeviceCreateXML success or
after the virVHBAManageVport(CREATE) ensuring that the new scsi_hostN by
FCHost wwnn/wwpn can be found and if not, attempt a DELETE.
If you consider the "current" code - it would save the XML, then attempt
the CREATE, but not reset the config file if that failed. I suppose I
could have split across multiple patches too. Of course that's why my
commit message is extra wordy ;-)
>> }
>> }
>>
>> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
>> - VPORT_CREATE) < 0)
>> - 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);
>> - }
>
> Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would have resulted in an early return, it's okay to move all of this outside the conditional (which no longer exists!)
>
Right - although if desired I can split this patch up into (at least)
two to make it more obvious.
Tks -
John
>
> Everything looks like a simple hatchet and stitch job to me - ACK.
>
>
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/15/2017 10:08 AM, John Ferlan wrote:
>
>
> On 03/12/2017 06:35 PM, Laine Stump wrote:
>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>> Move the bulk of the code to the node_device_conf and rename to
>>> virNodeDevice{Create|Delete}Vport.
>>>
>>> Alter the create algorithm slightly in order to return the name of
>>> the scsi_host vHBA that was created. The existing code would fetch
>>> the name and if it exists would start a thread looking for the LUNs;
>>> however, in no way did it validate the device was created for the
>>> storage pool even though it would be used thereafter.
>>>
>>> This slight change allows the code that checks if the node device
>>> by wwnn/wwpn already exists and return that name. This 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 startVport code can now just start the
>>> thread once it gets a non NULL name back.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
>>> src/conf/node_device_conf.h | 9 ++
>>> src/libvirt_private.syms | 2 +
>>> src/storage/storage_backend_scsi.c | 192 +++------------------------------
>>> 4 files changed, 235 insertions(+), 179 deletions(-)
>>>
>
> [...]
>
>>> -
>>> - /* Since we're creating the vHBA, then we need to manage removing it
>>> + /* Since we've created the vHBA, then we need to manage removing it
>>> * as well. Since we need this setting to "live" through a libvirtd
>>> * restart, we need to save the persistent configuration. So if not
>>> * already defined as YES, then force the issue.
>>> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
>>
>>
>> Right in this space the old code would set managed = YES and call virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever *that* does) and a few other things from post-saveconfig that have been moed into virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering won't cause a problem...
>>
>
> Right it was intentional and (famous last words) I don't think there's
> going to be a problem in the change of ordering...
>
> One of the things virNodeDeviceCreateVport now does is ensure that the
> vHBA was created (whether via using virNodeDeviceCreateXML success or
> after the virVHBAManageVport(CREATE) ensuring that the new scsi_hostN by
> FCHost wwnn/wwpn can be found and if not, attempt a DELETE.
>
> If you consider the "current" code - it would save the XML, then attempt
> the CREATE, but not reset the config file if that failed. I suppose I
> could have split across multiple patches too. Of course that's why my
> commit message is extra wordy ;-)
>
>
>>> }
>>> }
>>>
>>> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
>>> - VPORT_CREATE) < 0)
>>> - 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);
>>> - }
>>
>> Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would have resulted in an early return, it's okay to move all of this outside the conditional (which no longer exists!)
>>
>
> Right - although if desired I can split this patch up into (at least)
> two to make it more obvious.
Nope. It was all decipherable as it is. Of course if you're looking to
get your patch count up, then...
>
> Tks -
>
> John
>
>>
>> Everything looks like a simple hatchet and stitch job to me - ACK.
>>
>>
>
> [...]
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/15/2017 11:19 AM, Laine Stump wrote:
> On 03/15/2017 10:08 AM, John Ferlan wrote:
>>
>>
>> On 03/12/2017 06:35 PM, Laine Stump wrote:
>>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>>> Move the bulk of the code to the node_device_conf and rename to
>>>> virNodeDevice{Create|Delete}Vport.
>>>>
>>>> Alter the create algorithm slightly in order to return the name of
>>>> the scsi_host vHBA that was created. The existing code would fetch
>>>> the name and if it exists would start a thread looking for the LUNs;
>>>> however, in no way did it validate the device was created for the
>>>> storage pool even though it would be used thereafter.
>>>>
>>>> This slight change allows the code that checks if the node device
>>>> by wwnn/wwpn already exists and return that name. This 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 startVport code can now just start the
>>>> thread once it gets a non NULL name back.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>>> ---
>>>> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
>>>> src/conf/node_device_conf.h | 9 ++
>>>> src/libvirt_private.syms | 2 +
>>>> src/storage/storage_backend_scsi.c | 192 +++------------------------------
>>>> 4 files changed, 235 insertions(+), 179 deletions(-)
>>>>
>>
>> [...]
>>
>>>> -
>>>> - /* Since we're creating the vHBA, then we need to manage removing it
>>>> + /* Since we've created the vHBA, then we need to manage removing it
>>>> * as well. Since we need this setting to "live" through a libvirtd
>>>> * restart, we need to save the persistent configuration. So if not
>>>> * already defined as YES, then force the issue.
>>>> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
>>>
>>>
>>> Right in this space the old code would set managed = YES and call virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever *that* does) and a few other things from post-saveconfig that have been moed into virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering won't cause a problem...
>>>
>>
>> Right it was intentional and (famous last words) I don't think there's
>> going to be a problem in the change of ordering...
>>
>> One of the things virNodeDeviceCreateVport now does is ensure that the
>> vHBA was created (whether via using virNodeDeviceCreateXML success or
>> after the virVHBAManageVport(CREATE) ensuring that the new scsi_hostN by
>> FCHost wwnn/wwpn can be found and if not, attempt a DELETE.
>>
>> If you consider the "current" code - it would save the XML, then attempt
>> the CREATE, but not reset the config file if that failed. I suppose I
>> could have split across multiple patches too. Of course that's why my
>> commit message is extra wordy ;-)
>>
>>
Upon further reflection - I've flip-flopped - I'll do the save first and
then the create. Not sure I can remember or even recreate why the order
is as it is, so let's not mess with (ahem) success...
>>>> }
>>>> }
>>>>
>>>> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
>>>> - VPORT_CREATE) < 0)
>>>> - 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);
>>>> - }
>>>
>>> Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would have resulted in an early return, it's okay to move all of this outside the conditional (which no longer exists!)
>>>
>>
>> Right - although if desired I can split this patch up into (at least)
>> two to make it more obvious.
>
>
> Nope. It was all decipherable as it is. Of course if you're looking to
> get your patch count up, then...
>
Nah - I was originally hoping for the leap of faith by just having 1
patch which is now 12 and could have been more.
Still in order to be true to having one patch be purely code motion - I
did insert a patch which just moves the createVport/deleteVport and
renames it. Then the followup patch will do the ensure the device was
created logic.
So patches up through here have been ACK'd and I'll separate out and
push later on today... Then I'll rework the rest of the patches.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.