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

Re: [libvirt] [PATCH 8/8] storage: Guess the parent if it's not specified for vHBA



On 02/04/2013 08:32 AM, Osier Yang wrote:
> This finds the parent for vHBA by iterating over all the HBA
> which supports vport_ops capability on the host, and return
> the first one which is online, not saturated (vports in use
> is less than max_vports).
> ---
>  docs/formatstorage.html.in         |    3 +-
>  src/libvirt_private.syms           |    1 +
>  src/storage/storage_backend_scsi.c |   10 +++--
>  src/util/virutil.c                 |   83 ++++++++++++++++++++++++++++++++++++
>  src/util/virutil.h                 |    2 +
>  5 files changed, 93 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index af42fed..7315865 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -99,8 +99,7 @@
>          <code>wwpn</code> (<span class="since">1.0.2</span>) indicates
>          the (v)HBA. The optional attribute <code>parent</code>
>          (<span class="since">1.0.2</span>) specifies the parent device of
> -        the (v)HBA. It's optional for HBA, but required for vHBA.
> -        <span class="since">Since 0.6.2</span></dd>
> +        the (v)HBA. <span class="since">Since 0.6.2</span></dd>

The silent scream :-)

>        <dt><code>host</code></dt>
>        <dd>Provides the source for pools backed by storage from a
>          remote server. Will be used in combination with a <code>directory</code>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7334e15..759d630 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1841,6 +1841,7 @@ virFileStripSuffix;
>  virFileUnlock;
>  virFileWaitForDevices;
>  virFileWriteStr;
> +virFindFCHostCapableVport;
>  virFindFileInPath;
>  virFormatIntDecimal;
>  virGetDeviceID;
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index a9b96a3..59abeb0 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -646,10 +646,12 @@ createVport(virStoragePoolSourceAdapter adapter)
>                                adapter.data.fchost.wwpn))
>          return 0;
>  
> -    if (!adapter.data.fchost.parent) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("'parent' for vHBA must be specified"));
> -        return -1;
> +    if (!adapter.data.fchost.parent &&
> +        !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
> +         virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("'parent' for vHBA not specified, and "
> +                         "cannot find one on this host"));
> +         return -1;
>      }
>  
>      if ((parent_host = getHostNumber(adapter.data.fchost.parent)) < 0)
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 3073337..b9a6166 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -3555,6 +3555,82 @@ cleanup:
>      VIR_FREE(wwpn_buf);
>      return ret;
>  }
> +
> +# define PORT_STATE_ONLINE "Online"
> +
> +/* virFindFCHostCapableVport:
> + *
> + * Iterate over the sysfs and find out the first online HBA which
> + * supports vport, and not saturated,.
> + */
> +char *
> +virFindFCHostCapableVport(const char *sysfs_prefix)
> +{
> +    const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
> +    DIR *dir = NULL;
> +    struct dirent *entry = NULL;
> +    char *max_vports = NULL;
> +    char *vports = NULL;
> +    char *state = NULL;
> +    char *ret = NULL;
> +
> +    if (!(dir = opendir(prefix))) {
> +        virReportSystemError(errno,
> +                             _("Failed to opendir path '%s'"),
> +                             prefix);
> +        return NULL;
> +    }
> +
> +    while ((entry = readdir(dir))) {
> +        if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, ".."))
> +            continue;
> +
> +        int host;

uint32_t?  To be consistent with other comments.

> +
> +        ignore_value(sscanf(entry->d_name, "host%d", &host));

Why ignore_value()?  If == -1, then host is undefined - could be
something that results in the following being successful..

> +        if (!virIsCapableVport(NULL, host))
> +            continue;
> +
> +        if (virReadFCHost(NULL, host, "port_state", &state) < 0) {
> +             VIR_DEBUG("Failed to read port_state for host%d", host);
> +             continue;
> +        }
> +
> +        /* Skip the not online FC host */
> +        if (!STREQ(state, PORT_STATE_ONLINE)) {
> +            VIR_FREE(state);
> +            continue;
> +        }
> +        VIR_FREE(state);
> +
> +        if (virReadFCHost(NULL, host, "max_npiv_vports", &max_vports) < 0) {
> +             VIR_DEBUG("Failed to read max_npiv_vports for host%d", host);

VIR_FREE(max_vports);
> +             continue;
> +        }
> +
> +        if (virReadFCHost(NULL, host, "npiv_vports_inuse", &vports) < 0) {
> +             VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host);
> +             VIR_FREE(max_vports);
VIR_FREE(vports);
> +             continue;
> +        }

So do we document somewhere that the FC Host must have these two
attributes defined for us to consider using them?
> +
> +        if ((strlen(max_vports) > strlen(vports)) ||
> +            ((strlen(max_vports) == strlen(vports)) &&

Why not just one ">="

> +             strcmp(max_vports, vports) > 0)) {
> +            ret = strdup(entry->d_name);
> +            goto cleanup;
> +        }

What is being tested?  The name or the value?  I didn't go back to look
at what virReadFCHost() provides, but I do see there is a patch:

https://www.redhat.com/archives/libvir-list/2013-January/msg00947.html

that will convert the string to the number and compare using that.
There's possible some code reuse that could make this and that patch a
whole lot easier.

> +
> +        VIR_FREE(max_vports);
> +        VIR_FREE(vports);
> +    }
> +
> +cleanup:
> +    closedir(dir);
> +    VIR_FREE(max_vports);
> +    VIR_FREE(vports);
> +    return ret;
> +}
>  #else
>  int
>  virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> @@ -3600,4 +3676,11 @@ virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED,
>      return NULL;
>  }
>  
> +char *
> +virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED)
> +{
> +    virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> +    return NULL;
> +}
> +
>  #endif /* __linux__ */
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 78b50a8..3a68134 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -320,4 +320,6 @@ char * virGetFCHostNameByWWN(const char *sysfs_prefix,
>                               const char *wwpn)
>      ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>  
> +char * virFindFCHostCapableVport(const char *sysfs_prefix );
> +
>  #endif /* __VIR_UTIL_H__ */
> 


John


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