[libvirt] [PATCH 0/8 v2] Persistent vHBA support in storage pool

This is the 3rd part to implement NPIV migration support [1]. Part 1: (New version) https://www.redhat.com/archives/libvir-list/2013-February/msg00112.html Part 2: (Already ACKed by Michal) https://www.redhat.com/archives/libvir-list/2013-January/msg00859.html Part 4: (Partly ACKed by John) https://www.redhat.com/archives/libvir-list/2013-January/msg02113.html The patches are based on Part 2. The new XMLs might be too long, might be deserved to have them as sub-elements of <adapter>. But I'd like to see the feedback first. Osier Yang (8): New XML attributes for storage pool source adapter storage: Make the adapter name be consistent with node device driver storage: Move virStorageBackendSCSIGetHostNumber into iscsi backend phyp: Prohibit fc_host adapter for phyp driver util: Add helper to get the scsi host name by iterating over sysfs util: Fix bug of managing vport storage: Add startPool and stopPool for scsi backend storage: Guess the parent if it's not specified for vHBA docs/formatstorage.html.in | 15 ++- docs/schemas/storagepool.rng | 33 +++++- src/conf/storage_conf.c | 123 ++++++++++++++++-- src/conf/storage_conf.h | 23 +++- src/libvirt_private.syms | 4 + src/phyp/phyp_driver.c | 15 ++- src/storage/storage_backend_iscsi.c | 39 ++++++- src/storage/storage_backend_scsi.c | 190 +++++++++++++++++++-------- src/storage/storage_backend_scsi.h | 3 - src/util/virutil.c | 196 +++++++++++++++++++++++++++- src/util/virutil.h | 7 + tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlin/pool-scsi1.xml | 15 ++ tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlout/pool-scsi1.xml | 18 +++ tests/storagepoolxml2xmltest.c | 1 + 16 files changed, 602 insertions(+), 84 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi1.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi1.xml [1] https://www.redhat.com/archives/libvir-list/2012-November/msg00826.html Regards, Osier

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

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

On 2013年02月06日 03:48, John Ferlan wrote:
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./
Okay,
+ 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".
Sounds good.
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...
But one still can use "scsi_host" for vHBA. So I don't think "fc_host (vHBA)" and "scsi_host (HBA)" is good.
s/back-compact/backwards compatibility the/
Okay, but /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"/
I'd like: attribute <code>type</code> is optional for the "scsi_host".
+ mandatory for "fc_host" adapter. Attribute<code>wwnn</code> and
s/for/for the/
Okay.
+<code>wwpn</code> (<span class="since">1.0.2</span>) indicates
s/Attribute/Attributes/ s/indicates/indicate/
Okay.
Consider perhaps: "Attributes wwnn (world wide node name) and wwpn
Additions on your proposal: <code>wwnn</code>, <code>wwpn</code>
(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.
It's sorted out in later patch. Because "find one parent for the vHBA" is implemented in later patch, not in this one. The reason for making you confused is one can use "wwnn && wwpn" to indicate a HBA, which doesn't need the parent. Hmm I tend to agree with you that it's easier to just say it's optional here. Because it doesn't error out when parsing, and also the rng schema defines it as optional.
<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/
Okay.
+ 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.
As said above, this will be not problem anymore once I just say it's 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)
Good catch.
+ } 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'".
I like the later one.
+ 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);
Right.
+ + /* To keep back-compact, 'type' is not required to specify
s/back-compact/backwards compatibility/
Okay.
+ * 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?
Yes. It's the magic of virBufferEscapeString.
+ 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.
Yes, later patch adds checking before this.
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
Since we will support to create pool has adapter of 'fc_host' type. It can only be "scsi_host".
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
Likewise.
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
Likewise.
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.
Yeah, it's sorted out in later patch, the reason for I simply do the replacement is this patch is just for paring and formating.
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).
Agreed. The new name is nice.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

node device driver names the HBA like "scsi_host5", but storage driver uses "host5", which could make the user confused. This make them consistent. However, for back-compact reason, adapter name like "host5" is still supported. --- docs/formatstorage.html.in | 13 +++++---- src/storage/storage_backend_scsi.c | 43 +++++++++++++++++++++-------- tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 0849618..af42fed 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -89,12 +89,13 @@ <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters. May 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 + name (ex. "scsi_host1", name like 'host1' is not recommended to + use, though it's still supported for back-compact reason). + 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 diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c1c163e..a26bf59 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -632,14 +632,38 @@ out: } static int +getHostNumber(const char *adapter_name) +{ + int host; + + /* Specifying adpater like 'host5' is still supported for + * back-compact reason. + */ + if ((sscanf(adapter_name, "scsi_host%d", &host) != 1) && + (sscanf(adapter_name, "host%d", &host) != 1)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get host number from '%s'"), + adapter_name); + return -1; + } + + return host; +} + +static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) { char *path; + int host; *isActive = false; - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) { + + if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) + return -1; + + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) { virReportOOMError(); return -1; } @@ -656,29 +680,24 @@ static int virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - int retval = 0; - uint32_t host; + int ret = -1; + int host; pool->def->allocation = pool->def->capacity = pool->def->available = 0; - 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.data.name); - retval = -1; + if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) goto out; - } VIR_DEBUG("Scanning host%u", host); - if (virStorageBackendSCSITriggerRescan(host) < 0) { - retval = -1; + if (virStorageBackendSCSITriggerRescan(host) < 0) goto out; - } virStorageBackendSCSIFindLUs(pool, host); + ret = 0; out: - return retval; + return ret; } diff --git a/tests/storagepoolxml2xmlin/pool-scsi.xml b/tests/storagepoolxml2xmlin/pool-scsi.xml index 3650e43..091ecfc 100644 --- a/tests/storagepoolxml2xmlin/pool-scsi.xml +++ b/tests/storagepoolxml2xmlin/pool-scsi.xml @@ -2,7 +2,7 @@ <name>hba0</name> <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> <source> - <adapter name="host0"/> + <adapter name="scsi_host0"/> </source> <target> <path>/dev/disk/by-path</path> diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml index faf5342..101b61a 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 type='scsi_host' name='host0'/> + <adapter type='scsi_host' name='scsi_host0'/> </source> <target> <path>/dev/disk/by-path</path> -- 1.7.7.6

On 02/04/2013 08:31 AM, Osier Yang wrote:
node device driver names the HBA like "scsi_host5", but storage driver uses "host5", which could make the user confused. This make them consistent. However, for back-compact reason, adapter name like "host5" is still supported. --- docs/formatstorage.html.in | 13 +++++---- src/storage/storage_backend_scsi.c | 43 +++++++++++++++++++++-------- tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- 4 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 0849618..af42fed 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -89,12 +89,13 @@ <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters. May 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 + name (ex. "scsi_host1", name like 'host1' is not recommended to + use, though it's still supported for back-compact reason). + 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
Still getting used to this adjust the previous patch, especially when it comes to documenting. Anyway, it seems the "new" text is ""scsi_host1", name like 'host1' is not recommended to use, though it's still supported for back-compact reason" Consider the following: "scsi_host1". NB, although a name such as "host1" is still supported for backwards compatibility, it is not recommended
<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 diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c1c163e..a26bf59 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -632,14 +632,38 @@ out: }
static int +getHostNumber(const char *adapter_name) +{ + int host;
This was a uint32_t before... So it can be negative :-)
+ + /* Specifying adpater like 'host5' is still supported for
s/adpater/adapter/
+ * back-compact reason.
s/back-compact reason/backwards compatibility reasons/
+ */ + if ((sscanf(adapter_name, "scsi_host%d", &host) != 1) && + (sscanf(adapter_name, "host%d", &host) != 1)) {
This was %u before
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get host number from '%s'"), + adapter_name); + return -1; + } + + return host; +} + +static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) { char *path; + int host;
Use uint32_t
*isActive = false; - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name) < 0) { + + if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) + return -1; + + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) { virReportOOMError(); return -1; } @@ -656,29 +680,24 @@ static int virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - int retval = 0; - uint32_t host; + int ret = -1; + int host;
I think you need to keep this a uint32_t - if only because callee's expect it that way
pool->def->allocation = pool->def->capacity = pool->def->available = 0;
- 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.data.name); - retval = -1; + if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) goto out; - }
VIR_DEBUG("Scanning host%u", host);
host is now an 'int'?
- if (virStorageBackendSCSITriggerRescan(host) < 0) { - retval = -1; + if (virStorageBackendSCSITriggerRescan(host) < 0) goto out; - }
This call expects a uint32_t
virStorageBackendSCSIFindLUs(pool, host);
As does this one
+ ret = 0; out: - return retval; + return ret; }
diff --git a/tests/storagepoolxml2xmlin/pool-scsi.xml b/tests/storagepoolxml2xmlin/pool-scsi.xml index 3650e43..091ecfc 100644 --- a/tests/storagepoolxml2xmlin/pool-scsi.xml +++ b/tests/storagepoolxml2xmlin/pool-scsi.xml @@ -2,7 +2,7 @@ <name>hba0</name> <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> <source> - <adapter name="host0"/> + <adapter name="scsi_host0"/> </source> <target> <path>/dev/disk/by-path</path> diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml index faf5342..101b61a 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 type='scsi_host' name='host0'/> + <adapter type='scsi_host' name='scsi_host0'/> </source> <target> <path>/dev/disk/by-path</path>
Considering my comments in 1/8, here we now have the need for new tests. One that accepts "host0" and one that accepts "scsi_host0". That would be true for both the "name" only and the "type" & "name" options. The point being, you've gone from one way to describe things to 4 ways: "name=host0" "name=scsi_host0" "type=scsi_host name=host0" "type=scsi_host name=scsi_host0" Unless of course, options 2 & 3 above are not possible... John

