8 Apr
2013
8 Apr
'13
10:03 a.m.
On 06/04/13 03:11, John Ferlan wrote:
> On 03/25/2013 12:43 PM, Osier Yang wrote:
>> The helper iterates over sysfs, to find out the matched scsi host
>> name by comparing the wwnn,wwpn pair. It will be used by checkPool
>> and refreshPool of storage scsi backend. New helper getAdapterName
>> is introduced in storage_backend_scsi.c, which uses the new util
>> helper virGetFCHostNameByWWN to get the fc_host adapter name.
>> ---
>> src/libvirt_private.syms | 1 +
>> src/storage/storage_backend_scsi.c | 49 ++++++++++++++---
>> src/util/virutil.c | 109 +++++++++++++++++++++++++++++++++++++
>> src/util/virutil.h | 9 ++-
>> 4 files changed, 159 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 39c02ad..3df7632 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1854,6 +1854,7 @@ virFindFileInPath;
>> virFormatIntDecimal;
>> virGetDeviceID;
>> virGetDeviceUnprivSGIO;
>> +virGetFCHostNameByWWN;
>> virGetGroupID;
>> virGetGroupName;
>> virGetHostname;
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 1bf6c0b..258c82e 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -603,6 +603,8 @@ getHostNumber(const char *adapter_name,
>> */
>> if (STRPREFIX(host, "scsi_host")) {
>> host += strlen("scsi_host");
>> + } else if (STRPREFIX(host, "fc_host")) {
>> + host += strlen("fc_host");
> How is this related? Or even possible? The number is associated (at
> least so far) with the SCSI "name" parameter which isn't used for FC.
> Both callers below still only call here iff it's SCSI_HOST.
I use getHostNumber after getAdapterName, which suits for
both scsi_host and fc_host. :-)
>
>> } else if (STRPREFIX(host, "host")) {
>> host += strlen("host");
>> } else {
>> @@ -622,42 +624,74 @@ getHostNumber(const char *adapter_name,
>> return 0;
>> }
>>
>> +static char *
>> +getAdapterName(virStoragePoolSourceAdapter adapter)
>> +{
>> + char *name = NULL;
>> +
>> + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>> + return strdup(adapter.data.name);
>> +
>> + if (!(name = virGetFCHostNameByWWN(NULL,
>> + adapter.data.fchost.wwnn,
>> + adapter.data.fchost.wwpn))) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("Failed to find SCSI host with wwnn='%s', "
>> + "wwpn='%s'"), adapter.data.fchost.wwnn,
>> + adapter.data.fchost.wwpn);
>> + return NULL;
> superfluous since name = NULL... :-)
Cleaned.
>
>> + }
>> +
>> + return name;
>> +}
>> +
>> static int
>> virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>> virStoragePoolObjPtr pool,
>> bool *isActive)
>> {
>> - char *path;
>> + char *path = NULL;
>> + char *name = NULL;
>> unsigned int host;
>> + int ret = -1;
>>
>> *isActive = false;
>>
>> - if (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
>> + if (!(name = getAdapterName(pool->def->source.adapter)))
>> return -1;
>>
>> + if (getHostNumber(name, &host) < 0)
>> + goto cleanup;
>> +
>> if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) {
>> virReportOOMError();
>> - return -1;
>> + goto cleanup;
>> }
>>
>> if (access(path, F_OK) == 0)
>> *isActive = true;
>>
>> + ret = 0;
>> +cleanup:
>> VIR_FREE(path);
>> -
>> - return 0;
>> + VIR_FREE(name);
>> + return ret;
>> }
>>
>> static int
>> virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>> virStoragePoolObjPtr pool)
>> {
>> - int ret = -1;
>> + char *name = NULL;
>> unsigned int host;
>> + int ret = -1;
>>
>> pool->def->allocation = pool->def->capacity = pool->def->available = 0;
>>
>> - if (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
>> + if (!(name = getAdapterName(pool->def->source.adapter)))
>> + return -1;
>> +
>> + if (getHostNumber(name, &host) < 0)
>> goto out;
>>
>> VIR_DEBUG("Scanning host%u", host);
>> @@ -669,6 +703,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>>
>> ret = 0;
>> out:
>> + VIR_FREE(name);
>> return ret;
>> }
>>
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 557225c..a8b9962 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -26,6 +26,7 @@
>>
>> #include <config.h>
>>
>> +#include <dirent.h>
>> #include <stdio.h>
>> #include <stdarg.h>
>> #include <stdlib.h>
>> @@ -3570,6 +3571,105 @@ cleanup:
>> VIR_FREE(operation_path);
>> return ret;
>> }
>> +
>> +/* virGetHostNameByWWN:
>> + *
>> + * Iterate over the sysfs tree to get SCSI host name (e.g. scsi_host5)
>> + * by wwnn,wwpn pair.
>> + */
>> +char *
>> +virGetFCHostNameByWWN(const char *sysfs_prefix,
>> + const char *wwnn,
>> + const char *wwpn)
>> +{
>> + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
>> + struct dirent *entry = NULL;
>> + DIR *dir = NULL;
>> + char *wwnn_path = NULL;
>> + char *wwpn_path = NULL;
>> + char *wwnn_buf = NULL;
>> + char *wwpn_buf = NULL;
>> + char *p;
>> + char *ret = NULL;
>> +
>> + if (!(dir = opendir(prefix))) {
>> + virReportSystemError(errno,
>> + _("Failed to opendir path '%s'"),
>> + prefix);
>> + return NULL;
>> + }
>> +
>> +# define READ_WWN(wwn_path, buf) \
>> + do { \
>> + if (virFileReadAll(wwn_path, 1024, &buf) < 0) \
>> + goto cleanup; \
>> + if ((p = strchr(buf, '\n'))) \
>> + *p = '\0'; \
>> + if (STRPREFIX(buf, "0x")) \
>> + p = buf + strlen("0x"); \
>> + else \
>> + p = buf; \
> suggestions:
> 1. Add another parameter to take wwnn or wwpn (ww_name?)
> 2. VIR_FREE(wwn_path) here
> 3. VIR_FREE(buf) here too
> 4. You could probably put the virFileExists check in this macro too -
> appropriately placed of course. It'd only need to free the _path
> generated from virAsprintf().
>
> VIR_FREE(wwn_path); \
> if (STRNEQ(ww_name, p)) { \
> VIR_FREE(buf); \
> continue; \
This will just jump out this "do...while" loop, perhaps there is
a way to work around. If it is, let's do in a later patch.
> } \
> VIR_FREE(buf); \
>
> I think this works - you can double check my eyesight though. After you
> exit the macro, neither the virAsprintf buffer nor the virFileReadAll
> buffers will still be allocated. Thus reducing the need for keeping
> track...
>
>> + } while (0)
>> +
>> + while ((entry = readdir(dir))) {
>> + if (entry->d_name[0] == '.')
>> + continue;
>> +
>> + if (virAsprintf(&wwnn_path, "%s%s/node_name", prefix,
>> + entry->d_name) < 0) {
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> +
>> + if (!virFileExists(wwnn_path)) {
>> + VIR_FREE(wwnn_path);
>> + continue;
>> + }
>> +
>> + READ_WWN(wwnn_path, wwnn_buf);
>> +
>> + if (STRNEQ(wwnn, p)) {
>> + VIR_FREE(wwpn_buf);
> In any case:
>
> s/wwpn_buf/wwnn_buf
>
> right?
Right, cleaned.
>
>> + VIR_FREE(wwnn_path);
>> + continue;
>> + }
>> +
>> + if (virAsprintf(&wwpn_path, "%s%s/port_name", prefix,
>> + entry->d_name) < 0) {
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> +
>> + if (!virFileExists(wwpn_path)) {
>> + VIR_FREE(wwnn_buf);
>> + VIR_FREE(wwnn_path);
>> + VIR_FREE(wwpn_path);
>> + continue;
>> + }
>> +
>> + READ_WWN(wwpn_path, wwpn_buf);
>> +
>> + if (STRNEQ(wwpn, p)) {
>> + VIR_FREE(wwnn_path);
>> + VIR_FREE(wwpn_path);
>> + VIR_FREE(wwnn_buf);
>> + VIR_FREE(wwpn_buf);
>> + continue;
>> + }
>> +
>> + ret = strdup(entry->d_name);
>> + goto cleanup;
> s/goto cleanup/break
>
> Same difference...
Cleaned.
>> + }
>> +
>> +cleanup:
>> +# undef READ_WWN
>> + closedir(dir);
>> + VIR_FREE(wwnn_path);
>> + VIR_FREE(wwpn_path);
>> + VIR_FREE(wwnn_buf);
>> + VIR_FREE(wwpn_buf);
>> + return ret;
>> +}
>> #else
>> int
>> virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED,
>> @@ -3606,4 +3706,13 @@ virManageVport(const int parent_host ATTRIBUTE_UNUSED,
>> return -1;
>> }
>>
>> +char *
>> +virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED,
>> + const char *wwnn ATTRIBUTE_UNUSED,
>> + const char *wwpn 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 47357fa..d134034 100644
>> --- a/src/util/virutil.h
>> +++ b/src/util/virutil.h
>> @@ -295,8 +295,8 @@ int virSetDeviceUnprivSGIO(const char *path,
>> int virGetDeviceUnprivSGIO(const char *path,
>> const char *sysfs_dir,
>> int *unpriv_sgio);
>> -char * virGetUnprivSGIOSysfsPath(const char *path,
>> - const char *sysfs_dir);
>> +char *virGetUnprivSGIOSysfsPath(const char *path,
>> + const char *sysfs_dir);
>> int virReadFCHost(const char *sysfs_prefix,
>> int host,
>> const char *entry,
>> @@ -317,4 +317,9 @@ int virManageVport(const int parent_host,
>> int operation)
>> ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>
>> +char *virGetFCHostNameByWWN(const char *sysfs_prefix,
>> + const char *wwnn,
>> + const char *wwpn)
>> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>> +
>> #endif /* __VIR_UTIL_H__ */
>>
> ACK - with a bit of cleanup. Depending on responses and path chosen, a
> followup could be done.
>
> John
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list