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

Re: [libvirt] [PATCH 02/11] storage: Introduce new XMLs for stable SCSI host addressing



On 19/06/13 20:31, John Ferlan wrote:
On 06/07/2013 01:03 PM, Osier Yang wrote:
The SCSI host number is not stable on Linux platform, the number
can be changed after a system rebooting or scsi kernel modules
reloaded. To have a stable address for the scsi_host adapter of
scsi pool, this introduces new XMLs like:

Consider:

The SCSI host number is not a static value that should be used as a

Using term "stable" is better, the scsi host number can be changed
or not changed after a system reboot or kernel modules reloads, one
never knows it. "not static" sounds like it always changes..

unique identifier. The value can change based on SCSI kernel module
reloads or after a system reboot. This patch will introduce a mechanism
to uniquely identify the SCSI host based upon the parent and unique_id
attributes generated as part of the kernel file system path. The new XML
format will be:

   <adapter type='scsi_host' parent='pci_0000_00_1f_2' unique_id='5'/>

Where "parent" is the parent device of the scsi host, it should be
consistent with the name style what node device driver uses (Either
udev backend style or HAL backend style), or the PCI address in format
"domain:bus:slot:function" format. "unique_id" is the number exposed
by sysfs. E.g:

% cat /sys/bus/pci/devices/0000:00:1f.2/ata5/host4/scsi_host/host4/unique_id
5

The attribute "parent" is required, attribute "unique_id" is optional,
if it's omitted, the scsi host which has smallest unique_id under the
s/scsi/SCSI

"parent" device will be used.

"parent" and the old "name" attribute are exclusive, since they are both to
indicate scsi host number.

The "parent" and "name" attributes are mutually exclusive to identify
the SCSI host number. Use of the "parent" attribute will be the
preferred mechanism.

This only supports to parse and format the XMLs. Later patch will
add code to find out the scsi host number via "parent" and "unique_id";
Oh joy.

---
  docs/schemas/basictypes.rng                        | 20 ++++++--
  src/conf/storage_conf.c                            | 57 +++++++++++++++++++---
  src/conf/storage_conf.h                            |  3 ++
  .../pool-scsi-type-scsi-host-stable.xml            | 15 ++++++
  .../pool-scsi-type-scsi-host-stable.xml            | 18 +++++++
  tests/storagepoolxml2xmltest.c                     |  1 +
  6 files changed, 104 insertions(+), 10 deletions(-)
  create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml
  create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml

I see no change to docs/formatstorage.html - the examples there need to
be updated and the descriptions of fields/attributes need to be changed.

Of real importance would be that for 'scsi_host' one must choose to use
either "name" or "parent" and "unique_id" to describe things; however,
it seems that use of "parent" is preferred since it provides a better
way to uniquely identify things after reboots or SCSI kernel module reloads.

I would expect to see an example of the scsi_host entry and perhaps some
details about how someone could "generate" the parent name based on the
expectations of the code.  That is the parent name seems to have a
fairly specific format - that format is derived using a udev or hal
backend style (I don't know what those are, btw).  So as a consumer of
this code - where would I look for each style and how would I generate
the parent value in order to abide by the rules that the code will use.

Agreed. It's the thing I forgot.


diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 34c2254..3bdf923 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -347,9 +347,23 @@
                <value>scsi_host</value>
              </attribute>
            </optional>
-          <attribute name='name'>
-            <text/>
-          </attribute>
+          <choice>
+            <group>
+              <attribute name='name'>
+                <text/>
+              </attribute>
+            </group>
+            <group>
+              <attribute name='parent'>
+                <text/>
+              </attribute>
+              <optional>
+                <attribute name='unique_id'>
+                  <ref name='unsignedInt'/>
+                </attribute>
+              </optional>
+            </group>
+          </choice>
          </group>
          <group>
            <attribute name='type'>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index a44e021..d7901af 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -320,6 +320,7 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter)
      } else if (adapter.type ==
                 VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
          VIR_FREE(adapter.data.scsi_host.name);
+        VIR_FREE(adapter.data.scsi_host.parent);
      }
  }
@@ -613,6 +614,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
      source->dir = virXPathString("string(./dir/@path)", ctxt);
if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) {
+        int rc;
+
          if ((source->adapter.type =
               virStoragePoolSourceAdapterTypeTypeFromString(adapter_type)) <= 0) {
              virReportError(VIR_ERR_XML_ERROR,
@@ -633,23 +636,47 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
                     VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
              source->adapter.data.scsi_host.name =
                  virXPathString("string(./adapter/@name)", ctxt);
+            source->adapter.data.scsi_host.parent =
+                virXPathString("string(./adapter/@parent)", ctxt);
+
+            if ((rc = virXPathUInt("string(./adapter/@unique_id)", ctxt,
+                                   &source->adapter.data.scsi_host.unique_id)) < -1) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("Invalid unique_id for scsi source adapter"));
+                goto cleanup;
+            } else if (rc == 0) {
+                source->adapter.data.scsi_host.has_unique_id = true;
+            }
[1]
If I read the .rng file correctly, I think you only care about the
'unique_id' if "parent" is found. If someone supplies "name" and
'unique_id' that appears to be legal to this code, although illogical -
that is:

<adapter type='scsi_host' name="host0" unique_id='5'>

In this case, the unique_id is ignored, it won't be used anyway. I'm not sure
if it's necessary to be added, if we do like so, the XML parsing code will
be much large than current.  I'm thinking about it...


So either you keep this code as is or add a check below after the if
"!name && !parent" and "name && parent" for "if name and has_unique_id,
then illegal xml

          }
      } else {
          char *wwnn = NULL;
          char *wwpn = NULL;
          char *parent = NULL;
+        bool has_unique_id = false;
+        int rc;
+
+        rc = virXPathUInt("string(./adapter/@unique_id)", ctxt,
+                          &source->adapter.data.scsi_host.unique_id);
+
+        if (rc < -1) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Invalid unique_id for scsi source adapter"));
+            goto cleanup;
In the grand scheme of things - this is irrelevant.  It seems you should
just be checking if it's provided regardless of validity (there's no
validation of wwnn, wwpn, and parent below).  That is, no need for 'rc'
and if the return of virXPathUInt is successful, eg, == 0, then
has_unique_id = true.