On 2013年02月06日 04:20, John Ferlan wrote:
On 02/04/2013 08:31 AM, Osier Yang wrote:
node device driver names the HBA like "scsi_host5", but storage driver uses "host5", which could make the user confused. This make them consistent. However, for back-compact reason, adapter name like "host5" is still supported. --- docs/formatstorage.html.in | 13 +++++---- src/storage/storage_backend_scsi.c | 43 +++++++++++++++++++++-------- tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- 4 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 0849618..af42fed 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -89,12 +89,13 @@ <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters. May 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 + name (ex. "scsi_host1", name like 'host1' is not recommended to + use, though it's still supported for back-compact reason). + 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
Still getting used to this adjust the previous patch, especially when it comes to documenting.
Anyway, it seems the "new" text is ""scsi_host1", name like 'host1' is not recommended to use, though it's still supported for back-compact reason"
Consider the following:
"scsi_host1". NB, although a name such as "host1" is still supported for backwards compatibility, it is not recommended
I don't see much difference, but since English is not my native language, I believe what you suggested is better. Will update.
<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 diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c1c163e..a26bf59 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -632,14 +632,38 @@ out: }
static int +getHostNumber(const char *adapter_name) +{ + int host;
This was a uint32_t before... So it can be negative :-)
+ + /* Specifying adpater like 'host5' is still supported for
s/adpater/adapter/
+ * back-compact reason.
s/back-compact reason/backwards compatibility reasons/
I will use "back-compat", as it's used across.
+ */ + if ((sscanf(adapter_name, "scsi_host%d",&host) != 1)&& + (sscanf(adapter_name, "host%d",&host) != 1)) {
This was %u before
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get host number from '%s'"), + adapter_name); + return -1; + } + + return host; +} + +static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) { char *path; + int host;
Use uint32_t
*isActive = false; - if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter.data.name)< 0) { + + if ((host = getHostNumber(pool->def->source.adapter.data.name))< 0) + return -1; + + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host)< 0) { virReportOOMError(); return -1; } @@ -656,29 +680,24 @@ static int virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - int retval = 0; - uint32_t host; + int ret = -1; + int host;
I think you need to keep this a uint32_t - if only because callee's expect it that way
pool->def->allocation = pool->def->capacity = pool->def->available = 0;
- 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.data.name); - retval = -1; + if ((host = getHostNumber(pool->def->source.adapter.data.name))< 0) goto out; - }
VIR_DEBUG("Scanning host%u", host);
host is now an 'int'?
- if (virStorageBackendSCSITriggerRescan(host)< 0) { - retval = -1; + if (virStorageBackendSCSITriggerRescan(host)< 0) goto out; - }
This call expects a uint32_t
virStorageBackendSCSIFindLUs(pool, host);
As does this one
+ ret = 0; out: - return retval; + return ret; }
diff --git a/tests/storagepoolxml2xmlin/pool-scsi.xml b/tests/storagepoolxml2xmlin/pool-scsi.xml index 3650e43..091ecfc 100644 --- a/tests/storagepoolxml2xmlin/pool-scsi.xml +++ b/tests/storagepoolxml2xmlin/pool-scsi.xml @@ -2,7 +2,7 @@ <name>hba0</name> <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> <source> -<adapter name="host0"/> +<adapter name="scsi_host0"/> </source> <target> <path>/dev/disk/by-path</path> diff --git a/tests/storagepoolxml2xmlout/pool-scsi.xml b/tests/storagepoolxml2xmlout/pool-scsi.xml index faf5342..101b61a 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 type='scsi_host' name='host0'/> +<adapter type='scsi_host' name='scsi_host0'/> </source> <target> <path>/dev/disk/by-path</path>
Considering my comments in 1/8, here we now have the need for new tests. One that accepts "host0" and one that accepts "scsi_host0". That would be true for both the "name" only and the "type"& "name" options. The point being, you've gone from one way to describe things to 4 ways:
"name=host0" "name=scsi_host0" "type=scsi_host name=host0" "type=scsi_host name=scsi_host0"
Unless of course, options 2& 3 above are not possible...
Right, agreed, I will create a new test.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

