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