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

Osier Yang jyang at redhat.com
Thu Jun 20 01:27:29 UTC 2013


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;
>>   }
>>




More information about the libvir-list mailing list