Re: [libvirt] [PATCH 1/8] 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-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,

+ valid value is "fc_host" or "scsi_host", defaults to "scsi_host" + if <code>name</code> is specified. To keep back-compact,

+ <code>type</code> is optional for "scsi_host" adapter, but

+ mandatory for "fc_host" adapter. Attribute <code>wwnn</code> and

+ <code>wwpn</code> (<span class="since">1.0.2</span>) indicates

+ the (v)HBA. The optional attribute <code>parent</code>

+ (<span class="since">1.0.2</span>) specifies the parent device of + the (v)HBA. It's optional for HBA, but required for vHBA.

<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

+ 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 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);

+ } 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"));

+ goto cleanup; + }

+ + /* To keep back-compact, '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 +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) {

+ 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);

+ 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 =

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 =

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

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 =

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

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>

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");