It's only used by iscsi backend. --- src/storage/storage_backend_iscsi.c | 39 ++++++++++++++++++++++++++++++++++- src/storage/storage_backend_scsi.c | 39 ----------------------------------- src/storage/storage_backend_scsi.h | 3 -- 3 files changed, 38 insertions(+), 43 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index f374961..da4367c 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -23,6 +23,7 @@ #include <config.h> +#include <dirent.h> #include <sys/socket.h> #include <netdb.h> #include <sys/types.h> @@ -401,6 +402,42 @@ cleanup: return ret; } +static int +virStorageBackendISCSIGetHostNumber(const char *sysfs_path, + uint32_t *host) +{ + int retval = 0; + DIR *sysdir = NULL; + struct dirent *dirent = NULL; + + VIR_DEBUG("Finding host number from '%s'", sysfs_path); + + virFileWaitForDevices(); + + sysdir = opendir(sysfs_path); + + if (sysdir == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), sysfs_path); + retval = -1; + goto out; + } + + while ((dirent = readdir(sysdir))) { + if (STREQLEN(dirent->d_name, "target", strlen("target"))) { + if (sscanf(dirent->d_name, + "target%u:", host) != 1) { + VIR_DEBUG("Failed to parse target '%s'", dirent->d_name); + retval = -1; + break; + } + } + } + + closedir(sysdir); +out: + return retval; +} static int virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, @@ -416,7 +453,7 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, return -1; } - if (virStorageBackendSCSIGetHostNumber(sysfs_path, &host) < 0) { + if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0) { virReportSystemError(errno, _("Failed to get host number for iSCSI session " "with path '%s'"), diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a26bf59..257001c 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -547,45 +547,6 @@ out: return retval; } - -int -virStorageBackendSCSIGetHostNumber(const char *sysfs_path, - uint32_t *host) -{ - int retval = 0; - DIR *sysdir = NULL; - struct dirent *dirent = NULL; - - VIR_DEBUG("Finding host number from '%s'", sysfs_path); - - virFileWaitForDevices(); - - sysdir = opendir(sysfs_path); - - if (sysdir == NULL) { - virReportSystemError(errno, - _("Failed to opendir path '%s'"), sysfs_path); - retval = -1; - goto out; - } - - while ((dirent = readdir(sysdir))) { - if (STREQLEN(dirent->d_name, "target", strlen("target"))) { - if (sscanf(dirent->d_name, - "target%u:", host) != 1) { - VIR_DEBUG("Failed to parse target '%s'", dirent->d_name); - retval = -1; - break; - } - } - } - - closedir(sysdir); -out: - return retval; -} - - static int virStorageBackendSCSITriggerRescan(uint32_t host) { diff --git a/src/storage/storage_backend_scsi.h b/src/storage/storage_backend_scsi.h index 1cdd0da..0984fd5 100644 --- a/src/storage/storage_backend_scsi.h +++ b/src/storage/storage_backend_scsi.h @@ -33,9 +33,6 @@ extern virStorageBackend virStorageBackendSCSI; int -virStorageBackendSCSIGetHostNumber(const char *sysfs_path, - uint32_t *host); -int virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, uint32_t scanhost); -- 1.7.7.6

