Re: [libvirt] [PATCH v3 16/18] conf: Move/rename createVport and deleteVport

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(a)redhat.com&gt; --- 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; }