8:34 a.m.
On 2013年02月06日 05:44, John Ferlan wrote:
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..
I don't think it's possible to return -1, as all the entries
under SYSFS_FC_HOST_PATH should be "hostN". Entry "." and
".."
are already skipped.
> + 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);
No need. As it's NULL as long as the return value of
virReadFCHost < 0.
> + 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);
Likewise.
> + continue;
> + }
So do we document somewhere that the FC Host must have these two
attributes defined for us to consider using them?
Not clear about your meaning, but the two sysfs entries are only
for HBA. Not vHBA. A FC Host can be either a HBA or vHBA. And we
have debug message as long as they are not supported by a FC HOST.
> +
> + if ((strlen(max_vports)> strlen(vports)) ||
> + ((strlen(max_vports) == strlen(vports))&&
Why not just one ">="
Okay. That's a bad fault.
> + 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
This returns the sysfs entry's value as string.
that will convert the string to the number and compare using that.
So no number here.
There's possible some code reuse that could make this and that
patch a
whole lot easier.
So I compare the string directly above, to avoid convering the strings
to numbers first.
> +
> + 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
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list