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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
