[libvirt] [PATCH 2/7] storage: Make the adapter name be consistent with node device driver

[John Ferlan]

On 03/25/2013 12:43 PM, Osier Yang wrote:
> node device driver names the HBA like "scsi_host5", but storage
> driver uses "host5", which could make the user confused. This
> changes them to be consistent. However, for back-compat reason,
> adapter name like "host5" is still supported.
> 
> v1 - v2:
>   * Use virStrToLong_ui instead of sscanf
>   * No tests addition or changes, because this patch only affects
>     the way scsi backend works for adapter, adding xml2xml tests for
>     it is just meaningless.
> ---
>  docs/formatstorage.html.in         | 15 ++++++-----
>  src/storage/storage_backend_scsi.c | 54 +++++++++++++++++++++++++++++---------
>  2 files changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index eff3016..5fae933 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -89,13 +89,14 @@
>        <dt><code>adapter</code></dt>
>        <dd>Provides the source for pools backed by SCSI adapters. May
>          only occur once. Attribute <code>name</code> is the SCSI adapter
> -        name (ex. "host1"). Attribute <code>type</code>
> -        (<span class="since">1.0.4</span>) specifies the adapter type.
> -        Valid value are "fc_host" and "scsi_host".  If omitted and
> -        the <code>name</code> attribute is specified, then it defaults to
> -        "scsi_host". To keep backwards compatibility, the attribute
> -        <code>type</code> is optional for the "scsi_host" adapter, but
> -        mandatory for the "fc_host" adapter.  Attributes <code>wwnn</code>
> +        name (ex. "scsi_host1". NB, although a name such as "host1" is
> +        still supported for backwards compatibility, it is not recommended).
> +        Attribute <code>type</code> (<span class="since">1.0.4</span>)
> +        specifies the adapter type. Valid value are "fc_host" and "scsi_host".
> +        If omitted and the <code>name</code> attribute is specified, then it
> +        defaults to "scsi_host". To keep backwards compatibility, the
> +        attribute <code>type</code> is optional for the "scsi_host" adapter,
> +        but mandatory for the "fc_host" adapter.  Attributes <code>wwnn</code>
>          (Word Wide Node Name) and <code>wwpn</code> (Word Wide Port Name)
>          (<span class="since">1.0.4</span>) are used by the "fc_host" adapter
>          to uniquely indentify the device in the Fibre Channel storage farbic
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index c1c163e..cc1ebe2 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -632,14 +632,49 @@ out:
>  }
>  
>  static int
> +getHostNumber(const char *adapter_name,
> +              unsigned int *result)
> +{
> +    char *host = (char *)adapter_name;
> +
> +    /* Specifying adapter like 'host5' is still supported for
> +     * back-compat reason.
> +     */
> +    if (STRPREFIX(host, "scsi_host")) {
> +        host += strlen("scsi_host");
> +    } else if (STRPREFIX(host, "host")) {
> +        host += strlen("host");
> +    } else {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("invalid adapter name '%s' for scsi pool"),

s/invalid/Invalid
s/scsi/SCSI

> +                       adapter_name);
> +        return -1;
> +    }
> +
> +    if (result && virStrToLong_ui(host, NULL, 10, result) == -1) {

Consider using ATTRIBUTE_NONNULL(2) on the argument instead...  Although
this is a local/static function...

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("invalid adapter name '%s' for scsi pool"),

s/invalid/Invalid
s/scsi/SCSI

> +                       adapter_name);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int
>  virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                 virStoragePoolObjPtr pool,
>                                 bool *isActive)
>  {
>      char *path;
> +    unsigned int host;
>  
>      *isActive = false;
> -    if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) {
> +
> +    if (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
> +        return -1;
> +
> +    if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) {
>          virReportOOMError();
>          return -1;
>      }
> @@ -656,29 +691,24 @@ static int
>  virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   virStoragePoolObjPtr pool)
>  {
> -    int retval = 0;
> -    uint32_t host;
> +    int ret = -1;
> +    unsigned int host;
>  
>      pool->def->allocation = pool->def->capacity = pool->def->available = 0;
>  
> -    if (sscanf(pool->def->source.adapter.data.name, "host%u", &host) != 1) {
> -        VIR_DEBUG("Failed to get host number from '%s'",
> -                    pool->def->source.adapter.data.name);
> -        retval = -1;
> +    if (getHostNumber(pool->def->source.adapter.data.name, &host) < 0)
>          goto out;
> -    }
>  
>      VIR_DEBUG("Scanning host%u", host);
>  
> -    if (virStorageBackendSCSITriggerRescan(host) < 0) {
> -        retval = -1;
> +    if (virStorageBackendSCSITriggerRescan(host) < 0)
>          goto out;
> -    }
>  
>      virStorageBackendSCSIFindLUs(pool, host);
>  
> +    ret = 0;
>  out:
> -    return retval;
> +    return ret;
>  }
>  
>  
> 

ACK with cleanups