Agreed, it's redundant here.


+        } else if (rc == 0) {
+            has_unique_id = true;
+        }
wwnn = virXPathString("string(./adapter/@wwnn)", ctxt);
          wwpn = virXPathString("string(./adapter/@wwpn)", ctxt);
          parent = virXPathString("string(./adapter/@parent)", ctxt);
- if (wwnn || wwpn || parent) {
+        if (wwnn || wwpn || parent || has_unique_id) {
              VIR_FREE(wwnn);
              VIR_FREE(wwpn);
              VIR_FREE(parent);
              virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("Use of 'wwnn', 'wwpn', and 'parent' attributes "
-                             "requires the 'fc_host' adapter 'type'"));
+                           _("Use of 'wwnn', 'wwpn', 'parent' and 'unique_id' "
+                             "attributes requires the adapter 'type' specified"));
s/specified/is specified

              goto cleanup;
          }
@@ -927,9 +954,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
                  goto error;
          } else if (ret->source.adapter.type ==
                     VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
-            if (!ret->source.adapter.data.scsi_host.name) {
+            if (!ret->source.adapter.data.scsi_host.name &&
+                !ret->source.adapter.data.scsi_host.parent) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("Either 'name' or 'parent' must be specified "
+                                 "for 'scsi_host' adapter"));
s/for/for the/

+                goto error;
+            }
+
+            if (ret->source.adapter.data.scsi_host.name &&
+                ret->source.adapter.data.scsi_host.parent) {
                  virReportError(VIR_ERR_XML_ERROR, "%s",
-                               _("missing storage pool source adapter name"));
+                               _("'name' and 'parent' are exclusive "
+                                 "for 'scsi_host' adapter"));

The error message should read "Both 'name' and 'parent' cannot be
specified for the 'scsi_host' adapter."

                  goto error;
              }

[1] See note from above regarding 'name' and 'unique_id' being set.


          }
@@ -1084,8 +1121,14 @@ virStoragePoolSourceFormat(virBufferPtr buf,
                                src->adapter.data.fchost.wwpn);
          } else if (src->adapter.type ==
                   VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
-            virBufferAsprintf(buf," name='%s'/>\n",
-                              src->adapter.data.scsi_host.name);
+            if (src->adapter.data.scsi_host.name) {
+                virBufferAsprintf(buf," name='%s'/>\n",
+                                  src->adapter.data.scsi_host.name);
+            } else {
+                virBufferAsprintf(buf," parent='%s' unique_id='%u'/>\n",
+                                  src->adapter.data.scsi_host.parent,
+                                  src->adapter.data.scsi_host.unique_id);
+            }
Why no check for "has_unique_id" being set before printing?  If not
provided on, this would result in a unique_id=0, which probably is not
right...

Later patch will find the smallest unique_id if it's not specified in the XML,
it's fine to keep it as-is here, instead of checking it here and removing
afterwards. It's the thing which won't break anything.


John

          }
      }
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 823d5af..801e41c 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -239,6 +239,9 @@ struct _virStoragePoolSourceAdapter {
      union {
          struct {
              char *name;
+            char *parent;
+            unsigned int unique_id;
+            bool has_unique_id;
          } scsi_host;
          struct {
              char *parent;
diff --git a/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml
new file mode 100644
index 0000000..fdf4002
--- /dev/null
+++ b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml
@@ -0,0 +1,15 @@
+<pool type="scsi">
+  <name>hba0</name>
+  <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+  <source>
+    <adapter type='scsi_host' parent='scsi_host' unique_id="5"/>
+  </source>
+  <target>
+    <path>/dev/disk/by-path</path>
+    <permissions>
+      <mode>0700</mode>
+      <owner>0</owner>
+      <group>0</group>
+    </permissions>
+  </target>
+</pool>
diff --git a/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml
new file mode 100644
index 0000000..49d1cec
--- /dev/null
+++ b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml
@@ -0,0 +1,18 @@
+<pool type='scsi'>
+  <name>hba0</name>
+  <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+  <capacity unit='bytes'>0</capacity>
+  <allocation unit='bytes'>0</allocation>
+  <available unit='bytes'>0</available>
+  <source>
+    <adapter type='scsi_host' parent='scsi_host' unique_id='5'/>
+  </source>
+  <target>
+    <path>/dev/disk/by-path</path>
+    <permissions>
+      <mode>0700</mode>
+      <owner>0</owner>
+      <group>0</group>
+    </permissions>
+  </target>
+</pool>
diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index 0376e63..a9d36c5 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -97,6 +97,7 @@ mymain(void)
      DO_TEST("pool-iscsi-multiiqn");
      DO_TEST("pool-iscsi-vendor-product");
      DO_TEST("pool-sheepdog");
+    DO_TEST("pool-scsi-type-scsi-host-stable");
return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
  }



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