[libvirt] [PATCH 8/8] storage: Guess the parent if it's not specified for vHBA
Osier Yang
jyang at redhat.com
Wed Feb 6 14:34:04 UTC 2013
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list