Re: [libvirt] [PATCH 1/7] New XML attributes for storage pool source adapter

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-compat reason. However, mandatory for 'fc_host' adapter and any new future adapter types. Attribute 'parent' is to specify the parent for the fc_host adapter. * 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. Later patch will add the checking, as "adapter.data.name" is only valid for "scsi_host" adapter. * src/storage/storage_backend_scsi.c: - Like above * tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host.xml: * tests/storagepoolxml2xmlin/pool-scsi-type-fc-host.xml: - New test for 'fc_host' and "scsi_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-scsi-type-scsi-host.xml: * tests/storagepoolxml2xmlout/pool-scsi-type-fc-host.xml: - New test * tests/storagepoolxml2xmltest.c: - Include the test --- docs/formatstorage.html.in | 16 ++- docs/schemas/storagepool.rng | 33 +++++- src/conf/storage_conf.c | 132 +++++++++++++++++++-- src/conf/storage_conf.h | 23 +++- src/libvirt_private.syms | 2 + src/phyp/phyp_driver.c | 8 +- src/storage/storage_backend_scsi.c | 6 +- .../pool-scsi-type-fc-host.xml | 15 +++ .../pool-scsi-type-scsi-host.xml | 15 +++ .../pool-scsi-type-fc-host.xml | 18 +++ .../pool-scsi-type-scsi-host.xml | 18 +++ tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- tests/storagepoolxml2xmltest.c | 2 + 13 files changed, 266 insertions(+), 24 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host.xml create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 9b68738..eff3016 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -88,8 +88,20 @@ <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.4</span>) specifies the adapter type.

+ Valid value are "fc_host" and "scsi_host". If omitted and

+ the <code>name</code> attribute is specified, then it defaults to + "scsi_host". To keep backwards compatibility, the attribute + <code>type</code> is optional for the "scsi_host" adapter, but + mandatory for the "fc_host" adapter. Attributes <code>wwnn</code> + (Word Wide Node Name) and <code>wwpn</code> (Word Wide Port Name) + (<span class="since">1.0.4</span>) are used by the "fc_host" adapter

+ to uniquely indentify the device in the Fibre Channel storage farbic

+ (the device can be either a HBA or vHBA). The optional attribute

+ <code>parent</code> (<span class="since">1.0.4</span>) specifies the

+ parent device for the "fc_host" adapter.

<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 0cc0406..43283d2 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -276,9 +276,36 @@ <define name='sourceinfoadapter'> <element name='adapter'> - <attribute name='name'> - <text/> - </attribute> + <choice> + <group> + <!-- To keep back-compat, 'type' is not mandatory for + 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> + <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 9134a22..deb4221 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,19 @@ 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 +341,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 +506,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolOptionsPtr options; char *name = NULL; char *port = NULL; + char *adapter_type = NULL; int n; relnode = ctxt->node; @@ -580,7 +598,53 @@ 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 { + char *wwnn = NULL; + char *wwpn = NULL; + + wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); + wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); + + if (wwnn || wwpn) { + VIR_FREE(wwnn); + VIR_FREE(wwpn); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Use of 'wwnn' and 'wwpn' attributes " + "requires the 'fc_host' adapter 'type'")); + goto cleanup; + }

+ + /* To keep back-compat, 'type' is not required to specify + * 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 +682,7 @@ cleanup: VIR_FREE(port); VIR_FREE(authType); VIR_FREE(nodeset); + VIR_FREE(adapter_type); return ret; } @@ -819,11 +884,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) { + 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 +1040,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); + 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 +1957,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 a2c4a54..39c02ad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -614,6 +614,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 59cc1ca..3a97364 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2062,7 +2062,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 =

phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { VIR_ERROR(_("Unable to determine storage pools's source adapter.")); goto err; @@ -2284,7 +2284,7 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) pool.source.ndevice = 1; - if ((pool.source.adapter = + if ((pool.source.adapter.data.name =

phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { VIR_ERROR(_("Unable to determine storage sps's source adapter.")); goto cleanup; @@ -2524,7 +2524,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) managed_system, vios_id); virBufferAsprintf(&buf, "mksp -f %schild %s", def->name, - source.adapter); + source.adapter.data.name);

if (system_type == HMC) virBufferAddChar(&buf, '\''); @@ -2765,7 +2765,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 =

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; } diff --git a/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host.xml new file mode 100644 index 0000000..1e9826d --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host.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/storagepoolxml2xmlin/pool-scsi-type-scsi-host.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host.xml new file mode 100644 index 0000000..4f48133 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host.xml @@ -0,0 +1,15 @@ +<pool type="scsi"> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <adapter type='scsi_host' name="host0"/> + </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-fc-host.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-fc-host.xml new file mode 100644 index 0000000..fb1079f --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-scsi-type-fc-host.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> diff --git a/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host.xml new file mode 100644 index 0000000..faf5342 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host.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' name='host0'/> + </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> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 8cac978..9be63c5 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -90,6 +90,8 @@ mymain(void) DO_TEST("pool-iscsi-auth"); DO_TEST("pool-netfs"); DO_TEST("pool-scsi"); + DO_TEST("pool-scsi-type-scsi-host"); + DO_TEST("pool-scsi-type-fc-host"); DO_TEST("pool-mpath"); DO_TEST("pool-iscsi-multiiqn"); DO_TEST("pool-iscsi-vendor-product");