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

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

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux