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

Osier Yang jyang at redhat.com
Wed Feb 6 12:43:39 UTC 2013


On 2013年02月06日 04:20, John Ferlan wrote:
> 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

I don't see much difference, but since English is not my native
language, I believe what you suggested is better. Will update.

>
>
>>           <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/

I will use "back-compat", as it's used across.

>
>> +     */
>> +    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...

Right, agreed, I will create a new test.

>
> John
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list