On 02/06/2013 09:34 AM, Osier Yang wrote:
> 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.
>
sscanf can return -1 for other errors (ENOMEM included) - I would think
it's safer to check and fail than assume anything.
>>
>>> + 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.
>
The point is you're failing silently using 'continue;'. I suppose the
VIR_DEBUG() messages satisfy the need to perhaps determine why the FC
Host "someone" sets up isn't found. Although that assumes they are
debugging what's wrong with their config.
Also I'm OK that in the 1st condition the VIR_FREE(max_vports) is not
required, it will be in the second condition though.
>>> +
>>> + 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list