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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list