On 02/04/2013 08:32 AM, Osier Yang wrote:
It's only used by iscsi backend. --- src/storage/storage_backend_iscsi.c | 39 ++++++++++++++++++++++++++++++++++- src/storage/storage_backend_scsi.c | 39 ----------------------------------- src/storage/storage_backend_scsi.h | 3 -- 3 files changed, 38 insertions(+), 43 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index f374961..da4367c 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -23,6 +23,7 @@
#include <config.h>
+#include <dirent.h> #include <sys/socket.h> #include <netdb.h> #include <sys/types.h> @@ -401,6 +402,42 @@ cleanup: return ret; }
+static int +virStorageBackendISCSIGetHostNumber(const char *sysfs_path, + uint32_t *host)
need one more space ^
+{ + int retval = 0; + DIR *sysdir = NULL; + struct dirent *dirent = NULL; + + VIR_DEBUG("Finding host number from '%s'", sysfs_path); + + virFileWaitForDevices(); + + sysdir = opendir(sysfs_path); + + if (sysdir == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), sysfs_path); + retval = -1; + goto out; + } + + while ((dirent = readdir(sysdir))) { + if (STREQLEN(dirent->d_name, "target", strlen("target"))) { + if (sscanf(dirent->d_name, + "target%u:", host) != 1) { + VIR_DEBUG("Failed to parse target '%s'", dirent->d_name); + retval = -1; + break; + } + } + } + + closedir(sysdir); +out: + return retval; +}
static int virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, @@ -416,7 +453,7 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, return -1; }
- if (virStorageBackendSCSIGetHostNumber(sysfs_path, &host) < 0) { + if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0) { virReportSystemError(errno, _("Failed to get host number for iSCSI session " "with path '%s'"), diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a26bf59..257001c 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -547,45 +547,6 @@ out: return retval; }
- -int -virStorageBackendSCSIGetHostNumber(const char *sysfs_path, - uint32_t *host) -{ - int retval = 0; - DIR *sysdir = NULL; - struct dirent *dirent = NULL; - - VIR_DEBUG("Finding host number from '%s'", sysfs_path); - - virFileWaitForDevices(); - - sysdir = opendir(sysfs_path); - - if (sysdir == NULL) { - virReportSystemError(errno, - _("Failed to opendir path '%s'"), sysfs_path); - retval = -1; - goto out; - } - - while ((dirent = readdir(sysdir))) { - if (STREQLEN(dirent->d_name, "target", strlen("target"))) { - if (sscanf(dirent->d_name, - "target%u:", host) != 1) { - VIR_DEBUG("Failed to parse target '%s'", dirent->d_name); - retval = -1; - break; - } - } - } - - closedir(sysdir); -out: - return retval; -} - - static int virStorageBackendSCSITriggerRescan(uint32_t host) { diff --git a/src/storage/storage_backend_scsi.h b/src/storage/storage_backend_scsi.h index 1cdd0da..0984fd5 100644 --- a/src/storage/storage_backend_scsi.h +++ b/src/storage/storage_backend_scsi.h @@ -33,9 +33,6 @@ extern virStorageBackend virStorageBackendSCSI;
int -virStorageBackendSCSIGetHostNumber(const char *sysfs_path, - uint32_t *host); -int virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, uint32_t scanhost);
ACK w/ the space edit.

It's possible to support fc_host adapter for phyp driver too, but at this stage I'd like to not allow it when I'm not that clear how it works. --- src/phyp/phyp_driver.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index a12a430..c359048 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2515,6 +2515,13 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + if (source.adapter.type != + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Only 'scsi_host' adapter is supported")); + goto cleanup; + } + if (system_type == HMC) virBufferAsprintf(&buf, "viosvrcmd -m %s --id %d -c '", managed_system, vios_id); -- 1.7.7.6

The helper iterates over sysfs, to find out the matched scsi host name by comparing the wwnn,wwpn pair. It's used for checkPool and refreshPool of storage scsi backend. New helper getAdapterName is introduced in storage_backend_scsi.c, it uses the new util helper virGetFCHostNameByWWN to get the fc_host adapter name. --- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 50 ++++++++++++++--- src/util/virutil.c | 109 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 5 ++ 4 files changed, 157 insertions(+), 8 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c0c6b91..7334e15 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1845,6 +1845,7 @@ virFindFileInPath; virFormatIntDecimal; virGetDeviceID; virGetDeviceUnprivSGIO; +virGetFCHostNameByWWN; virGetGroupID; virGetGroupName; virGetHostname; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 257001c..8166311 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -601,7 +601,8 @@ getHostNumber(const char *adapter_name) * back-compact reason. */ if ((sscanf(adapter_name, "scsi_host%d", &host) != 1) && - (sscanf(adapter_name, "host%d", &host) != 1)) { + (sscanf(adapter_name, "host%d", &host) != 1) && + (sscanf(adapter_name, "fc_host%d", &host) != 1)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get host number from '%s'"), adapter_name); @@ -611,42 +612,74 @@ getHostNumber(const char *adapter_name) return host; } +static char * +getAdapterName(virStoragePoolSourceAdapter adapter) +{ + char *name = NULL; + + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + return strdup(adapter.data.name); + + if (!(name = virGetFCHostNameByWWN(NULL, + adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn))) { + virReportError(VIR_ERR_XML_ERROR, + _("Failed to find SCSI host with wwnn='%s', " + "wwpn='%s'"), adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn); + return NULL; + } + + return name; +} + static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) { - char *path; + char *path = NULL; + char *name = NULL; int host; + int ret = -1; *isActive = false; - if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) + if (!(name = getAdapterName(pool->def->source.adapter))) return -1; + if ((host = getHostNumber(name)) < 0) + goto cleanup; + if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) { virReportOOMError(); - return -1; + goto cleanup; } if (access(path, F_OK) == 0) *isActive = true; + ret = 0; +cleanup: VIR_FREE(path); - - return 0; + VIR_FREE(name); + return ret; } static int virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - int ret = -1; + char *name = NULL; int host; + int ret = -1; pool->def->allocation = pool->def->capacity = pool->def->available = 0; - if ((host = getHostNumber(pool->def->source.adapter.data.name)) < 0) + if (!(name = getAdapterName(pool->def->source.adapter))) + return -1; + + if ((host = getHostNumber(name)) < 0) goto out; VIR_DEBUG("Scanning host%u", host); @@ -658,6 +691,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, ret = 0; out: + VIR_FREE(name); return ret; } diff --git a/src/util/virutil.c b/src/util/virutil.c index dd8829d..f38ad5a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -26,6 +26,7 @@ #include <config.h> +#include <dirent.h> #include <stdio.h> #include <stdarg.h> #include <stdlib.h> @@ -3455,6 +3456,105 @@ cleanup: VIR_FREE(operation_path); return ret; } + +/* virGetHostNameByWWN: + * + * Iterate over the sysfs tree to get SCSI host name (e.g. scsi_host5) + * by wwnn,wwpn pair. + */ +char * +virGetFCHostNameByWWN(const char *sysfs_prefix, + const char *wwnn, + const char *wwpn) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; + struct dirent *entry = NULL; + DIR *dir = NULL; + char *wwnn_path = NULL; + char *wwpn_path = NULL; + char *wwnn_buf = NULL; + char *wwpn_buf = NULL; + char *p; + char *ret = NULL; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + return NULL; + } + +# define READ_WWN(wwn_path, buf) \ + do { \ + if (virFileReadAll(wwn_path, 1024, &buf) < 0) \ + goto cleanup; \ + if ((p = strchr(buf, '\n'))) \ + *p = '\0'; \ + if (STRPREFIX(buf, "0x")) \ + p = buf + strlen("0x"); \ + else \ + p = buf; \ + } while (0) + + while ((entry = readdir(dir))) { + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + if (virAsprintf(&wwnn_path, "%s%s/node_name", prefix, + entry->d_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virFileExists(wwnn_path)) { + VIR_FREE(wwnn_path); + continue; + } + + READ_WWN(wwnn_path, wwnn_buf); + + if (STRNEQ(wwnn, p)) { + VIR_FREE(wwpn_buf); + VIR_FREE(wwnn_path); + continue; + } + + if (virAsprintf(&wwpn_path, "%s%s/port_name", prefix, + entry->d_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!virFileExists(wwpn_path)) { + VIR_FREE(wwnn_buf); + VIR_FREE(wwnn_path); + VIR_FREE(wwpn_path); + continue; + } + + READ_WWN(wwpn_path, wwpn_buf); + + if (STRNEQ(wwpn, p)) { + VIR_FREE(wwnn_path); + VIR_FREE(wwpn_path); + VIR_FREE(wwnn_buf); + VIR_FREE(wwpn_buf); + continue; + } + + ret = strdup(entry->d_name); + goto cleanup; + } + +cleanup: +# undef READ_WWN + closedir(dir); + VIR_FREE(wwnn_path); + VIR_FREE(wwpn_path); + VIR_FREE(wwnn_buf); + VIR_FREE(wwpn_buf); + return ret; +} #else int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, @@ -3491,4 +3591,13 @@ virManageVport(const int parent_host ATTRIBUTE_UNUSED, return -1; } +char * +virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, + const char *wwnn ATTRIBUTE_UNUSED, + const char *wwpn ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + #endif /* __linux__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 3283180..78b50a8 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -315,4 +315,9 @@ int virManageVport(const int parent_host, int operation) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +char * virGetFCHostNameByWWN(const char *sysfs_prefix, + const char *wwnn, + const char *wwpn) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_UTIL_H__ */ -- 1.7.7.6

The string written to "vport_create" or "vport_delete" should be "wwnn:wwpn", but not "wwpn:wwnn". --- src/util/virutil.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index f38ad5a..3073337 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3437,8 +3437,8 @@ virManageVport(const int parent_host, if (virAsprintf(&vport_name, "%s:%s", - wwpn, - wwnn) < 0) { + wwnn, + wwpn) < 0) { virReportOOMError(); goto cleanup; } -- 1.7.7.6

On 02/04/2013 08:32 AM, Osier Yang wrote:
The string written to "vport_create" or "vport_delete" should be "wwnn:wwpn", but not "wwpn:wwnn". --- src/util/virutil.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index f38ad5a..3073337 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3437,8 +3437,8 @@ virManageVport(const int parent_host,
if (virAsprintf(&vport_name, "%s:%s", - wwpn, - wwnn) < 0) { + wwnn, + wwpn) < 0) { virReportOOMError(); goto cleanup; }
ACK John

startPool creates the vHBA if it's not existed yet, stopPool destroys the vHBA. Also to support autostart, checkPool will creates the vHBA if it's not existed yet. --- src/storage/storage_backend_scsi.c | 64 ++++++++++++++++++++++++++++++++++++ 1 files changed, 64 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 8166311..a9b96a3 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -634,6 +634,36 @@ getAdapterName(virStoragePoolSourceAdapter adapter) } static int +createVport(virStoragePoolSourceAdapter adapter) +{ + int parent_host; + + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + return 0; + + /* This filters either HBA or already created vHBA */ + if (virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn)) + return 0; + + if (!adapter.data.fchost.parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA must be specified")); + return -1; + } + + if ((parent_host = getHostNumber(adapter.data.fchost.parent)) < 0) + return -1; + + if (virManageVport(parent_host, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn, VPORT_CREATE) < 0) + return -1; + + virFileWaitForDevices(); + return 0; +} + +static int virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) @@ -695,10 +725,44 @@ out: return ret; } +static int +virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + virStoragePoolSourceAdapter adapter = pool->def->source.adapter; + + return createVport(adapter); +} + +static int +virStorageBackendSCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool) +{ + virStoragePoolSourceAdapter adapter = pool->def->source.adapter; + int parent_host; + + if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + return 0; + + if (!(virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn))) + return -1; + + if ((parent_host = getHostNumber(adapter.data.fchost.parent)) < 0) + return -1; + + if (virManageVport(parent_host, adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn, VPORT_DELETE) < 0) + return -1; + + return 0; +} virStorageBackend virStorageBackendSCSI = { .type = VIR_STORAGE_POOL_SCSI, .checkPool = virStorageBackendSCSICheckPool, .refreshPool = virStorageBackendSCSIRefreshPool, + .startPool = virStorageBackendSCSIStartPool, + .stopPool = virStorageBackendSCSIStopPool, }; -- 1.7.7.6

This finds the parent for vHBA by iterating over all the HBA which supports vport_ops capability on the host, and return the first one which is online, not saturated (vports in use is less than max_vports). --- docs/formatstorage.html.in | 3 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 10 +++-- src/util/virutil.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 + 5 files changed, 93 insertions(+), 6 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index af42fed..7315865 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -99,8 +99,7 @@ <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> + the (v)HBA. <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 remote server. Will be used in combination with a <code>directory</code> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7334e15..759d630 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1841,6 +1841,7 @@ virFileStripSuffix; virFileUnlock; virFileWaitForDevices; virFileWriteStr; +virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; virGetDeviceID; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a9b96a3..59abeb0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -646,10 +646,12 @@ createVport(virStoragePoolSourceAdapter adapter) adapter.data.fchost.wwpn)) return 0; - if (!adapter.data.fchost.parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA must be specified")); - return -1; + if (!adapter.data.fchost.parent && + !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + return -1; } if ((parent_host = getHostNumber(adapter.data.fchost.parent)) < 0) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3073337..b9a6166 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3555,6 +3555,82 @@ cleanup: VIR_FREE(wwpn_buf); return ret; } + +# define PORT_STATE_ONLINE "Online" + +/* virFindFCHostCapableVport: + * + * Iterate over the sysfs and find out the first online HBA which + * supports vport, and not saturated,. + */ +char * +virFindFCHostCapableVport(const char *sysfs_prefix) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; + DIR *dir = NULL; + struct dirent *entry = NULL; + char *max_vports = NULL; + char *vports = NULL; + char *state = NULL; + char *ret = NULL; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + return NULL; + } + + while ((entry = readdir(dir))) { + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + int host; + + ignore_value(sscanf(entry->d_name, "host%d", &host)); + if (!virIsCapableVport(NULL, host)) + continue; + + if (virReadFCHost(NULL, host, "port_state", &state) < 0) { + VIR_DEBUG("Failed to read port_state for host%d", host); + continue; + } + + /* Skip the not online FC host */ + if (!STREQ(state, PORT_STATE_ONLINE)) { + VIR_FREE(state); + continue; + } + VIR_FREE(state); + + if (virReadFCHost(NULL, host, "max_npiv_vports", &max_vports) < 0) { + VIR_DEBUG("Failed to read max_npiv_vports for host%d", host); + continue; + } + + if (virReadFCHost(NULL, host, "npiv_vports_inuse", &vports) < 0) { + VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); + VIR_FREE(max_vports); + continue; + } + + if ((strlen(max_vports) > strlen(vports)) || + ((strlen(max_vports) == strlen(vports)) && + strcmp(max_vports, vports) > 0)) { + ret = strdup(entry->d_name); + goto cleanup; + } + + VIR_FREE(max_vports); + VIR_FREE(vports); + } + +cleanup: + closedir(dir); + VIR_FREE(max_vports); + VIR_FREE(vports); + return ret; +} #else int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, @@ -3600,4 +3676,11 @@ virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, return NULL; } +char * +virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + #endif /* __linux__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 78b50a8..3a68134 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -320,4 +320,6 @@ char * virGetFCHostNameByWWN(const char *sysfs_prefix, const char *wwpn) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +char * virFindFCHostCapableVport(const char *sysfs_prefix ); + #endif /* __VIR_UTIL_H__ */ -- 1.7.7.6

On 02/04/2013 08:32 AM, Osier Yang wrote:
This finds the parent for vHBA by iterating over all the HBA which supports vport_ops capability on the host, and return the first one which is online, not saturated (vports in use is less than max_vports). --- docs/formatstorage.html.in | 3 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 10 +++-- src/util/virutil.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 + 5 files changed, 93 insertions(+), 6 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index af42fed..7315865 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -99,8 +99,7 @@ <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> + the (v)HBA. <span class="since">Since 0.6.2</span></dd>
The silent scream :-)
<dt><code>host</code></dt> <dd>Provides the source for pools backed by storage from a remote server. Will be used in combination with a <code>directory</code> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7334e15..759d630 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1841,6 +1841,7 @@ virFileStripSuffix; virFileUnlock; virFileWaitForDevices; virFileWriteStr; +virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; virGetDeviceID; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a9b96a3..59abeb0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -646,10 +646,12 @@ createVport(virStoragePoolSourceAdapter adapter) adapter.data.fchost.wwpn)) return 0;
- if (!adapter.data.fchost.parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA must be specified")); - return -1; + if (!adapter.data.fchost.parent && + !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + return -1; }
if ((parent_host = getHostNumber(adapter.data.fchost.parent)) < 0) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3073337..b9a6166 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3555,6 +3555,82 @@ cleanup: VIR_FREE(wwpn_buf); return ret; } + +# define PORT_STATE_ONLINE "Online" + +/* virFindFCHostCapableVport: + * + * Iterate over the sysfs and find out the first online HBA which + * supports vport, and not saturated,. + */ +char * +virFindFCHostCapableVport(const char *sysfs_prefix) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; + DIR *dir = NULL; + struct dirent *entry = NULL; + char *max_vports = NULL; + char *vports = NULL; + char *state = NULL; + char *ret = NULL; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + return NULL; + } + + while ((entry = readdir(dir))) { + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + int host;
uint32_t? To be consistent with other comments.
+ + ignore_value(sscanf(entry->d_name, "host%d", &host));
Why ignore_value()? If == -1, then host is undefined - could be something that results in the following being successful..
+ if (!virIsCapableVport(NULL, host)) + continue; + + if (virReadFCHost(NULL, host, "port_state", &state) < 0) { + VIR_DEBUG("Failed to read port_state for host%d", host); + continue; + } + + /* Skip the not online FC host */ + if (!STREQ(state, PORT_STATE_ONLINE)) { + VIR_FREE(state); + continue; + } + VIR_FREE(state); + + if (virReadFCHost(NULL, host, "max_npiv_vports", &max_vports) < 0) { + VIR_DEBUG("Failed to read max_npiv_vports for host%d", host);
VIR_FREE(max_vports);
+ continue; + } + + if (virReadFCHost(NULL, host, "npiv_vports_inuse", &vports) < 0) { + VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); + VIR_FREE(max_vports); VIR_FREE(vports); + continue; + }
So do we document somewhere that the FC Host must have these two attributes defined for us to consider using them?
+ + if ((strlen(max_vports) > strlen(vports)) || + ((strlen(max_vports) == strlen(vports)) &&
Why not just one ">="
+ strcmp(max_vports, vports) > 0)) { + ret = strdup(entry->d_name); + goto cleanup; + }
What is being tested? The name or the value? I didn't go back to look at what virReadFCHost() provides, but I do see there is a patch: https://www.redhat.com/archives/libvir-list/2013-January/msg00947.html that will convert the string to the number and compare using that. There's possible some code reuse that could make this and that patch a whole lot easier.
+ + VIR_FREE(max_vports); + VIR_FREE(vports); + } + +cleanup: + closedir(dir); + VIR_FREE(max_vports); + VIR_FREE(vports); + return ret; +} #else int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, @@ -3600,4 +3676,11 @@ virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, return NULL; }
+char * +virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + #endif /* __linux__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 78b50a8..3a68134 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -320,4 +320,6 @@ char * virGetFCHostNameByWWN(const char *sysfs_prefix, const char *wwpn) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+char * virFindFCHostCapableVport(const char *sysfs_prefix ); + #endif /* __VIR_UTIL_H__ */
John

