[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