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

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

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.

> 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'>

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.

> + } 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...

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