On 2013年02月06日 05:44, John Ferlan wrote:
On 02/04/2013 08:32 AM, Osier Yang wrote:
This finds the parent for vHBA by iterating over all the HBA which supports vport_ops capability on the host, and return the first one which is online, not saturated (vports in use is less than max_vports). --- docs/formatstorage.html.in | 3 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 10 +++-- src/util/virutil.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 + 5 files changed, 93 insertions(+), 6 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index af42fed..7315865 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -99,8 +99,7 @@ <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> + the (v)HBA.<span class="since">Since 0.6.2</span></dd>
The silent scream :-)
<dt><code>host</code></dt> <dd>Provides the source for pools backed by storage from a remote server. Will be used in combination with a<code>directory</code> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7334e15..759d630 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1841,6 +1841,7 @@ virFileStripSuffix; virFileUnlock; virFileWaitForDevices; virFileWriteStr; +virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; virGetDeviceID; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a9b96a3..59abeb0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -646,10 +646,12 @@ createVport(virStoragePoolSourceAdapter adapter) adapter.data.fchost.wwpn)) return 0;
- if (!adapter.data.fchost.parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA must be specified")); - return -1; + if (!adapter.data.fchost.parent&& + !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + return -1; }
if ((parent_host = getHostNumber(adapter.data.fchost.parent))< 0) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3073337..b9a6166 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3555,6 +3555,82 @@ cleanup: VIR_FREE(wwpn_buf); return ret; } + +# define PORT_STATE_ONLINE "Online" + +/* virFindFCHostCapableVport: + * + * Iterate over the sysfs and find out the first online HBA which + * supports vport, and not saturated,. + */ +char * +virFindFCHostCapableVport(const char *sysfs_prefix) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; + DIR *dir = NULL; + struct dirent *entry = NULL; + char *max_vports = NULL; + char *vports = NULL; + char *state = NULL; + char *ret = NULL; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + return NULL; + } + + while ((entry = readdir(dir))) { + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + int host;
uint32_t? To be consistent with other comments.
+ + ignore_value(sscanf(entry->d_name, "host%d",&host));
Why ignore_value()? If == -1, then host is undefined - could be something that results in the following being successful..
I don't think it's possible to return -1, as all the entries under SYSFS_FC_HOST_PATH should be "hostN". Entry "." and ".." are already skipped.
+ if (!virIsCapableVport(NULL, host)) + continue; + + if (virReadFCHost(NULL, host, "port_state",&state)< 0) { + VIR_DEBUG("Failed to read port_state for host%d", host); + continue; + } + + /* Skip the not online FC host */ + if (!STREQ(state, PORT_STATE_ONLINE)) { + VIR_FREE(state); + continue; + } + VIR_FREE(state); + + if (virReadFCHost(NULL, host, "max_npiv_vports",&max_vports)< 0) { + VIR_DEBUG("Failed to read max_npiv_vports for host%d", host);
VIR_FREE(max_vports);
No need. As it's NULL as long as the return value of virReadFCHost < 0.
+ continue; + } + + if (virReadFCHost(NULL, host, "npiv_vports_inuse",&vports)< 0) { + VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); + VIR_FREE(max_vports); VIR_FREE(vports);
Likewise.
+ continue; + }
So do we document somewhere that the FC Host must have these two attributes defined for us to consider using them?
Not clear about your meaning, but the two sysfs entries are only for HBA. Not vHBA. A FC Host can be either a HBA or vHBA. And we have debug message as long as they are not supported by a FC HOST.
+ + if ((strlen(max_vports)> strlen(vports)) || + ((strlen(max_vports) == strlen(vports))&&
Why not just one ">="
Okay. That's a bad fault.
+ strcmp(max_vports, vports)> 0)) { + ret = strdup(entry->d_name); + goto cleanup; + }
What is being tested? The name or the value? I didn't go back to look at what virReadFCHost() provides, but I do see there is a patch:
https://www.redhat.com/archives/libvir-list/2013-January/msg00947.html
This returns the sysfs entry's value as string.
that will convert the string to the number and compare using that.
So no number here.
There's possible some code reuse that could make this and that patch a whole lot easier.
So I compare the string directly above, to avoid convering the strings to numbers first.
+ + VIR_FREE(max_vports); + VIR_FREE(vports); + } + +cleanup: + closedir(dir); + VIR_FREE(max_vports); + VIR_FREE(vports); + return ret; +} #else int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, @@ -3600,4 +3676,11 @@ virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, return NULL; }
+char * +virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + #endif /* __linux__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 78b50a8..3a68134 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -320,4 +320,6 @@ char * virGetFCHostNameByWWN(const char *sysfs_prefix, const char *wwpn) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+char * virFindFCHostCapableVport(const char *sysfs_prefix ); + #endif /* __VIR_UTIL_H__ */
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 02/06/2013 09:34 AM, Osier Yang wrote:
On 2013年02月06日 05:44, John Ferlan wrote:
On 02/04/2013 08:32 AM, Osier Yang wrote:
This finds the parent for vHBA by iterating over all the HBA which supports vport_ops capability on the host, and return the first one which is online, not saturated (vports in use is less than max_vports). --- docs/formatstorage.html.in | 3 +- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 10 +++-- src/util/virutil.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 + 5 files changed, 93 insertions(+), 6 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index af42fed..7315865 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -99,8 +99,7 @@ <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> + the (v)HBA.<span class="since">Since 0.6.2</span></dd>
The silent scream :-)
<dt><code>host</code></dt> <dd>Provides the source for pools backed by storage from a remote server. Will be used in combination with a<code>directory</code> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7334e15..759d630 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1841,6 +1841,7 @@ virFileStripSuffix; virFileUnlock; virFileWaitForDevices; virFileWriteStr; +virFindFCHostCapableVport; virFindFileInPath; virFormatIntDecimal; virGetDeviceID; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a9b96a3..59abeb0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -646,10 +646,12 @@ createVport(virStoragePoolSourceAdapter adapter) adapter.data.fchost.wwpn)) return 0;
- if (!adapter.data.fchost.parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA must be specified")); - return -1; + if (!adapter.data.fchost.parent&& + !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + return -1; }
if ((parent_host = getHostNumber(adapter.data.fchost.parent))< 0) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3073337..b9a6166 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3555,6 +3555,82 @@ cleanup: VIR_FREE(wwpn_buf); return ret; } + +# define PORT_STATE_ONLINE "Online" + +/* virFindFCHostCapableVport: + * + * Iterate over the sysfs and find out the first online HBA which + * supports vport, and not saturated,. + */ +char * +virFindFCHostCapableVport(const char *sysfs_prefix) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; + DIR *dir = NULL; + struct dirent *entry = NULL; + char *max_vports = NULL; + char *vports = NULL; + char *state = NULL; + char *ret = NULL; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + return NULL; + } + + while ((entry = readdir(dir))) { + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + int host;
uint32_t? To be consistent with other comments.
+ + ignore_value(sscanf(entry->d_name, "host%d",&host));
Why ignore_value()? If == -1, then host is undefined - could be something that results in the following being successful..
I don't think it's possible to return -1, as all the entries under SYSFS_FC_HOST_PATH should be "hostN". Entry "." and ".." are already skipped.
sscanf can return -1 for other errors (ENOMEM included) - I would think it's safer to check and fail than assume anything.
+ if (!virIsCapableVport(NULL, host)) + continue; + + if (virReadFCHost(NULL, host, "port_state",&state)< 0) { + VIR_DEBUG("Failed to read port_state for host%d", host); + continue; + } + + /* Skip the not online FC host */ + if (!STREQ(state, PORT_STATE_ONLINE)) { + VIR_FREE(state); + continue; + } + VIR_FREE(state); + + if (virReadFCHost(NULL, host, "max_npiv_vports",&max_vports)< 0) { + VIR_DEBUG("Failed to read max_npiv_vports for host%d", host);
VIR_FREE(max_vports);
No need. As it's NULL as long as the return value of virReadFCHost < 0.
+ continue; + } + + if (virReadFCHost(NULL, host, "npiv_vports_inuse",&vports)< 0) { + VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); + VIR_FREE(max_vports); VIR_FREE(vports);
Likewise.
+ continue; + }
So do we document somewhere that the FC Host must have these two attributes defined for us to consider using them?
Not clear about your meaning, but the two sysfs entries are only for HBA. Not vHBA. A FC Host can be either a HBA or vHBA. And we have debug message as long as they are not supported by a FC HOST.
The point is you're failing silently using 'continue;'. I suppose the VIR_DEBUG() messages satisfy the need to perhaps determine why the FC Host "someone" sets up isn't found. Although that assumes they are debugging what's wrong with their config. Also I'm OK that in the 1st condition the VIR_FREE(max_vports) is not required, it will be in the second condition though.
+ + if ((strlen(max_vports)> strlen(vports)) || + ((strlen(max_vports) == strlen(vports))&&
Why not just one ">="
Okay. That's a bad fault.
+ strcmp(max_vports, vports)> 0)) { + ret = strdup(entry->d_name); + goto cleanup; + }
What is being tested? The name or the value? I didn't go back to look at what virReadFCHost() provides, but I do see there is a patch:
https://www.redhat.com/archives/libvir-list/2013-January/msg00947.html
This returns the sysfs entry's value as string.
that will convert the string to the number and compare using that.
So no number here.
There's possible some code reuse that could make this and that patch a whole lot easier.
So I compare the string directly above, to avoid convering the strings to numbers first.
+ + VIR_FREE(max_vports); + VIR_FREE(vports); + } + +cleanup: + closedir(dir); + VIR_FREE(max_vports); + VIR_FREE(vports); + return ret; +} #else int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, @@ -3600,4 +3676,11 @@ virGetFCHostNameByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, return NULL; }
+char * +virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + #endif /* __linux__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 78b50a8..3a68134 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -320,4 +320,6 @@ char * virGetFCHostNameByWWN(const char *sysfs_prefix, const char *wwpn) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+char * virFindFCHostCapableVport(const char *sysfs_prefix ); + #endif /* __VIR_UTIL_H__ */
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 02/06/2013 11:32 AM, John Ferlan wrote:
+ + ignore_value(sscanf(entry->d_name, "host%d",&host));
Why ignore_value()? If == -1, then host is undefined - could be something that results in the following being successful..
I don't think it's possible to return -1, as all the entries under SYSFS_FC_HOST_PATH should be "hostN". Entry "." and ".." are already skipped.
sscanf can return -1 for other errors (ENOMEM included) - I would think it's safer to check and fail than assume anything.
sscanf should NEVER be used to parse raw "%d". POSIX says that the result is undefined on integer overflow (sscanf is not required to fail in that case, yet you are not guaranteed if you got MAX_INT, wraparound modulo 2**32, or some other weird behavior). Something like sscanf("%5d") for parsing at most five digits is slightly more tolerable because it prevents you from getting to the overflow situation, although I still think that using sscanf to parse integers is dangerous. And even in the cases where using sscanf is safe (fixed-length parsing not subject to integer overflow), you should NEVER ignore failure, and you should always end with a %n to ensure that you parsed as much as you were expecting to parse. sscanf() is so difficult to correctly use directly that I recommend that we avoid adding any new uses of it into libvirt (I'd like to turn on the 'make syntax-check' rule that forbids *scanf entirely, but we'd have to first convert our existing uses into alternative code). I'd much rather see virStrToLong_i() used here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Forgot to say this is only rebasing, no changes since v1. On 2013年02月04日 21:31, Osier Yang wrote:
This is the 3rd part to implement NPIV migration support [1].
Part 1: (New version) https://www.redhat.com/archives/libvir-list/2013-February/msg00112.html
Part 2: (Already ACKed by Michal) https://www.redhat.com/archives/libvir-list/2013-January/msg00859.html
Part 4: (Partly ACKed by John) https://www.redhat.com/archives/libvir-list/2013-January/msg02113.html
The patches are based on Part 2.
The new XMLs might be too long, might be deserved to have them as sub-elements of<adapter>. But I'd like to see the feedback first.
Osier Yang (8): New XML attributes for storage pool source adapter storage: Make the adapter name be consistent with node device driver storage: Move virStorageBackendSCSIGetHostNumber into iscsi backend phyp: Prohibit fc_host adapter for phyp driver util: Add helper to get the scsi host name by iterating over sysfs util: Fix bug of managing vport storage: Add startPool and stopPool for scsi backend storage: Guess the parent if it's not specified for vHBA
docs/formatstorage.html.in | 15 ++- docs/schemas/storagepool.rng | 33 +++++- src/conf/storage_conf.c | 123 ++++++++++++++++-- src/conf/storage_conf.h | 23 +++- src/libvirt_private.syms | 4 + src/phyp/phyp_driver.c | 15 ++- src/storage/storage_backend_iscsi.c | 39 ++++++- src/storage/storage_backend_scsi.c | 190 +++++++++++++++++++-------- src/storage/storage_backend_scsi.h | 3 - src/util/virutil.c | 196 +++++++++++++++++++++++++++- src/util/virutil.h | 7 + tests/storagepoolxml2xmlin/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlin/pool-scsi1.xml | 15 ++ tests/storagepoolxml2xmlout/pool-scsi.xml | 2 +- tests/storagepoolxml2xmlout/pool-scsi1.xml | 18 +++ tests/storagepoolxml2xmltest.c | 1 + 16 files changed, 602 insertions(+), 84 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi1.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi1.xml
[1] https://www.redhat.com/archives/libvir-list/2012-November/msg00826.html
Regards, Osier
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Eric Blake
-
John Ferlan
-
Osier Yang