[libvirt] [PATCH 0/8] storage_scsi: Stable SCSI host addressing
Just over a year ago, Osier Yang submitted some patches to provide stable SCSI host addressing support: http://www.redhat.com/archives/libvir-list/2013-June/msg00396.html Although reviewed - none of the patches were ever pushed and Osier never got back to the work. I eventually inherited the changes and now after languishing in the todo list - I took the time to rework things. There is a bz associated: https://bugzilla.redhat.com/show_bug.cgi?id=963817 Prior to leaving Red Hat, Osier and I spent some time revisiting the patch series and hashing out new/different ideas to come up with the same answer. My focus was to avoid the "generic recursive directory search" (patch 3), to not require having to know whether the scsi_host was using udev or hal, and to simplify as much as possible using existing data/information. The generic directory search I believe was less generic than intended and over complicated things. The old patches essentially used the output of a 'virsh nodedev-list scsi_host' and then selecting a resulting entry ran a 'virsh nodedev-dumpxml' to grab/use the resulting <parent> value. Taking that value and optionally adding a unique_id value was the basis for the design that would generate a PCI address (either in udev or hal format) and recursively search through /sys/devices looking for a matching address in either udev or hal format. These patches replace the bulk of the directory traversal logic with a more direct (and already in use) approach to scan the /sys/class/scsi_host directories looking for a matching PCI address found in the symlink of the files in the directory. The changed logic will add a new XML element 'parentaddr' to describe the scsi_host by it's PCI address. Additionally, the 'unique_id' has become a required attribute. The code reuses the virDevicePCIAddressParseXML() in order parse the required 'address' element. The 'address' will be in the expect PCI Address format like other described host devices. In order to help view the required unique_id value, the nodedev-dumpxml output has been adjusted to provide the <unique_id> value. This value will be an optional value on input. The documentation is updated to describe how to generate the address from the nodedev-dumpxml output. The new scsihosttest creates the expected PCI infrastructure on the fly since adding files with colons (':') is prohibited. There are multiple directories and hosts within each to ensure the search logic worked as expected. Reviewer notes: Patch 1 is new - it's just forcing the specific adapter.type checking Patches 2 & 3 are the former patches 1 & 2 with some edits to adjust for new XML syntax (and changed associated structure) Patch 4 is new - it's just utilizing the 'LINUX_SYSFS_SCSI_HOST_PREFIX' definition rather than using the hardcoded value Patches 5 & 6 are new. They handle the optional unique_id value (including using the new virNodeDevCapsDefParseIntOptional()) Patch 7 is similar to the former patch 5 insomuch as it's where the comparison of the PCI directory path and unique_id file is made. Patch 8 is similar to the former patch 7 insomuch as it's where the link is made between the scsi_host host# and the loading/refreshing of the scsi_host adapter. Former patches 3, 4, 6, and 8-11 are no longer used Please check my XML schema (patch 3) - I think I figured out the right syntax to use regarding using either "<name>" or "<parentaddr>" where "<parentaddr>" is an element. The syntax passes the make check, but it's not an area of expertise for me. Using parentaddr was preferred over overloading the parent attribute. John Ferlan (6): getAdapterName: check for SCSI_HOST scsi_backend: Use existing LINUX_SYSFS_SCSI_HOST_PREFIX definition virutil: Introduce virReadSCSIUniqueId Add unique_id to nodedev output scsi_host: Introduce virFindSCSIHostByPCI getAdapterName: Lookup stable scsi_host Osier Yang (2): virStoragePoolSourceAdapter: Refine the SCSI_HOST adapter name storage: Introduce parentaddr into virStoragePoolSourceAdapter docs/formatnode.html.in | 11 + docs/formatstorage.html.in | 130 +++++++-- docs/schemas/basictypes.rng | 24 +- docs/schemas/nodedev.rng | 6 + src/conf/node_device_conf.c | 23 +- src/conf/node_device_conf.h | 1 + src/conf/storage_conf.c | 111 +++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 2 + src/node_device/node_device_linux_sysfs.c | 6 + src/phyp/phyp_driver.c | 8 +- src/storage/storage_backend_scsi.c | 53 +++- src/test/test_driver.c | 5 +- src/util/virutil.c | 154 +++++++++++ src/util/virutil.h | 8 + tests/Makefile.am | 7 + .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 + tests/nodedevxml2xmltest.c | 1 + tests/scsihosttest.c | 308 +++++++++++++++++++++ .../pool-scsi-type-scsi-host-stable.xml | 19 ++ .../pool-scsi-type-scsi-host-stable.xml | 22 ++ tests/storagepoolxml2xmltest.c | 1 + 22 files changed, 855 insertions(+), 61 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml create mode 100644 tests/scsihosttest.c create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml -- 1.9.3
Rather than assume that NOT FC_HOST is SCSI_HOST, let's call them out specifically. Makes it easier to find SCSI_HOST code/structs and ensures something isn't missed in the future Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index f6f3ca2..21c13e5 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -547,18 +547,17 @@ getAdapterName(virStoragePoolSourceAdapter adapter) { char *name = NULL; - if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { ignore_value(VIR_STRDUP(name, adapter.data.name)); - return 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); + } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + 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 name; -- 1.9.3
From: Osier Yang <jyang@redhat.com> Preparation for future patches by creating a scsi_host union. For now, just the 'name' will be present. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 15 ++++++++------- src/conf/storage_conf.h | 4 +++- src/phyp/phyp_driver.c | 8 ++++---- src/storage/storage_backend_scsi.c | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b6fd79..2e71c64 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -343,7 +343,7 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) VIR_FREE(adapter.data.fchost.parent); } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - VIR_FREE(adapter.data.name); + VIR_FREE(adapter.data.scsi_host.name); } } @@ -674,7 +674,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virXPathString("string(./adapter/@wwpn)", ctxt); } else if (source->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - source->adapter.data.name = + source->adapter.data.scsi_host.name = virXPathString("string(./adapter/@name)", ctxt); } } else { @@ -699,7 +699,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, /* To keep back-compat, 'type' is not required to specify * for scsi_host adapter. */ - if ((source->adapter.data.name = + if ((source->adapter.data.scsi_host.name = virXPathString("string(./adapter/@name)", ctxt))) source->adapter.type = VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; @@ -942,7 +942,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } else if (ret->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (!ret->source.adapter.data.name) { + if (!ret->source.adapter.data.scsi_host.name) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source adapter name")); goto error; @@ -1110,7 +1110,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, src->adapter.data.fchost.wwpn); } else if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - virBufferAsprintf(buf, " name='%s'/>\n", src->adapter.data.name); + virBufferAsprintf(buf, " name='%s'/>\n", + src->adapter.data.scsi_host.name); } } @@ -2128,8 +2129,8 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, 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)) + if (STREQ(pool->def->source.adapter.data.scsi_host.name, + def->source.adapter.data.scsi_host.name)) matchpool = pool; } break; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 04d99eb..7a92a47 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -211,7 +211,9 @@ struct _virStoragePoolSourceAdapter { int type; /* virStoragePoolSourceAdapterType */ union { - char *name; + struct { + char *name; + } scsi_host; struct { char *parent; char *wwnn; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 508e4c0..d5147d1 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1981,7 +1981,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, spdef->source.ndevice = 1; /*XXX source adapter not working properly, should show hdiskX */ - if ((spdef->source.adapter.data.name = + if ((spdef->source.adapter.data.scsi_host.name = phypGetStoragePoolDevice(pool->conn, pool->name)) == NULL) { VIR_ERROR(_("Unable to determine storage pools's source adapter.")); goto err; @@ -2203,7 +2203,7 @@ phypStorageVolGetXMLDesc(virStorageVolPtr vol, unsigned int flags) pool.source.ndevice = 1; - if ((pool.source.adapter.data.name = + if ((pool.source.adapter.data.scsi_host.name = phypGetStoragePoolDevice(sp->conn, sp->name)) == NULL) { VIR_ERROR(_("Unable to determine storage sps's source adapter.")); goto cleanup; @@ -2442,7 +2442,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) managed_system, vios_id); virBufferAsprintf(&buf, "mksp -f %schild %s", def->name, - source.adapter.data.name); + source.adapter.data.scsi_host.name); if (system_type == HMC) virBufferAddChar(&buf, '\''); @@ -2673,7 +2673,7 @@ phypStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags) def.source.ndevice = 1; /*XXX source adapter not working properly, should show hdiskX */ - if ((def.source.adapter.data.name = + if ((def.source.adapter.data.scsi_host.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 21c13e5..b5dbe51 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -548,7 +548,7 @@ getAdapterName(virStoragePoolSourceAdapter adapter) char *name = NULL; if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - ignore_value(VIR_STRDUP(name, adapter.data.name)); + ignore_value(VIR_STRDUP(name, adapter.data.scsi_host.name)); } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, -- 1.9.3
From: Osier Yang <jyang@redhat.com> Between reboots and kernel reloads, the SCSI host number used for SCSI storage pools may change requiring modification to the storage pool XML in order to use a specific SCSI host adapter. This patch introduces the "parentaddr" element and "unique_id" attribute for the SCSI host adapter in order to uniquely identify the adapter between reboots and kernel reloads. For now the goal is to only parse and format the XML. Both will be required to be provided in order to uniquely identify the desired SCSI host. The new XML is expected to be as follows: <adapter type='scsi_host'> <parentaddr unique_id='3'> <address domain='0x0000' bus='0x00' slot='0x1f' func='0x2'/> </parentaddr> </adapter> where "parentaddr" is the parent device of the SCSI host using the PCI address on which the device resides and the value from the unique_id file for the device. Both the PCI address and unique_id values will be used to traverse the /sys/class/scsi_host/ directories looking at each link to match the PCI address reformatted to the directory link format where "domain:bus:slot:function" is found. Then for each matching directory the unique_id file for the scsi_host will be used to match the unique_id value in the xml. For a PCI address listed above, this will be formatted to "0000:00:1f.2" and the links in /sys/class/scsi_host will be used to find the host# to be used for the 'scsi_host' device. Each entry is a link to the /sys/bus/pci/devices directories, e.g.: % ls -al /sys/class/scsi_host/host2 lrwxrwxrwx. 1 root root 0 Jun 1 00:22 /sys/class/scsi_host/host2 -> ../../devices/pci0000:00/0000:00:1f.2/ata3/host2/scsi_host/host2 % cat /sys/class/scsi_host/host2/unique_id 3 The "parentaddr" and "name" attributes are mutually exclusive to identify the SCSI host number. Use of the "parentaddr" element will be the preferred mechanism. This patch only supports to parse and format the XMLs. Later patches will add code to find out the scsi host number. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 128 ++++++++++++++++++--- docs/schemas/basictypes.rng | 24 +++- src/conf/storage_conf.c | 106 +++++++++++++++-- src/conf/storage_conf.h | 4 + .../pool-scsi-type-scsi-host-stable.xml | 19 +++ .../pool-scsi-type-scsi-host-stable.xml | 22 ++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 273 insertions(+), 31 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 1cd82b4..0e43fd8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -89,6 +89,24 @@ <pre> ... <source> + <adapter type='scsi_host' name='scsi_host1'/> + </source> + ...</pre> + + <pre> + ... + <source> + <adapter type='scsi_host'> + <parentaddr unique_id='1'> + <address domain='0x0000' bus='0x00' slot='0x1f' addr='0x2'/> + </parentaddr> + </adapter> + </source> + ...</pre> + + <pre> + ... + <source> <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> </source> ...</pre> @@ -111,25 +129,97 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters (pool - type <code>scsi</code>). May - only occur once. Attribute <code>name</code> is the SCSI adapter - name (ex. "scsi_host1". NB, although a name such as "host1" is - still supported for backwards compatibility, it is not recommended). - Attribute <code>type</code> (<span class="since">1.0.5</span>) - specifies the adapter type. Valid values are "fc_host" and "scsi_host". - If omitted and the <code>name</code> attribute is specified, then it - defaults to "scsi_host". To keep backwards compatibility, the attribute - <code>type</code> is optional for the "scsi_host" adapter, but - mandatory for the "fc_host" adapter. Attributes <code>wwnn</code> - (Word Wide Node Name) and <code>wwpn</code> (Word Wide Port Name) - (<span class="since">1.0.4</span>) are used by the "fc_host" adapter - to uniquely identify the device in the Fibre Channel storage fabric - (the device can be either a HBA or vHBA). Both wwnn and wwpn should - be specified (See command 'virsh nodedev-dumpxml' to known how to get - wwnn/wwpn of a (v)HBA). The optional attribute <code>parent</code> - (<span class="since">1.0.4</span>) specifies the parent device for - the "fc_host" adapter. - <span class="since">Since 0.6.2</span></dd> + type <code>scsi</code>). May only occur once. + <dl> + <dt><code>name</code></dt> + <dd>The SCSI adapter name (e.g. "scsi_host1", although a name + such as "host1" is still supported for backwards compatibility, + it is not recommended). It is further recommended to utilize + the <code>parentaddr</code> element since the <code>name</code> + may change between system reboots and kernel reloads if the + hardware configuration is modified. + <span class="since">Since 0.6.2</span> + </dd> + </dl> + <dl> + <dt><code>type</code></dt> + <dd>Specifies the adapter type. Valid values are "scsi_host" or + "fc_host". If omitted and the <code>name</code> attribute is + specified, then it defaults to "scsi_host". To keep backwards + compatibility, this attribute is optional <b>only</b> for the + "scsi_host" adapter, but is mandatory for the "fc_host" adapter. + <span class="since">Since 1.0.5</span> + </dd> + </dl> + <dl> + <dt><code>wwwn</code> and <code>wwpn</code></dt> + <dd>The "World Wide Node Name" (<code>wwnn</code>) and "World Wide + Port Name" (<code>wwpn</code>) are used by the "fc_host" adapter + to uniquely identify the device in the Fibre Channel storage fabric + (the device can be either a HBA or vHBA). Both wwnn and wwpn should + be specified. Use the command 'virsh nodedev-dumpxml' to determine + how to set the values for the wwnn/wwpn of a (v)HBA. + <span class="since">Since 1.0.4</span> + </dd> + </dl> + <dl> + <dt><code>parent</code></dt> + <dd>Used by the "fc_host" adapter type to optionally specify the + parent scsi_host device defined in the + <a href="formatnode.html">Node Device</a> database as the + <a href="http://wiki.libvirt.org/page/NPIV_in_libvirt">NPIV</a> + virtual Host Bus Adapter (vHBA). + <span class="since">Since 1.0.4</span> + </dd> + </dl> + <dl> + <dt><code>parentaddr</code></dt> + <dd>Used by the "scsi_host" adapter type instead of the + <code>name</code> attribute to uniquely identify the SCSI host + adapter's parent PCI address. Using a combination of the + <code>unique_id</code> attribute and the <code>address</code> + element, a search will be peformed of the system filesystem for + a match. + <span class="since">Since 1.2.6</span> + <dl> + <dt><code>address</code></dt> + <dd>The PCI address of the scsi_host device to be used. Using + a PCI address provides consistent naming across system reboots + and kernel reloads. The address will have four attributes: + <code>domain</code> (a 2-byte hex integer, not currently used + by qemu), <code>bus</code> (a hex value between 0 and 0xff, + inclusive), <code>slot</code> (a hex value between 0x0 and + 0x1f, inclusive), and <code>function</code> (a value between + 0 and 7, inclusive). The PCI address can be determined by + listing the <code>/sys/bus/pci/devices</code> and the + <code>/sys/class/scsi_host</code> directories in order to + find the expected scsi_host device. The address will be + provided in a format such as "0000:00:1f:2" which can be + used to generate the expected PCI address + "domain='0x0000' bus='0x00' slot='0x1f' function='0x0'". + Optionally, using the combination of the commands 'virsh + nodedev-list scsi_host' and 'virsh nodedev-dumpxml' for a + specific list entry and converting the resulting + <code>path</code> element as the basis to formulate the + correctly formatted PCI address. + </dd> + </dl> + <dl> + <dt><code>unique_id</code></dt> + <dd>Required <code>parentaddr</code> attribute used to determine + which of the scsi_host adapters for the provided PCI address + should be used. The value is determine by contents of the + <code>unique_id</code> file for the specific scsi_host adapter. + For a PCI address of "0000:00:1f:2", the unique identifer files + can be found using the commands: <code> + find -H /sys/bus/pci/devices/0000:00:1f.2 -name unique_id | + xargs grep '[0-9]'</code> or <code> + grep '[0-9]' /sys/class/scsi_host/host{0..9}/unique_id</code>. + </dd> + </dl> + </dd> + </dl> + </dd> <dt><code>host</code></dt> <dd>Provides the source for pools backed by storage from a remote server (pool types <code>netfs</code>, <code>iscsi</code>, diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 34ef613..29eece3 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -355,9 +355,27 @@ <value>scsi_host</value> </attribute> </optional> - <attribute name='name'> - <text/> - </attribute> + <choice> + <group> + <attribute name='name'> + <text/> + </attribute> + </group> + <group> + <interleave> + <element name="parentaddr"> + <optional> + <attribute name='unique_id'> + <ref name='positiveInteger'/> + </attribute> + </optional> + <element name="address"> + <ref name="pciaddress"/> + </element> + </element> + </interleave> + </group> + </choice> </group> <group> <attribute name='type'> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2e71c64..31648cb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -674,14 +674,43 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virXPathString("string(./adapter/@wwpn)", ctxt); } else if (source->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + source->adapter.data.scsi_host.name = virXPathString("string(./adapter/@name)", ctxt); + if (virXPathNode("./adapter/parentaddr", ctxt)) { + xmlNodePtr addrnode = virXPathNode("./adapter/parentaddr/address", + ctxt); + virDevicePCIAddressPtr addr = + &source->adapter.data.scsi_host.parentaddr; + + if (!addrnode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing scsi_host PCI address element")); + goto cleanup; + } + source->adapter.data.scsi_host.has_parent = true; + if (virDevicePCIAddressParseXML(addrnode, addr) < 0) + goto cleanup; + if ((virXPathInt("string(./adapter/parentaddr/@unique_id)", + ctxt, + &source->adapter.data.scsi_host.unique_id) < 0) || + (source->adapter.data.scsi_host.unique_id < 0)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing or invalid scsi adapter " + "'unique_id' value")); + goto cleanup; + } + } } } else { char *wwnn = NULL; char *wwpn = NULL; char *parent = NULL; + /* "type" was not specified in the XML, so we must verify that + * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the + * XML. If any are found, then we cannot just use "name" alone". + */ wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); parent = virXPathString("string(./adapter/@parent)", ctxt); @@ -692,7 +721,14 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_FREE(parent); virReportError(VIR_ERR_XML_ERROR, "%s", _("Use of 'wwnn', 'wwpn', and 'parent' attributes " - "requires the 'fc_host' adapter 'type'")); + "requires use of the adapter 'type'")); + goto cleanup; + } + + if (virXPathNode("./adapter/parentaddr", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Use of 'parent' element requires use " + "of the adapter 'type'")); goto cleanup; } @@ -942,9 +978,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } else if (ret->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (!ret->source.adapter.data.scsi_host.name) { + if (!ret->source.adapter.data.scsi_host.name && + !ret->source.adapter.data.scsi_host.has_parent) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage pool source adapter name")); + _("Either 'name' or 'parent' must be specified " + "for the 'scsi_host' adapter")); + goto error; + } + + if (ret->source.adapter.data.scsi_host.name && + ret->source.adapter.data.scsi_host.has_parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Both 'name' and 'parent' cannot be specified " + "for the 'scsi_host' adapter")); goto error; } } @@ -1109,9 +1155,24 @@ virStoragePoolSourceFormat(virBufferPtr buf, 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.scsi_host.name); + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + if (src->adapter.data.scsi_host.name) { + virBufferAsprintf(buf, " name='%s'/>\n", + src->adapter.data.scsi_host.name); + } else { + virDevicePCIAddress addr; + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n", + src->adapter.data.scsi_host.unique_id); + virBufferAdjustIndent(buf, 2); + addr = src->adapter.data.scsi_host.parentaddr; + ignore_value(virDevicePCIAddressFormat(buf, addr, false)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</parentaddr>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</adapter>\n"); + } } } @@ -2087,6 +2148,28 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, return ret; } +static bool +matchSCSIAdapterParent(virStoragePoolObjPtr pool, + virStoragePoolDefPtr def) +{ + virDevicePCIAddressPtr pooladdr = + &pool->def->source.adapter.data.scsi_host.parentaddr; + virDevicePCIAddressPtr defaddr = + &def->source.adapter.data.scsi_host.parentaddr; + int pool_unique_id = + pool->def->source.adapter.data.scsi_host.unique_id; + int def_unique_id = + def->source.adapter.data.scsi_host.unique_id; + if (pooladdr->domain == defaddr->domain && + pooladdr->bus == defaddr->bus && + pooladdr->slot == defaddr->slot && + pooladdr->function == defaddr->function && + pool_unique_id == def_unique_id) { + return true; + } + return false; +} + int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def) @@ -2129,9 +2212,14 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, matchpool = pool; } else if (pool->def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST){ - if (STREQ(pool->def->source.adapter.data.scsi_host.name, - def->source.adapter.data.scsi_host.name)) - matchpool = pool; + if (pool->def->source.adapter.data.scsi_host.name) { + if (STREQ(pool->def->source.adapter.data.scsi_host.name, + def->source.adapter.data.scsi_host.name)) + matchpool = pool; + } else { + if (matchSCSIAdapterParent(pool, def)) + matchpool = pool; + } } break; case VIR_STORAGE_POOL_ISCSI: diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 7a92a47..4659231 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -29,6 +29,7 @@ # include "virstoragefile.h" # include "virbitmap.h" # include "virthread.h" +# include "device_conf.h" # include <libxml/tree.h> @@ -213,6 +214,9 @@ struct _virStoragePoolSourceAdapter { union { struct { char *name; + virDevicePCIAddress parentaddr; /* host address */ + int unique_id; + bool has_parent; } scsi_host; struct { char *parent; diff --git a/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml new file mode 100644 index 0000000..db13cd0 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml @@ -0,0 +1,19 @@ +<pool type="scsi"> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <adapter type='scsi_host'> + <parentaddr unique_id='5'> + <address domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </parentaddr> + </adapter> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml new file mode 100644 index 0000000..dd3d87d --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml @@ -0,0 +1,22 @@ +<pool type='scsi'> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <adapter type='scsi_host'> + <parentaddr unique_id='5'> + <address domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </parentaddr> + </adapter> + </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 971fe3b..d7ae10b 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -104,6 +104,7 @@ mymain(void) DO_TEST("pool-sheepdog"); DO_TEST("pool-gluster"); DO_TEST("pool-gluster-sub"); + DO_TEST("pool-scsi-type-scsi-host-stable"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.3
With the goal to make it clearer that the combination of PCI address and unique_id value is the intention behind this set of patches, I've adjusted the text of formatstorage.html.in by squashing the following in: diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 0e43fd8..eefa314 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -134,11 +134,18 @@ <dt><code>name</code></dt> <dd>The SCSI adapter name (e.g. "scsi_host1", although a name such as "host1" is still supported for backwards compatibility, - it is not recommended). It is further recommended to utilize - the <code>parentaddr</code> element since the <code>name</code> - may change between system reboots and kernel reloads if the - hardware configuration is modified. - <span class="since">Since 0.6.2</span> + it is not recommended). The scsi_host name to be used can be + determined from the output of a <code>virsh nodedev-list + scsi_host</code> command followed by a combination of + <code>lspci</code> and <code>virsh nodedev-dumpxml + scsi_hostN</code> commands to find the <code>scsi_hostN</code> + to be used. <span class="since">Since 0.6.2</span> + <p> + It is further recommended to utilize the + <code>parentaddr</code> element since it's possible to have + the path to which the scsi_hostN uses change between system + reboots. <span class="since">Since 1.2.6</span> + </p> </dd> </dl> <dl> @@ -175,11 +182,17 @@ <dl> <dt><code>parentaddr</code></dt> <dd>Used by the "scsi_host" adapter type instead of the - <code>name</code> attribute to uniquely identify the SCSI host - adapter's parent PCI address. Using a combination of the - <code>unique_id</code> attribute and the <code>address</code> - element, a search will be peformed of the system filesystem for - a match. + <code>name</code> attribute to more uniquely identify the + SCSI host. Using a combination of the <code>unique_id</code> + attribute and the <code>address</code> element to formulate + a PCI address, a search will be performed of the + <code>/sys/class/scsi_host/hostNN</code> links for a + matching PCI address with a matching <code>unique_id</code> + value in the <code>/sys/class/scsi_host/hostNN/unique_id</code> + file. The value in the "unique_id" file will be unique enough + for the specific PCI address. The <code>hostNN</code> will be + used by libvirt as the basis to define which SCSI host is to + be used for the currently booted system. <span class="since">Since 1.2.6</span> <dl> <dt><code>address</code></dt> @@ -211,10 +224,9 @@ should be used. The value is determine by contents of the <code>unique_id</code> file for the specific scsi_host adapter. For a PCI address of "0000:00:1f:2", the unique identifer files - can be found using the commands: <code> - find -H /sys/bus/pci/devices/0000:00:1f.2 -name unique_id | - xargs grep '[0-9]'</code> or <code> - grep '[0-9]' /sys/class/scsi_host/host{0..9}/unique_id</code>. + can be found using the command + <code>find -H /sys/class/scsi_host/host*/unique_id | + xargs grep '[0-9]'</code> </dd> </dl> </dd>
Rather than supplying the path again in the formatting of the sysfs scsi_host directory. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b5dbe51..e42778f 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -475,7 +475,8 @@ virStorageBackendSCSITriggerRescan(uint32_t host) VIR_DEBUG("Triggering rescan of host %d", host); - if (virAsprintf(&path, "/sys/class/scsi_host/host%u/scan", host) < 0) { + if (virAsprintf(&path, "%s/host%u/scan", + LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) { retval = -1; goto out; } @@ -663,7 +664,8 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (getHostNumber(name, &host) < 0) goto cleanup; - if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) + if (virAsprintf(&path, "%s/host%d", + LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) goto cleanup; *isActive = virFileExists(path); -- 1.9.3
Introduce a new function to read the current scsi_host entry and return the value found in the 'unique_id' file. Add a 'scsihosttest' test (similar to the fchosttest, but incorporating some of the concepts of the mocked pci test library) in order to read the unique_id file like would be found in the /sys/class/scsi_host tree. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 53 ++++++++++ src/util/virutil.h | 4 + tests/Makefile.am | 7 ++ tests/scsihosttest.c | 254 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 319 insertions(+) create mode 100644 tests/scsihosttest.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d1d6ff3..a51e456 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2079,6 +2079,7 @@ virParseOwnershipIds; virParseVersionString; virPipeReadUntilEOF; virReadFCHost; +virReadSCSIUniqueId; virScaleInteger; virSetBlocking; virSetCloseExec; diff --git a/src/util/virutil.c b/src/util/virutil.c index 95d1ff9..c73ce06 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1681,6 +1681,50 @@ virGetDeviceUnprivSGIO(const char *path, # define SYSFS_FC_HOST_PATH "/sys/class/fc_host/" # define SYSFS_SCSI_HOST_PATH "/sys/class/scsi_host/" +/* virReadSCSIUniqueId: + * @sysfs_prefix: "scsi_host" sysfs path, defaults to SYSFS_SCSI_HOST_PATH + * @host: Host number, E.g. 5 of "scsi_host/host5" + * @result: Return the entry value as an unsigned int + * + * Read the value of the "scsi_host" unique_id file. + * + * Returns 0 on success, and @result is filled with the unique_id value + * Otherwise returns -1 + */ +int +virReadSCSIUniqueId(const char *sysfs_prefix, + int host, + int *result) +{ + char *sysfs_path = NULL; + char *p = NULL; + int ret = -1; + char *buf = NULL; + int unique_id; + + if (virAsprintf(&sysfs_path, "%s/host%d/unique_id", + sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH, + host) < 0) + goto cleanup; + + if (virFileReadAll(sysfs_path, 1024, &buf) < 0) + goto cleanup; + + if ((p = strchr(buf, '\n'))) + *p = '\0'; + + if (virStrToLong_i(buf, NULL, 10, &unique_id) < 0) + goto cleanup; + + *result = unique_id; + ret = 0; + + cleanup: + VIR_FREE(sysfs_path); + VIR_FREE(buf); + return ret; +} + /* virReadFCHost: * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH * @host: Host number, E.g. 5 of "fc_host/host5" @@ -2034,6 +2078,15 @@ virFindFCHostCapableVport(const char *sysfs_prefix) } #else int +virReadSCSIUniqueId(const char *sysfs_prefix ATTRIBUTE_UNUSED, + int host ATTRIBUTE_UNUSED, + unsigned int *result ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return -1; +} + +int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, int host ATTRIBUTE_UNUSED, const char *entry ATTRIBUTE_UNUSED, diff --git a/src/util/virutil.h b/src/util/virutil.h index 2bb74e2..1407dfd 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -164,6 +164,10 @@ int virGetDeviceUnprivSGIO(const char *path, int *unpriv_sgio); char *virGetUnprivSGIOSysfsPath(const char *path, const char *sysfs_dir); +int virReadSCSIUniqueId(const char *sysfs_prefix, + int host, + int *result) + ATTRIBUTE_NONNULL(3); int virReadFCHost(const char *sysfs_prefix, int host, const char *entry, diff --git a/tests/Makefile.am b/tests/Makefile.am index c999061..a15bd81 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -81,6 +81,7 @@ EXTRA_DIST = \ domainsnapshotxml2xmlin \ domainsnapshotxml2xmlout \ fchostdata \ + scsihostdata \ interfaceschemadata \ lxcconf2xmldata \ lxcxml2xmldata \ @@ -185,6 +186,7 @@ endif WITH_REMOTE if WITH_LINUX test_programs += fchosttest +test_programs += scsihosttest endif WITH_LINUX if WITH_LIBVIRTD @@ -1129,8 +1131,13 @@ fchosttest_SOURCES = \ fchosttest.c testutils.h testutils.c fchosttest_LDADD = $(LDADDS) +scsihosttest_SOURCES = \ + scsihosttest.c testutils.h testutils.c +scsihosttest_LDADD = $(LDADDS) + else ! WITH_LINUX EXTRA_DIST += fchosttest.c +EXTRA_DIST += scsihosttest.c endif ! WITH_LINUX if WITH_LINUX diff --git a/tests/scsihosttest.c b/tests/scsihosttest.c new file mode 100644 index 0000000..990fe80 --- /dev/null +++ b/tests/scsihosttest.c @@ -0,0 +1,254 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "testutils.h" + +#ifdef __linux__ + +# include <fcntl.h> +# include <sys/stat.h> +# include "virstring.h" +# include "virutil.h" +# include "virerror.h" +# include "virlog.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.scsihosttest"); + +char *scsihost_class_path; +# define TEST_SCSIHOST_CLASS_PATH scsihost_class_path + +/* + * Initialized/create a mock sysfs environment with 4 scsi_host devices + * located on "0000:00:1f.1" and "0000:00:1f.2". Each directory will + * contain 4 unique_id files having the same value. + * + * The environment is: + * + * 4 files: + * + * sys/devices/pci0000:00/0000:00:1f.1/ata1/host0/scsi_host/host0/unique_id + * sys/devices/pci0000:00/0000:00:1f.1/ata2/host1/scsi_host/host1/unique_id + * sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/scsi_host/host0/unique_id + * sys/devices/pci0000:00/0000:00:1f.2/ata2/host1/scsi_host/host1/unique_id + * + * 4 symlinks: + * + * sys/class/scsi_host/host0 -> link to 1f.1 host 0 + * sys/class/scsi_host/host1 -> link to 1f.1 host 1 + * sys/class/scsi_host/host2 -> link to 1f.2 host 0 + * sys/class/scsi_host/host3 -> link to 1f.2 host 1 + * + * The unique_id's for host0 and host2 are set to "1" + * The unique_id's for host1 and host3 are set to "2" + */ + +static int +create_scsihost(const char *fakesysfsdir, const char *devicepath, + const char *unique_id, const char *hostname) +{ + char *unique_id_path = NULL; + char *link_path = NULL; + char *spot; + int ret = -1; + int fd = -1; + + if (virAsprintfQuiet(&unique_id_path, "%s/devices/pci0000:00/%s/unique_id", + fakesysfsdir, devicepath) < 0 || + virAsprintfQuiet(&link_path, "%s/class/scsi_host/%s", + fakesysfsdir, hostname) < 0) { + fprintf(stderr, "Out of memory\n"); + goto cleanup; + } + + /* Rather than create path & file, temporarily snip off the file to + * create the path + */ + if (!(spot = strstr(unique_id_path, "unique_id"))) { + fprintf(stderr, "Did not find unique_id in path\n"); + goto cleanup; + } + spot--; + *spot = '\0'; + if (virFileMakePathWithMode(unique_id_path, 0755) < 0) { + fprintf(stderr, "Unable to make path to '%s'\n", unique_id_path); + goto cleanup; + } + *spot = '/'; + + /* Rather than create path & file, temporarily snip off the file to + * create the path + */ + if (!(spot = strstr(link_path, hostname))) { + fprintf(stderr, "Did not find hostname in path\n"); + goto cleanup; + } + spot--; + *spot = '\0'; + if (virFileMakePathWithMode(link_path, 0755) < 0) { + fprintf(stderr, "Unable to make path to '%s'\n", link_path); + goto cleanup; + } + *spot = '/'; + + if ((fd = open(unique_id_path, O_CREAT|O_WRONLY, 0444)) < 0) { + fprintf(stderr, "Unable to create '%s'\n", unique_id_path); + goto cleanup; + } + + if (safewrite(fd, unique_id, 1) != 1) { + fprintf(stderr, "Unable to write '%s'\n", unique_id); + goto cleanup; + } + VIR_DEBUG("Created unique_id '%s'", unique_id_path); + + /* The link is to the path not the file - so remove the file */ + if (!(spot = strstr(unique_id_path, "unique_id"))) { + fprintf(stderr, "Did not find unique_id in path\n"); + goto cleanup; + } + spot--; + *spot = '\0'; + if (symlink(unique_id_path, link_path) < 0) { + fprintf(stderr, "Unable to create symlink '%s' to '%s'\n", + link_path, unique_id_path); + goto cleanup; + } + VIR_DEBUG("Created symlink '%s'", link_path); + + ret = 0; + + cleanup: + VIR_FORCE_CLOSE(fd); + VIR_FREE(unique_id_path); + VIR_FREE(link_path); + return ret; +} + +static int +init_scsihost_sysfs(const char *fakesysfsdir) +{ + int ret = 0; + + if (create_scsihost(fakesysfsdir, + "0000:00:1f.1/ata1/host0/scsi_host/host0", + "1", "host0") < 0 || + create_scsihost(fakesysfsdir, + "0000:00:1f.1/ata2/host1/scsi_host/host1", + "2", "host1") < 0 || + create_scsihost(fakesysfsdir, + "0000:00:1f.2/ata1/host0/scsi_host/host0", + "1", "host2") < 0 || + create_scsihost(fakesysfsdir, + "0000:00:1f.2/ata2/host1/scsi_host/host1", + "2", "host3") < 0) + ret = -1; + + return ret; +} + +/* Test virReadSCSIUniqueId */ +static int +testVirReadSCSIUniqueId(const void *data ATTRIBUTE_UNUSED) +{ + int hostnum, unique_id; + + for (hostnum = 0; hostnum < 4; hostnum++) { + if (virReadSCSIUniqueId(TEST_SCSIHOST_CLASS_PATH, + hostnum, &unique_id) < 0) { + fprintf(stderr, "Failed to read hostnum=%d unique_id\n", hostnum); + return -1; + } + + /* host0 and host2 have unique_id == 1 + * host1 and host3 have unique_id == 2 + */ + if ((hostnum == 0 || hostnum == 2) && unique_id != 1) { + fprintf(stderr, "The unique_id='%d' for hostnum=%d is wrong\n", + unique_id, hostnum); + return -1; + } else if ((hostnum == 1 || hostnum == 3) && unique_id != 2) { + fprintf(stderr, "The unique_id='%d' for hostnum=%d is wrong\n", + unique_id, hostnum); + return -1; + } + } + + return 0; +} + +# define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX" + +static int +mymain(void) +{ + int ret = -1; + char *fakesysfsdir = NULL; + + if (VIR_STRDUP_QUIET(fakesysfsdir, FAKESYSFSDIRTEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + goto cleanup; + } + + if (!mkdtemp(fakesysfsdir)) { + fprintf(stderr, "Cannot create fakesysfsdir"); + goto cleanup; + } + + setenv("LIBVIRT_FAKE_SYSFS_DIR", fakesysfsdir, 1); + + if (init_scsihost_sysfs(fakesysfsdir) < 0) { + fprintf(stderr, "Failed to create fakesysfs='%s'\n", fakesysfsdir); + goto cleanup; + } + + if (virAsprintfQuiet(&scsihost_class_path, "%s/class/scsi_host", + fakesysfsdir) < 0) { + fprintf(stderr, "Out of memory\n"); + goto cleanup; + } + VIR_DEBUG("Reading from '%s'", scsihost_class_path); + + if (virtTestRun("testVirReadSCSIUniqueId", + testVirReadSCSIUniqueId, NULL) < 0) { + ret = -1; + goto cleanup; + } + + ret = 0; + + cleanup: + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + virFileDeleteTree(fakesysfsdir); + VIR_FREE(fakesysfsdir); + VIR_FREE(scsihost_class_path); + return ret; +} + +VIRT_TEST_MAIN(mymain) +#else +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif -- 1.9.3
Add an optional unique_id parameter to nodedev. Allows for easier lookup and display of the unique_id value in order to document for use with scsi_host code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatnode.html.in | 11 +++++++++++ docs/schemas/nodedev.rng | 6 ++++++ src/conf/node_device_conf.c | 23 ++++++++++++++++------ src/conf/node_device_conf.h | 1 + src/node_device/node_device_linux_sysfs.c | 6 ++++++ src/test/test_driver.c | 5 +++-- .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 ++++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 76bf8af..cdf0277 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -173,6 +173,17 @@ <dl> <dt><code>host</code></dt> <dd>The SCSI host number.</dd> + <dt><code>unique_id</code></dt> + <dd>On input, this optionally provides the value from the + 'unique_id' file found in the scsi_host's directory. To + view the values of all 'unique_id' files, use <code>find -H + /sys/class/scsi_host/host{0..9}/unique_id | + xargs grep '[0-9]'</code>. On output, if the unique_id + file exists, the value from the file will be displayed. + This can be used in order to help uniquely identify the + scsi_host adapter in a <a href="formatstorage.html"> + Storage Pool</a>. <span class="since">Since 1.2.6</span> + </dd> <dt><code>capability</code></dt> <dd>Current capabilities include "vports_ops" (indicates vport operations are supported) and "fc_host". "vport_ops" diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 02d4106..31319da 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -310,6 +310,12 @@ </element> <optional> + <element name='unique_id'> + <ref name='positiveInteger'/> + </element> + </optional> + + <optional> <zeroOrMore> <element name='capability'> <choice> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 99fa448..e73a9f4 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -399,6 +399,9 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_SCSI_HOST: virBufferAsprintf(&buf, "<host>%d</host>\n", data->scsi_host.host); + if (data->scsi_host.unique_id != -1) + virBufferAsprintf(&buf, "<unique_id>%d</unique_id>\n", + data->scsi_host.unique_id); if (data->scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { virBufferAddLit(&buf, "<capability type='fc_host'>\n"); virBufferAdjustIndent(&buf, 2); @@ -788,12 +791,20 @@ virNodeDevCapSCSIHostParseXML(xmlXPathContextPtr ctxt, orignode = ctxt->node; ctxt->node = node; - if (create == EXISTING_DEVICE && - virNodeDevCapsDefParseULong("number(./host[1])", ctxt, - &data->scsi_host.host, def, - _("no SCSI host ID supplied for '%s'"), - _("invalid SCSI host ID supplied for '%s'")) < 0) { - goto out; + if (create == EXISTING_DEVICE) { + if (virNodeDevCapsDefParseULong("number(./host[1])", ctxt, + &data->scsi_host.host, def, + _("no SCSI host ID supplied for '%s'"), + _("invalid SCSI host ID supplied for '%s'")) < 0) { + goto out; + } + /* Optional unique_id value */ + data->scsi_host.unique_id = -1; + if (virNodeDevCapsDefParseIntOptional("number(./unique_id[1])", ctxt, + &data->scsi_host.unique_id, def, + _("invalid unique_id supplied for '%s'")) < 0) { + goto out; + } } if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) { diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 50ce4b3..e949d93 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -140,6 +140,7 @@ struct _virNodeDevCapsDef { } net; struct { unsigned int host; + int unique_id; char *wwnn; char *wwpn; char *fabric_wwn; diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 6d9726f..be95e51 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -47,6 +47,12 @@ detect_scsi_host_caps(union _virNodeDevCapData *d) char *vports = NULL; int ret = -1; + if (virReadSCSIUniqueId(NULL, d->scsi_host.host, + &d->scsi_host.unique_id) < 0) { + VIR_DEBUG("Failed to read unique_id for host%d", d->scsi_host.host); + d->scsi_host.unique_id = -1; + } + VIR_DEBUG("Checking if host%d is an FC HBA", d->scsi_host.host); if (virIsCapableFCHost(NULL, d->scsi_host.host)) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f9e2b3d..8fc2ff3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6088,14 +6088,15 @@ testNodeDeviceCreateXML(virConnectPtr conn, if (VIR_STRDUP(def->name, wwpn) < 0) goto cleanup; - /* Fill in a random 'host' value, since this would also come from - * the backend */ + /* Fill in a random 'host' and 'unique_id' value, + * since this would also come from the backend */ caps = def->caps; while (caps) { if (caps->type != VIR_NODE_DEV_CAP_SCSI_HOST) continue; caps->data.scsi_host.host = virRandomBits(10); + caps->data.scsi_host.unique_id = 2; caps = caps->next; } diff --git a/tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml b/tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml new file mode 100644 index 0000000..5428f59 --- /dev/null +++ b/tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml @@ -0,0 +1,8 @@ +<device> + <name>pci_8086_27c5_scsi_host_0</name> + <parent>pci_8086_27c5</parent> + <capability type='scsi_host'> + <host>1</host> + <unique_id>2</unique_id> + </capability> +</device> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 9390bf5..ea41d84 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -82,6 +82,7 @@ mymain(void) DO_TEST("pci_1002_71c4"); DO_TEST("pci_8086_10c9_sriov_pf"); DO_TEST("pci_8086_27c5_scsi_host_0"); + DO_TEST("pci_8086_27c5_scsi_host_0_unique_id"); DO_TEST("pci_8086_27c5_scsi_host_scsi_device_lun0"); DO_TEST("pci_8086_27c5_scsi_host_scsi_host"); DO_TEST("pci_8086_27c5_scsi_host"); -- 1.9.3
On Tue, Jun 10, 2014 at 03:03:55PM -0400, John Ferlan wrote:
Add an optional unique_id parameter to nodedev. Allows for easier lookup and display of the unique_id value in order to document for use with scsi_host code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatnode.html.in | 11 +++++++++++ docs/schemas/nodedev.rng | 6 ++++++ src/conf/node_device_conf.c | 23 ++++++++++++++++------ src/conf/node_device_conf.h | 1 + src/node_device/node_device_linux_sysfs.c | 6 ++++++ src/test/test_driver.c | 5 +++-- .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 ++++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 76bf8af..cdf0277 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -173,6 +173,17 @@ <dl> <dt><code>host</code></dt> <dd>The SCSI host number.</dd> + <dt><code>unique_id</code></dt> + <dd>On input, this optionally provides the value from the + 'unique_id' file found in the scsi_host's directory. To + view the values of all 'unique_id' files, use <code>find -H + /sys/class/scsi_host/host{0..9}/unique_id | + xargs grep '[0-9]'</code>. On output, if the unique_id + file exists, the value from the file will be displayed. + This can be used in order to help uniquely identify the + scsi_host adapter in a <a href="formatstorage.html"> + Storage Pool</a>. <span class="since">Since 1.2.6</span> + </dd>
Where is the data from the 'unique_id' coming from ? Is this since that Linux makes up, or is it some standard attribute from the hardware ? It would be desirable to document what it is in a way that's not simply refering to Linux sysfs, since sysfs itself is mostly undocumented :-) Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2014 05:31 AM, Daniel P. Berrange wrote:
On Tue, Jun 10, 2014 at 03:03:55PM -0400, John Ferlan wrote:
Add an optional unique_id parameter to nodedev. Allows for easier lookup and display of the unique_id value in order to document for use with scsi_host code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatnode.html.in | 11 +++++++++++ docs/schemas/nodedev.rng | 6 ++++++ src/conf/node_device_conf.c | 23 ++++++++++++++++------ src/conf/node_device_conf.h | 1 + src/node_device/node_device_linux_sysfs.c | 6 ++++++ src/test/test_driver.c | 5 +++-- .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 ++++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 76bf8af..cdf0277 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -173,6 +173,17 @@ <dl> <dt><code>host</code></dt> <dd>The SCSI host number.</dd> + <dt><code>unique_id</code></dt> + <dd>On input, this optionally provides the value from the + 'unique_id' file found in the scsi_host's directory. To + view the values of all 'unique_id' files, use <code>find -H + /sys/class/scsi_host/host{0..9}/unique_id | + xargs grep '[0-9]'</code>. On output, if the unique_id + file exists, the value from the file will be displayed. + This can be used in order to help uniquely identify the + scsi_host adapter in a <a href="formatstorage.html"> + Storage Pool</a>. <span class="since">Since 1.2.6</span> + </dd>
Where is the data from the 'unique_id' coming from ? Is this since that Linux makes up, or is it some standard attribute from the hardware ? It would be desirable to document what it is in a way that's not simply refering to Linux sysfs, since sysfs itself is mostly undocumented :-)
Using 'unique_id' was from Osier's initial design - I 'assumed' that his research into this had determined using this was unique enough. A small amount of searching turns this up: https://lkml.org/lkml/2011/8/17/274 ... +What: /sys/bus/scsi/devices/host*/scsi_host/host*/unique_id +Date: February, 2003 +KernelVersion: Unknown +Contact: James Bottomley <James.Bottomley@HansenPartnership.com> +Description: + Read only unsigned integer uniquely identifying the SCSI host + within the system. This value is assigned by the low level + driver. + ... Also from various sources, there's '/linux/include/scsi/scsi_host.h /* * This is a unique identifier that must be assigned so that we * have some way of identifying each detected host adapter properly * and uniquely. For hosts that do not support more than one card * in the system at one time, this does not need to be set. It is * initialized to 0 in scsi_register. */ unsigned int unique_id;
Regards, Daniel
On Wed, Jun 11, 2014 at 10:00:24AM -0400, John Ferlan wrote:
On 06/11/2014 05:31 AM, Daniel P. Berrange wrote:
On Tue, Jun 10, 2014 at 03:03:55PM -0400, John Ferlan wrote:
Add an optional unique_id parameter to nodedev. Allows for easier lookup and display of the unique_id value in order to document for use with scsi_host code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatnode.html.in | 11 +++++++++++ docs/schemas/nodedev.rng | 6 ++++++ src/conf/node_device_conf.c | 23 ++++++++++++++++------ src/conf/node_device_conf.h | 1 + src/node_device/node_device_linux_sysfs.c | 6 ++++++ src/test/test_driver.c | 5 +++-- .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 ++++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 76bf8af..cdf0277 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -173,6 +173,17 @@ <dl> <dt><code>host</code></dt> <dd>The SCSI host number.</dd> + <dt><code>unique_id</code></dt> + <dd>On input, this optionally provides the value from the + 'unique_id' file found in the scsi_host's directory. To + view the values of all 'unique_id' files, use <code>find -H + /sys/class/scsi_host/host{0..9}/unique_id | + xargs grep '[0-9]'</code>. On output, if the unique_id + file exists, the value from the file will be displayed. + This can be used in order to help uniquely identify the + scsi_host adapter in a <a href="formatstorage.html"> + Storage Pool</a>. <span class="since">Since 1.2.6</span> + </dd>
Where is the data from the 'unique_id' coming from ? Is this since that Linux makes up, or is it some standard attribute from the hardware ? It would be desirable to document what it is in a way that's not simply refering to Linux sysfs, since sysfs itself is mostly undocumented :-)
Using 'unique_id' was from Osier's initial design - I 'assumed' that his research into this had determined using this was unique enough.
A small amount of searching turns this up:
https://lkml.org/lkml/2011/8/17/274 ... +What: /sys/bus/scsi/devices/host*/scsi_host/host*/unique_id +Date: February, 2003 +KernelVersion: Unknown +Contact: James Bottomley <James.Bottomley@HansenPartnership.com> +Description: + Read only unsigned integer uniquely identifying the SCSI host + within the system. This value is assigned by the low level + driver. + ...
Also from various sources, there's '/linux/include/scsi/scsi_host.h
/* * This is a unique identifier that must be assigned so that we * have some way of identifying each detected host adapter properly * and uniquely. For hosts that do not support more than one card * in the system at one time, this does not need to be set. It is * initialized to 0 in scsi_register. */ unsigned int unique_id;
Hmm, that's all rather fuzzy. I wonder how much "better" this is unique int ID is than the current hostNN numbers. eg is this unique id stable when you plug / unplug iSCSI targets in arbitrary order Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2014 10:10 AM, Daniel P. Berrange wrote:
On Wed, Jun 11, 2014 at 10:00:24AM -0400, John Ferlan wrote:
On 06/11/2014 05:31 AM, Daniel P. Berrange wrote:
On Tue, Jun 10, 2014 at 03:03:55PM -0400, John Ferlan wrote:
Where is the data from the 'unique_id' coming from ? Is this since that Linux makes up, or is it some standard attribute from the hardware ? It would be desirable to document what it is in a way that's not simply refering to Linux sysfs, since sysfs itself is mostly undocumented :-)
Using 'unique_id' was from Osier's initial design - I 'assumed' that his research into this had determined using this was unique enough.
A small amount of searching turns this up:
https://lkml.org/lkml/2011/8/17/274 ... +What: /sys/bus/scsi/devices/host*/scsi_host/host*/unique_id +Date: February, 2003 +KernelVersion: Unknown +Contact: James Bottomley <James.Bottomley@HansenPartnership.com> +Description: + Read only unsigned integer uniquely identifying the SCSI host + within the system. This value is assigned by the low level + driver. + ...
Also from various sources, there's '/linux/include/scsi/scsi_host.h
/* * This is a unique identifier that must be assigned so that we * have some way of identifying each detected host adapter properly * and uniquely. For hosts that do not support more than one card * in the system at one time, this does not need to be set. It is * initialized to 0 in scsi_register. */ unsigned int unique_id;
Hmm, that's all rather fuzzy. I wonder how much "better" this is unique int ID is than the current hostNN numbers. eg is this unique id stable when you plug / unplug iSCSI targets in arbitrary order
Not sure I can answer with authority on the plug/unplug topic; however, as I understand it the hostNN numbers can "change" what they are pointing at after perhaps a reboot or scsi kernel module reload. I'm not an expert in these matters, but as I do understand it hardware is scanned and devices named in the order they are found. Using the PCI address and unique_id in combination provides a bit more of a chance that if the hostNN number changes, then libvirt will/could find the same adapter that was previously used. John
Introduce a new function to parse the provided scsi_host parent address and unique_id value in order to find the /sys/class/scsi_host directory which will allow a stable SCSI host address Add a test to scsihosttest to lookup the host# name by using the PCI address and unique_id value Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 2 + src/libvirt_private.syms | 1 + src/util/virutil.c | 101 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++ tests/scsihosttest.c | 54 ++++++++++++++++++++++++ 5 files changed, 162 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 0e43fd8..1ad6aad 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -215,6 +215,8 @@ find -H /sys/bus/pci/devices/0000:00:1f.2 -name unique_id | xargs grep '[0-9]'</code> or <code> grep '[0-9]' /sys/class/scsi_host/host{0..9}/unique_id</code>. + Optionally, the 'virsh nodedev-dumpxml' of a specific scsi_host + list entry will list the <code>unique_id</code> value. </dd> </dl> </dd> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a51e456..1915227 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2048,6 +2048,7 @@ virDoubleToStr; virEnumFromString; virEnumToString; virFindFCHostCapableVport; +virFindSCSIHostByPCI; virFormatIntDecimal; virGetDeviceID; virGetDeviceUnprivSGIO; diff --git a/src/util/virutil.c b/src/util/virutil.c index c73ce06..350b6fe 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1725,6 +1725,98 @@ virReadSCSIUniqueId(const char *sysfs_prefix, return ret; } +/* virFindSCSIHostByPCI: + * @sysfs_prefix: "scsi_host" sysfs path, defaults to SYSFS_SCSI_HOST_PATH + * @parentaddr: string of the PCI address "scsi_host" device to be found + * @unique_id: unique_id value of the to be found "scsi_host" device + * @result: Return the host# of the matching "scsi_host" device + * + * Iterate over the SYSFS_SCSI_HOST_PATH entries looking for a matching + * PCI Address in the expected format (dddd:bb:ss.f, where 'dddd' is the + * 'domain' value, 'bb' is the 'bus' value, 'ss' is the 'slot' value, and + * 'f' is the 'function' value from the PCI address) with a unique_id file + * entry having the value expected. Unlike virReadSCSIUniqueId() we don't + * have a host number yet and that's what we're looking for. + * + * Returns the host name of the "scsi_host" which must be freed by the caller, + * or NULL on failure + */ +char * +virFindSCSIHostByPCI(const char *sysfs_prefix, + const char *parentaddr, + unsigned int unique_id) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; + struct dirent *entry = NULL; + DIR *dir = NULL; + char *host_link = NULL; + char *host_path = NULL; + char *p = NULL; + char *ret = NULL; + char *buf = NULL; + char *unique_path = NULL; + unsigned int read_unique_id; + + if (!(dir = opendir(prefix))) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + prefix); + goto cleanup; + } + + while (virDirRead(dir, &entry, prefix) > 0) { + if (entry->d_name[0] == '.' || !virFileIsLink(entry->d_name)) + continue; + + if (virAsprintf(&host_link, "%s/%s", prefix, entry->d_name) < 0) + goto cleanup; + + if (virFileResolveLink(host_link, &host_path) < 0) + goto cleanup; + + if (!strstr(host_path, parentaddr)) { + VIR_FREE(host_link); + VIR_FREE(host_path); + continue; + } + VIR_FREE(host_link); + VIR_FREE(host_path); + + if (virAsprintf(&unique_path, "%s/%s/unique_id", prefix, + entry->d_name) < 0) + goto cleanup; + + if (!virFileExists(unique_path)) { + VIR_FREE(unique_path); + continue; + } + + if (virFileReadAll(unique_path, 1024, &buf) < 0) + goto cleanup; + + if ((p = strchr(buf, '\n'))) + *p = '\0'; + + if (virStrToLong_ui(buf, NULL, 10, &read_unique_id) < 0) + goto cleanup; + + if (read_unique_id != unique_id) { + VIR_FREE(unique_path); + continue; + } + + ignore_value(VIR_STRDUP(ret, entry->d_name)); + break; + } + + cleanup: + closedir(dir); + VIR_FREE(unique_path); + VIR_FREE(host_link); + VIR_FREE(host_path); + return ret; +} + /* virReadFCHost: * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH * @host: Host number, E.g. 5 of "fc_host/host5" @@ -2086,6 +2178,15 @@ virReadSCSIUniqueId(const char *sysfs_prefix ATTRIBUTE_UNUSED, return -1; } +char * +virFindSCSIHostByPCI(const char *sysfs_prefix ATTRIBUTE_UNUSED, + const char *parentaddr ATTRIBUTE_UNUSED, + unsigned int unique_id ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return -1; +} + int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, int host ATTRIBUTE_UNUSED, diff --git a/src/util/virutil.h b/src/util/virutil.h index 1407dfd..e8c1d7c 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -168,6 +168,10 @@ int virReadSCSIUniqueId(const char *sysfs_prefix, int host, int *result) ATTRIBUTE_NONNULL(3); +char * +virFindSCSIHostByPCI(const char *sysfs_prefix, + const char *parentaddr, + unsigned int unique_id); int virReadFCHost(const char *sysfs_prefix, int host, const char *entry, diff --git a/tests/scsihosttest.c b/tests/scsihosttest.c index 990fe80..eecb1c3 100644 --- a/tests/scsihosttest.c +++ b/tests/scsihosttest.c @@ -196,6 +196,54 @@ testVirReadSCSIUniqueId(const void *data ATTRIBUTE_UNUSED) return 0; } +/* Test virFindSCSIHostByPCI */ +static int +testVirFindSCSIHostByPCI(const void *data ATTRIBUTE_UNUSED) +{ + unsigned int unique_id1 = 1; + unsigned int unique_id2 = 2; + const char *pci_addr1 = "0000:00:1f.1"; + const char *pci_addr2 = "0000:00:1f.2"; + char *path_addr = NULL; + char *ret_host = NULL; + int ret = -1; + + if (virAsprintf(&path_addr, "%s/%s", abs_srcdir, + "sysfs/class/scsi_host") < 0) + goto cleanup; + + if (!(ret_host = virFindSCSIHostByPCI(TEST_SCSIHOST_CLASS_PATH, + pci_addr1, unique_id1)) || + STRNEQ(ret_host, "host0")) + goto cleanup; + VIR_FREE(ret_host); + + if (!(ret_host = virFindSCSIHostByPCI(TEST_SCSIHOST_CLASS_PATH, + pci_addr1, unique_id2)) || + STRNEQ(ret_host, "host1")) + goto cleanup; + VIR_FREE(ret_host); + + if (!(ret_host = virFindSCSIHostByPCI(TEST_SCSIHOST_CLASS_PATH, + pci_addr2, unique_id1)) || + STRNEQ(ret_host, "host2")) + goto cleanup; + VIR_FREE(ret_host); + + if (!(ret_host = virFindSCSIHostByPCI(TEST_SCSIHOST_CLASS_PATH, + pci_addr2, unique_id2)) || + STRNEQ(ret_host, "host3")) + goto cleanup; + VIR_FREE(ret_host); + + ret = 0; + + cleanup: + VIR_FREE(ret_host); + VIR_FREE(path_addr); + return ret; +} + # define FAKESYSFSDIRTEMPLATE abs_builddir "/fakesysfsdir-XXXXXX" static int @@ -234,6 +282,12 @@ mymain(void) goto cleanup; } + if (virtTestRun("testVirFindSCSIHostByPCI", + testVirFindSCSIHostByPCI, NULL) < 0) { + ret = -1; + goto cleanup; + } + ret = 0; cleanup: -- 1.9.3
If a parentaddr was provided in the XML, have getAdapterName lookup the stable address. This allows virStorageBackendSCSICheckPool() and virStorageBackendSCSIRefreshPool() to automagically find the scsi_host by its PCI address and unique_id Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index e42778f..7c5044c 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -547,9 +547,29 @@ static char * getAdapterName(virStoragePoolSourceAdapter adapter) { char *name = NULL; + char *parentaddr = NULL; if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - ignore_value(VIR_STRDUP(name, adapter.data.scsi_host.name)); + if (adapter.data.scsi_host.has_parent) { + unsigned int unique_id = adapter.data.scsi_host.unique_id; + + if (virAsprintf(&parentaddr, "%04x:%02x:%02x.%01x", + adapter.data.scsi_host.parentaddr.domain, + adapter.data.scsi_host.parentaddr.bus, + adapter.data.scsi_host.parentaddr.slot, + adapter.data.scsi_host.parentaddr.function) < 0) + goto cleanup; + if (!(name = virFindSCSIHostByPCI(NULL, parentaddr, + unique_id))) { + virReportError(VIR_ERR_XML_ERROR, + _("Failed to find scsi_host using PCI '%s' " + "and unique_id='%u'"), + parentaddr, unique_id); + goto cleanup; + } + } else { + ignore_value(VIR_STRDUP(name, adapter.data.scsi_host.name)); + } } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, @@ -561,6 +581,8 @@ getAdapterName(virStoragePoolSourceAdapter adapter) } } + cleanup: + VIR_FREE(parentaddr); return name; } -- 1.9.3
Ping? Tks, John On 06/10/2014 03:03 PM, John Ferlan wrote:
Just over a year ago, Osier Yang submitted some patches to provide stable SCSI host addressing support:
http://www.redhat.com/archives/libvir-list/2013-June/msg00396.html
Although reviewed - none of the patches were ever pushed and Osier never got back to the work. I eventually inherited the changes and now after languishing in the todo list - I took the time to rework things. There is a bz associated:
https://bugzilla.redhat.com/show_bug.cgi?id=963817
Prior to leaving Red Hat, Osier and I spent some time revisiting the patch series and hashing out new/different ideas to come up with the same answer. My focus was to avoid the "generic recursive directory search" (patch 3), to not require having to know whether the scsi_host was using udev or hal, and to simplify as much as possible using existing data/information.
The generic directory search I believe was less generic than intended and over complicated things. The old patches essentially used the output of a 'virsh nodedev-list scsi_host' and then selecting a resulting entry ran a 'virsh nodedev-dumpxml' to grab/use the resulting <parent> value. Taking that value and optionally adding a unique_id value was the basis for the design that would generate a PCI address (either in udev or hal format) and recursively search through /sys/devices looking for a matching address in either udev or hal format.
These patches replace the bulk of the directory traversal logic with a more direct (and already in use) approach to scan the /sys/class/scsi_host directories looking for a matching PCI address found in the symlink of the files in the directory.
The changed logic will add a new XML element 'parentaddr' to describe the scsi_host by it's PCI address. Additionally, the 'unique_id' has become a required attribute. The code reuses the virDevicePCIAddressParseXML() in order parse the required 'address' element. The 'address' will be in the expect PCI Address format like other described host devices.
In order to help view the required unique_id value, the nodedev-dumpxml output has been adjusted to provide the <unique_id> value. This value will be an optional value on input.
The documentation is updated to describe how to generate the address from the nodedev-dumpxml output.
The new scsihosttest creates the expected PCI infrastructure on the fly since adding files with colons (':') is prohibited. There are multiple directories and hosts within each to ensure the search logic worked as expected.
Reviewer notes:
Patch 1 is new - it's just forcing the specific adapter.type checking Patches 2 & 3 are the former patches 1 & 2 with some edits to adjust for new XML syntax (and changed associated structure) Patch 4 is new - it's just utilizing the 'LINUX_SYSFS_SCSI_HOST_PREFIX' definition rather than using the hardcoded value Patches 5 & 6 are new. They handle the optional unique_id value (including using the new virNodeDevCapsDefParseIntOptional()) Patch 7 is similar to the former patch 5 insomuch as it's where the comparison of the PCI directory path and unique_id file is made. Patch 8 is similar to the former patch 7 insomuch as it's where the link is made between the scsi_host host# and the loading/refreshing of the scsi_host adapter.
Former patches 3, 4, 6, and 8-11 are no longer used
Please check my XML schema (patch 3) - I think I figured out the right syntax to use regarding using either "<name>" or "<parentaddr>" where "<parentaddr>" is an element. The syntax passes the make check, but it's not an area of expertise for me. Using parentaddr was preferred over overloading the parent attribute.
John Ferlan (6): getAdapterName: check for SCSI_HOST scsi_backend: Use existing LINUX_SYSFS_SCSI_HOST_PREFIX definition virutil: Introduce virReadSCSIUniqueId Add unique_id to nodedev output scsi_host: Introduce virFindSCSIHostByPCI getAdapterName: Lookup stable scsi_host
Osier Yang (2): virStoragePoolSourceAdapter: Refine the SCSI_HOST adapter name storage: Introduce parentaddr into virStoragePoolSourceAdapter
docs/formatnode.html.in | 11 + docs/formatstorage.html.in | 130 +++++++-- docs/schemas/basictypes.rng | 24 +- docs/schemas/nodedev.rng | 6 + src/conf/node_device_conf.c | 23 +- src/conf/node_device_conf.h | 1 + src/conf/storage_conf.c | 111 +++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 2 + src/node_device/node_device_linux_sysfs.c | 6 + src/phyp/phyp_driver.c | 8 +- src/storage/storage_backend_scsi.c | 53 +++- src/test/test_driver.c | 5 +- src/util/virutil.c | 154 +++++++++++ src/util/virutil.h | 8 + tests/Makefile.am | 7 + .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 + tests/nodedevxml2xmltest.c | 1 + tests/scsihosttest.c | 308 +++++++++++++++++++++ .../pool-scsi-type-scsi-host-stable.xml | 19 ++ .../pool-scsi-type-scsi-host-stable.xml | 22 ++ tests/storagepoolxml2xmltest.c | 1 + 22 files changed, 855 insertions(+), 61 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml create mode 100644 tests/scsihosttest.c create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml
Any chance this series gets reviewed before freeze? Or should I just abandon it (and the close the associated bz)? Tks, John On 06/10/2014 03:03 PM, John Ferlan wrote:
Just over a year ago, Osier Yang submitted some patches to provide stable SCSI host addressing support:
http://www.redhat.com/archives/libvir-list/2013-June/msg00396.html
Although reviewed - none of the patches were ever pushed and Osier never got back to the work. I eventually inherited the changes and now after languishing in the todo list - I took the time to rework things. There is a bz associated:
https://bugzilla.redhat.com/show_bug.cgi?id=963817
Prior to leaving Red Hat, Osier and I spent some time revisiting the patch series and hashing out new/different ideas to come up with the same answer. My focus was to avoid the "generic recursive directory search" (patch 3), to not require having to know whether the scsi_host was using udev or hal, and to simplify as much as possible using existing data/information.
The generic directory search I believe was less generic than intended and over complicated things. The old patches essentially used the output of a 'virsh nodedev-list scsi_host' and then selecting a resulting entry ran a 'virsh nodedev-dumpxml' to grab/use the resulting <parent> value. Taking that value and optionally adding a unique_id value was the basis for the design that would generate a PCI address (either in udev or hal format) and recursively search through /sys/devices looking for a matching address in either udev or hal format.
These patches replace the bulk of the directory traversal logic with a more direct (and already in use) approach to scan the /sys/class/scsi_host directories looking for a matching PCI address found in the symlink of the files in the directory.
The changed logic will add a new XML element 'parentaddr' to describe the scsi_host by it's PCI address. Additionally, the 'unique_id' has become a required attribute. The code reuses the virDevicePCIAddressParseXML() in order parse the required 'address' element. The 'address' will be in the expect PCI Address format like other described host devices.
In order to help view the required unique_id value, the nodedev-dumpxml output has been adjusted to provide the <unique_id> value. This value will be an optional value on input.
The documentation is updated to describe how to generate the address from the nodedev-dumpxml output.
The new scsihosttest creates the expected PCI infrastructure on the fly since adding files with colons (':') is prohibited. There are multiple directories and hosts within each to ensure the search logic worked as expected.
Reviewer notes:
Patch 1 is new - it's just forcing the specific adapter.type checking Patches 2 & 3 are the former patches 1 & 2 with some edits to adjust for new XML syntax (and changed associated structure) Patch 4 is new - it's just utilizing the 'LINUX_SYSFS_SCSI_HOST_PREFIX' definition rather than using the hardcoded value Patches 5 & 6 are new. They handle the optional unique_id value (including using the new virNodeDevCapsDefParseIntOptional()) Patch 7 is similar to the former patch 5 insomuch as it's where the comparison of the PCI directory path and unique_id file is made. Patch 8 is similar to the former patch 7 insomuch as it's where the link is made between the scsi_host host# and the loading/refreshing of the scsi_host adapter.
Former patches 3, 4, 6, and 8-11 are no longer used
Please check my XML schema (patch 3) - I think I figured out the right syntax to use regarding using either "<name>" or "<parentaddr>" where "<parentaddr>" is an element. The syntax passes the make check, but it's not an area of expertise for me. Using parentaddr was preferred over overloading the parent attribute.
John Ferlan (6): getAdapterName: check for SCSI_HOST scsi_backend: Use existing LINUX_SYSFS_SCSI_HOST_PREFIX definition virutil: Introduce virReadSCSIUniqueId Add unique_id to nodedev output scsi_host: Introduce virFindSCSIHostByPCI getAdapterName: Lookup stable scsi_host
Osier Yang (2): virStoragePoolSourceAdapter: Refine the SCSI_HOST adapter name storage: Introduce parentaddr into virStoragePoolSourceAdapter
docs/formatnode.html.in | 11 + docs/formatstorage.html.in | 130 +++++++-- docs/schemas/basictypes.rng | 24 +- docs/schemas/nodedev.rng | 6 + src/conf/node_device_conf.c | 23 +- src/conf/node_device_conf.h | 1 + src/conf/storage_conf.c | 111 +++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 2 + src/node_device/node_device_linux_sysfs.c | 6 + src/phyp/phyp_driver.c | 8 +- src/storage/storage_backend_scsi.c | 53 +++- src/test/test_driver.c | 5 +- src/util/virutil.c | 154 +++++++++++ src/util/virutil.h | 8 + tests/Makefile.am | 7 + .../pci_8086_27c5_scsi_host_0_unique_id.xml | 8 + tests/nodedevxml2xmltest.c | 1 + tests/scsihosttest.c | 308 +++++++++++++++++++++ .../pool-scsi-type-scsi-host-stable.xml | 19 ++ .../pool-scsi-type-scsi-host-stable.xml | 22 ++ tests/storagepoolxml2xmltest.c | 1 + 22 files changed, 855 insertions(+), 61 deletions(-) create mode 100644 tests/nodedevschemadata/pci_8086_27c5_scsi_host_0_unique_id.xml create mode 100644 tests/scsihosttest.c create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml
participants (2)
-
Daniel P. Berrange -
John Ferlan