New subject: [libvirt] [PATCH 1/7] New XML attributes for storage pool source adapter

25 Mar 2013

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

[libvirt] [PATCH 0/7 v3] Persistent vHBA support in storage driver

Osier Yang

Osier Yang

John Ferlan

Osier Yang

Osier Yang

John Ferlan

Osier Yang

John Ferlan

Osier Yang

John Ferlan

Osier Yang

Osier Yang

John Ferlan

Osier Yang

Osier Yang

John Ferlan

Osier Yang

Osier Yang

John Ferlan

Osier Yang

Eric Blake

Osier Yang

Osier Yang

Osier Yang

tags

participants (3)