On 02/04/2013 08:31 AM, Osier Yang wrote: > This introduces 4 new attributes for storage pool source adapter. > E.g. > > <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> > > Attribute 'type' can be either 'scsi_host' or 'fc_host', and defaults > to 'scsi_host' if attribute 'name' is specified. I.e. It's optional > for 'scsi_host' adapter, for back-compact reason. However, it's mandatory > for 'fc_host' adapter and any new future adapter types. Attribute 'parent' > is required for vHBA, but optional for HBA. > > * docs/formatstorage.html.in: > - Add documents for the 4 new attrs > * docs/schemas/storagepool.rng: > - Add RNG schema > * src/conf/storage_conf.c: > - Parse and format the new XMLs > * src/conf/storage_conf.h: > - New struct virStoragePoolSourceAdapter, replace "char *adapter" with it; > - New enum virStoragePoolSourceAdapterType > * src/libvirt_private.syms: > - Export TypeToString and TypeFromString > * src/phyp/phyp_driver.c: > - Replace "adapter" with "adapter.data.name", which is member of the union > of the new struct virStoragePoolSourceAdapter now > * src/storage/storage_backend_scsi.c: > - Like above > * tests/storagepoolxml2xmlin/pool-scsi1.xml: > - New test for 'fc_host' adapter > * tests/storagepoolxml2xmlout/pool-scsi.xml: > - Change the expected output, as the 'type' defaults to 'scsi_host' if 'name" > specified now > * tests/storagepoolxml2xmlout/pool-scsi1.xml: > - New test > * tests/storagepoolxml2xmltest.c: > - Include the test > --- > docs/formatstorage.html.in | 13 +++- > docs/schemas/storagepool.rng | 33 +++++++- > src/conf/storage_conf.c | 123 +++++++++++++++++++++++++-- > src/conf/storage_conf.h | 23 +++++- > src/libvirt_private.syms | 2 + > src/phyp/phyp_driver.c | 8 +- > src/storage/storage_backend_scsi.c | 6 +- > tests/storagepoolxml2xmlin/pool-scsi1.xml | 15 ++++ > tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- > tests/storagepoolxml2xmlout/pool-scsi1.xml | 18 ++++ > tests/storagepoolxml2xmltest.c | 1 + > 11 files changed, 220 insertions(+), 24 deletions(-) > create mode 100644 tests/storagepoolxml2xmlin/pool-scsi1.xml > create mode 100644 tests/storagepoolxml2xmlout/pool-scsi1.xml > > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in > index 9f93db8..0849618 100644 > --- a/docs/formatstorage.html.in > +++ b/docs/formatstorage.html.in > @@ -88,8 +88,17 @@ > <span class="since">Since 0.4.1</span></dd> > <dt><code>adapter</code></dt> > <dd>Provides the source for pools backed by SCSI adapters. May > - only occur once. Contains a single attribute <code>name</code> > - which is the SCSI adapter name (ex. "host1"). > + 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, s/type,/type./ > + valid value is "fc_host" or "scsi_host", defaults to "scsi_host" > + if <code>name</code> is specified. To keep back-compact, Consider the following instead: Valid values are "fc_host" (vHBA) and "scsi_host"(HBA). If omitted and the <code>name</code> attribute is specified, then it defaults to "scsi_host". NOTE: Based on further description, I'm assuming "fc_host" is "vHBA" and "scsi_host" is "HBA". Rather than introduce (v)HBA below, I used this opportunity to define them here... s/back-compact/backwards compatibility the/ > + <code>type</code> is optional for "scsi_host" adapter, but /s/is optional for "scsi_host"/attribute is optional for the "scsi_host"/ > + mandatory for "fc_host" adapter. Attribute <code>wwnn</code> and s/for/for the/ > + <code>wwpn</code> (<span class="since">1.0.2</span>) indicates s/Attribute/Attributes/ s/indicates/indicate/ Consider perhaps: "Attributes wwnn (world wide node name) and wwpn (world wide port name) are used by the "fc_host" (vHBA) adapter to uniquely identify the device in the Fibre Channel storage fabric. Thus "the (v)HBA" below becomes unnecessary. > + the (v)HBA. The optional attribute <code>parent</code> s/optional// > + (<span class="since">1.0.2</span>) specifies the parent device of > + the (v)HBA. It's optional for HBA, but required for vHBA. s/of the v(HBA)/ The text is here is confusing because you say it's required for vHBA here, but declare it optional later. It's completely ignored for "scsi_host", but can be present for "fc_host". This is where you introduced vHBA without defining it so it has led to my confusion. If someone mistakenly supplied parent with scsi_host, then the virStoragePoolSourceFormat() will not write out the parent. > <span class="since">Since 0.6.2</span></dd> > <dt><code>host</code></dt> > <dd>Provides the source for pools backed by storage from a > diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng > index 165e276..a3204ec 100644 > --- a/docs/schemas/storagepool.rng > +++ b/docs/schemas/storagepool.rng > @@ -275,9 +275,36 @@ > > <define name='sourceinfoadapter'> > <element name='adapter'> > - <attribute name='name'> > - <text/> > - </attribute> > + <choice> > + <group> > + <!-- To keep back-compact, 'type' is not mandatory for s/compact/compat/ > + scsi_host adapter --> > + <optional> > + <attribute name='type'> > + <value>scsi_host</value> > + </attribute> > + </optional> > + <attribute name='name'> > + <text/> > + </attribute> > + </group> > + <group> > + <attribute name='type'> > + <value>fc_host</value> > + </attribute> > + <optional> > + <attribute name='parent'> > + <text/> > + </attribute> > + </optional> The text description implies otherwise, but the code certainly declares this optional. > + <attribute name='wwnn'> > + <ref name='wwn'/> > + </attribute> > + <attribute name='wwpn'> > + <ref name='wwn'/> > + </attribute> > + </group> > + </choice> > <empty/> > </element> > </define> > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 7a39998..ddb2945 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -90,6 +90,10 @@ VIR_ENUM_IMPL(virStoragePartedFsType, > "ext2", "ext2", > "extended") > > +VIR_ENUM_IMPL(virStoragePoolSourceAdapterType, > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, > + "default", "scsi_host", "fc_host") > + > typedef const char *(*virStorageVolFormatToString)(int format); > typedef int (*virStorageVolFormatFromString)(const char *format); > > @@ -304,6 +308,18 @@ virStorageVolDefFree(virStorageVolDefPtr def) { > VIR_FREE(def); > } > > +static void > +virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) > +{ > + if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > + VIR_FREE(adapter.data.fchost.wwnn); > + VIR_FREE(adapter.data.fchost.wwpn); ?? VIR_FREE(adapter.data.fchost.parent) > + } else if (adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > + VIR_FREE(adapter.data.name); > + } > +} > + > void > virStoragePoolSourceClear(virStoragePoolSourcePtr source) > { > @@ -324,7 +340,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) > VIR_FREE(source->devices); > VIR_FREE(source->dir); > VIR_FREE(source->name); > - VIR_FREE(source->adapter); > + virStoragePoolSourceAdapterClear(source->adapter); > VIR_FREE(source->initiator.iqn); > VIR_FREE(source->vendor); > VIR_FREE(source->product); > @@ -489,6 +505,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > virStoragePoolOptionsPtr options; > char *name = NULL; > char *port = NULL; > + char *adapter_type = NULL; > int n; > > relnode = ctxt->node; > @@ -580,7 +597,45 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > } > > source->dir = virXPathString("string(./dir/@path)", ctxt); > - source->adapter = virXPathString("string(./adapter/@name)", ctxt); > + > + if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { > + if ((source->adapter.type = > + virStoragePoolSourceAdapterTypeTypeFromString(adapter_type)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Unknown pool adapter type '%s'"), > + adapter_type); > + goto cleanup; > + } > + > + if (source->adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > + source->adapter.data.fchost.parent = > + virXPathString("string(./adapter/@parent)", ctxt); > + source->adapter.data.fchost.wwnn = > + virXPathString("string(./adapter/@wwnn)", ctxt); > + source->adapter.data.fchost.wwpn = > + virXPathString("string(./adapter/@wwpn)", ctxt); > + } else if (source->adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > + source->adapter.data.name = > + virXPathString("string(./adapter/@name)", ctxt); > + } > + } else { > + if (virXPathString("string(./adapter/@wwnn)", ctxt) || > + virXPathString("string(./adapter/@wwpn)", ctxt)) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("'type' for 'fc_host' adapter must be specified")); Message is confusing, consider "Use of 'wwnn' and 'wwpn' attributes not valid for 'scsi_host" adapter." or "Use of 'wwnn' and 'wwpn' attributes requires the 'fc_host' adapter 'type'". > + goto cleanup; > + } I believe you will leak here if either of these is true. Thus you'll need some sort of wwnn = virXPath... wwpn = virXPath... if (wwnn || wwpn) { VIR_FREE(wwnn); VIR_FREE(wwpn); > + > + /* To keep back-compact, 'type' is not required to specify s/back-compact/backwards compatibility/ > + * for scsi_host adapter. > + */ > + if ((source->adapter.data.name = > + virXPathString("string(./adapter/@name)", ctxt))) > + source->adapter.type = > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; > + } > > authType = virXPathString("string(./auth/@type)", ctxt); > if (authType == NULL) { > @@ -618,6 +673,7 @@ cleanup: > VIR_FREE(port); > VIR_FREE(authType); > VIR_FREE(nodeset); > + VIR_FREE(adapter_type); > return ret; > } > > @@ -819,11 +875,33 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { > } > > if (options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) { > - if (!ret->source.adapter) { > - virReportError(VIR_ERR_XML_ERROR, > - "%s", _("missing storage pool source adapter name")); > + if (!ret->source.adapter.type) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing storage pool source adapter")); > goto cleanup; > } > + > + if (ret->source.adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { The fchost.parent is optional here - following the code, but not the docs. > + if (!ret->source.adapter.data.fchost.wwnn || > + !ret->source.adapter.data.fchost.wwpn) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("'wwnn' and 'wwpn' must be specified for adapter " > + "type 'fchost'")); > + goto cleanup; > + } > + > + if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || > + !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) > + goto cleanup; > + } else if (ret->source.adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > + if (!ret->source.adapter.data.name) { > + virReportError(VIR_ERR_XML_ERROR, > + "%s", _("missing storage pool source adapter name")); > + goto cleanup; > + } > + } > } > > /* If DEVICE is the only source type, then its required */ > @@ -953,9 +1031,23 @@ virStoragePoolSourceFormat(virBufferPtr buf, > if ((options->flags & VIR_STORAGE_POOL_SOURCE_DIR) && > src->dir) > virBufferAsprintf(buf," <dir path='%s'/>\n", src->dir); > - if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && > - src->adapter) > - virBufferAsprintf(buf," <adapter name='%s'/>\n", src->adapter); > + if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER)) { > + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST || > + src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) > + virBufferAsprintf(buf, " <adapter type='%s'", > + virStoragePoolSourceAdapterTypeTypeToString(src->adapter.type)); > + > + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > + virBufferEscapeString(buf, " parent='%s'", > + src->adapter.data.fchost.parent); Since parent is possible NULL you won't write anything, right? > + virBufferAsprintf(buf," wwnn='%s' wwpn='%s'/>\n", > + src->adapter.data.fchost.wwnn, > + 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.name); > + } > + } > if ((options->flags & VIR_STORAGE_POOL_SOURCE_NAME) && > src->name) > virBufferAsprintf(buf," <name>%s</name>\n", src->name); > @@ -1856,8 +1948,19 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, > matchpool = pool; > break; > case VIR_STORAGE_POOL_SCSI: > - if (STREQ(pool->def->source.adapter, def->source.adapter)) > - matchpool = pool; > + if (pool->def->source.adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > + if (STREQ(pool->def->source.adapter.data.fchost.wwnn, > + def->source.adapter.data.fchost.wwnn) && > + STREQ(pool->def->source.adapter.data.fchost.wwpn, > + def->source.adapter.data.fchost.wwpn)) > + matchpool = pool; > + } else if (pool->def->source.adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST){ > + if (STREQ(pool->def->source.adapter.data.name, > + def->source.adapter.data.name)) > + matchpool = pool; > + } > break; > case VIR_STORAGE_POOL_ISCSI: > { > diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h > index ad16eca..60b8034 100644 > --- a/src/conf/storage_conf.h > +++ b/src/conf/storage_conf.h > @@ -233,7 +233,28 @@ struct _virStoragePoolSourceDevice { > } geometry; > }; > > +enum virStoragePoolSourceAdapterType { > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_DEFAULT = 0, > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST, > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST, > > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, > +}; > +VIR_ENUM_DECL(virStoragePoolSourceAdapterType) > + > +typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; > +struct _virStoragePoolSourceAdapter { > + int type; /* enum virStoragePoolSourceAdapterType */ > + > + union { > + char *name; > + struct { > + char *parent; > + char *wwnn; > + char *wwpn; > + } fchost; > + } data; > +}; > > typedef struct _virStoragePoolSource virStoragePoolSource; > typedef virStoragePoolSource *virStoragePoolSourcePtr; > @@ -250,7 +271,7 @@ struct _virStoragePoolSource { > char *dir; > > /* Or an adapter */ > - char *adapter; > + virStoragePoolSourceAdapter adapter; > > /* Or a name */ > char *name; > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 4a84b2b..c0c6b91 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -777,6 +777,8 @@ virStoragePoolObjLock; > virStoragePoolObjRemove; > virStoragePoolObjSaveDef; > virStoragePoolObjUnlock; > +virStoragePoolSourceAdapterTypeTypeFromString; > +virStoragePoolSourceAdapterTypeTypeToString; > virStoragePoolSourceClear; > virStoragePoolSourceFindDuplicate; > virStoragePoolSourceFindDuplicateDevices; > diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c > index 74f04ff..a12a430 100644 > --- a/src/phyp/phyp_driver.c > +++ b/src/phyp/phyp_driver.c > @@ -2058,7 +2058,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, > spdef->source.ndevice = 1; > > /*XXX source adapter not working properly, should show hdiskX */ > - if ((spdef->source.adapter = > + if ((spdef->source.adapter.data.name = So this code only works for scsi_host then? Since data.name is not valid for fc_host. > phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { > VIR_ERROR(_("Unable to determine storage pools's source adapter.")); > goto err; > @@ -2280,7 +2280,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) > > pool.source.ndevice = 1; > > - if ((pool.source.adapter = > + if ((pool.source.adapter.data.name = Same comment > phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { > VIR_ERROR(_("Unable to determine storage sps's source adapter.")); > goto cleanup; > @@ -2520,7 +2520,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) > managed_system, vios_id); > > virBufferAsprintf(&buf, "mksp -f %schild %s", def->name, > - source.adapter); > + source.adapter.data.name); and again > > if (system_type == HMC) > virBufferAddChar(&buf, '\''); > @@ -2761,7 +2761,7 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags) > def.source.ndevice = 1; > > /*XXX source adapter not working properly, should show hdiskX */ > - if ((def.source.adapter = > + if ((def.source.adapter.data.name = ditto > phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { > VIR_ERROR(_("Unable to determine storage pools's source adapter.")); > goto err; > diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c > index 90bbf59..c1c163e 100644 > --- a/src/storage/storage_backend_scsi.c > +++ b/src/storage/storage_backend_scsi.c > @@ -639,7 +639,7 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, > char *path; > > *isActive = false; > - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter) < 0) { > + if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) { > virReportOOMError(); > return -1; > } > @@ -661,9 +661,9 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, > > pool->def->allocation = pool->def->capacity = pool->def->available = 0; > > - if (sscanf(pool->def->source.adapter, "host%u", &host) != 1) { > + 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); > + pool->def->source.adapter.data.name); > retval = -1; > goto out; > } I'll make the broader assumption that something with _scsi in the filename means it only works for scsi. > diff --git a/tests/storagepoolxml2xmlin/pool-scsi1.xml b/tests/storagepoolxml2xmlin/pool-scsi1.xml > new file mode 100644 > index 0000000..1e9826d > --- /dev/null > +++ b/tests/storagepoolxml2xmlin/pool-scsi1.xml > @@ -0,0 +1,15 @@ > +<pool type='scsi'> > + <name>hba0</name> > + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> > + <source> > + <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> > + </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.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml > index 321dc2b..faf5342 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 name='host0'/> > + <adapter type='scsi_host' name='host0'/> > </source> > <target> > <path>/dev/disk/by-path</path> I think you shouldn't change this one - it becomes the backwards compatibility check. Then create a "pool-scsi-type-scsi.xml" using the new syntax (or something close/similar to indicate the "scsi_host" syntax is in use). > diff --git a/tests/storagepoolxml2xmlout/pool-scsi1.xml b/tests/storagepoolxml2xmlout/pool-scsi1.xml > new file mode 100644 > index 0000000..fb1079f > --- /dev/null > +++ b/tests/storagepoolxml2xmlout/pool-scsi1.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='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> > + </source> > + <target> > + <path>/dev/disk/by-path</path> > + <permissions> > + <mode>0700</mode> > + <owner>0</owner> > + <group>0</group> > + </permissions> > + </target> > +</pool> Change the name from "pool-scsi1.xml" -> "pool-scsi-type-fc.xml" I think that'll be clearer > diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c > index 8cac978..c04eb69 100644 > --- a/tests/storagepoolxml2xmltest.c > +++ b/tests/storagepoolxml2xmltest.c > @@ -90,6 +90,7 @@ mymain(void) > DO_TEST("pool-iscsi-auth"); > DO_TEST("pool-netfs"); > DO_TEST("pool-scsi"); > + DO_TEST("pool-scsi1"); > DO_TEST("pool-mpath"); > DO_TEST("pool-iscsi-multiiqn"); > DO_TEST("pool-iscsi-vendor-product"); > Then change/add the names here too. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list