[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 02/04/2013 08:31 AM, Osier Yang wrote:
> node device driver names the HBA like "scsi_host5", but storage
> driver uses "host5", which could make the user confused. This
> make them consistent. However, for back-compact reason, adapter
> name like "host5" is still supported.
> ---
>  docs/formatstorage.html.in                |   13 +++++----
>  src/storage/storage_backend_scsi.c        |   43 +++++++++++++++++++++--------
>  tests/storagepoolxml2xmlin/pool-scsi.xml  |    2 +-
>  tests/storagepoolxml2xmlout/pool-scsi.xml |    2 +-
>  4 files changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index 0849618..af42fed 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -89,12 +89,13 @@
>        <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.2</span>) specifies the adapter type,
> -        valid value is "fc_host" or "scsi_host", defaults to "scsi_host"
> -        if <code>name</code> is specified. To keep back-compact,
> -        <code>type</code> is optional for "scsi_host" adapter, but
> -        mandatory for "fc_host" adapter.  Attribute <code>wwnn</code> and
> +        name (ex. "scsi_host1", name like 'host1' is not recommended to
> +        use, though it's still supported for back-compact reason).
> +        Attribute <code>type</code> (<span class="since">1.0.2</span>)
> +        specifies the adapter type, valid value is "fc_host" or "scsi_host",
> +        defaults to "scsi_host" if <code>name</code> is specified. To keep
> +        back-compact, <code>type</code> is optional for "scsi_host" adapter,
> +        but mandatory for "fc_host" adapter.  Attribute <code>wwnn</code> and

Still getting used to this adjust the previous patch, especially when it
comes to documenting.

Anyway, it seems the "new" text is ""scsi_host1", name like 'host1' is
not recommended to use, though it's still supported for back-compact reason"

Consider the following:

"scsi_host1". NB, although a name such as "host1" is still supported for
backwards compatibility, it is not recommended


>          <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
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index c1c163e..a26bf59 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -632,14 +632,38 @@ out:
>  }
>  
>  static int
> +getHostNumber(const char *adapter_name)
> +{
> +    int host;

This was a uint32_t before...  So it can be negative :-)

> +
> +    /* Specifying adpater like 'host5' is still supported for

s/adpater/adapter/

> +     * back-compact reason.

s/back-compact reason/backwards compatibility reasons/

> +     */
> +    if ((sscanf(adapter_name, "scsi_host%d", &host) != 1) &&
> +        (sscanf(adapter_name, "host%d", &host) != 1)) {

This was %u before

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to get host number from '%s'"),
> +                       adapter_name);
> +        return -1;
> +    }
> +
> +    return host;
> +}
> +
> +static int
>  virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                 virStoragePoolObjPtr pool,
>                                 bool *isActive)
>  {
>      char *path;
> +    int host;

Use uint32_t

>  
>      *isActive = false;
> -    if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) {
> +
> +    if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0)
> +        return -1;
> +
> +    if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) {
>          virReportOOMError();
>          return -1;
>      }
> @@ -656,29 +680,24 @@ static int
>  virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   virStoragePoolObjPtr pool)
>  {
> -    int retval = 0;
> -    uint32_t host;
> +    int ret = -1;
> +    int host;

I think you need to keep this a uint32_t - if only because callee's
expect it that way
>  
>      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 ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0)
>          goto out;
> -    }
>  
>      VIR_DEBUG("Scanning host%u", host);

host is now an 'int'?

>  
> -    if (virStorageBackendSCSITriggerRescan(host) < 0) {
> -        retval = -1;
> +    if (virStorageBackendSCSITriggerRescan(host) < 0)
>          goto out;
> -    }

This call expects a uint32_t

>  
>      virStorageBackendSCSIFindLUs(pool, host);

As does this one

>  
> +    ret = 0;
>  out:
> -    return retval;
> +    return ret;
>  }
>  
>  
> diff --git a/tests/storagepoolxml2xmlin/pool-scsi.xml b/tests/storagepoolxml2xmlin/pool-scsi.xml
> index 3650e43..091ecfc 100644
> --- a/tests/storagepoolxml2xmlin/pool-scsi.xml
> +++ b/tests/storagepoolxml2xmlin/pool-scsi.xml
> @@ -2,7 +2,7 @@
>    <name>hba0</name>
>    <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>    <source>
> -    <adapter name="host0"/>
> +    <adapter name="scsi_host0"/>
>    </source>
>    <target>
>      <path>/dev/disk/by-path</path>
> diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml
> index faf5342..101b61a 100644
> --- a/tests/storagepoolxml2xmlout/pool-scsi.xml
> +++ b/tests/storagepoolxml2xmlout/pool-scsi.xml
> @@ -5,7 +5,7 @@
>    <allocation unit='bytes'>0</allocation>
>    <available unit='bytes'>0</available>
>    <source>
> -    <adapter type='scsi_host' name='host0'/>
> +    <adapter type='scsi_host' name='scsi_host0'/>
>    </source>
>    <target>
>      <path>/dev/disk/by-path</path>
> 

Considering my comments in 1/8, here we now have the need for new tests.
 One that accepts "host0" and one that accepts "scsi_host0". That would
be true for both the "name" only and the "type" & "name" options.  The
point being, you've gone from one way to describe things to 4 ways:

"name=host0"
"name=scsi_host0"
"type=scsi_host name=host0"
"type=scsi_host name=scsi_host0"

Unless of course, options 2 & 3 above are not possible...

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]