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


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