Providing XML such as: <device> <name>vhba</name> <capability type='scsi_host'> <capability type='fc_host'> </capability> </capability> </device> would crash libvirt because the '<parent>' isn't a required field, but for vHBA creation it's expected (day 1 issue - see commit id '81d0ffbc'). The nodedev.rng added in commit id '2c22a68c' has this as an optional field. NB: On normal udev discovery if a parent field wasn't found, it would be set to "computer" by udevSetParent, so this is a somewhat unique path. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/node_device_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 1cd0baf..bf5b22f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -118,7 +118,7 @@ virNodeDeviceObjPtr virNodeDeviceFindByName(virNodeDeviceObjListPtr devs, for (i = 0; i < devs->count; i++) { virNodeDeviceObjLock(devs->objs[i]); - if (STREQ(devs->objs[i]->def->name, name)) + if (STREQ_NULLABLE(devs->objs[i]->def->name, name))

return devs->objs[i]; virNodeDeviceObjUnlock(devs->objs[i]); }

On Fri, Nov 18, 2016 at 09:26:29AM -0500, John Ferlan wrote: > If a <parent> is not supplied in the XML used to create a non-persistent > vHBA, then instead of failing, let's try to find a "vports" capable node > device and use that. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/node_device_conf.c | 39 > ++++++++++++++++++++++++++++++++++++ > src/conf/node_device_conf.h | 3 +++ > src/libvirt_private.syms | 1 + > src/node_device/node_device_driver.c | 15 +++++++++----- > 4 files changed, 53 insertions(+), 5 deletions(-) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 5396681..3aa77cf 100644 > + > +int > +virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs, > + int *parent_host) > +{ > + virNodeDeviceObjPtr parent = NULL; > + int ret; > + > + if (!(parent = virNodeDeviceFindByCap(devs, "vports"))) { Why do we pass the capability as a string?

> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not find any vport capable device")); > + return -1; > + } > + > diff --git a/src/node_device/node_device_driver.c > b/src/node_device/node_device_driver.c > index 91bb142..0e091fe 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -584,11 +584,16 @@ nodeDeviceCreateXML(virConnectPtr conn, > if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) > goto cleanup; > > - if (virNodeDeviceGetParentHost(&driver->devs, > - def->name, > - def->parent, > - &parent_host) == -1) { > - goto cleanup; > + if (def->parent) { > + if (virNodeDeviceGetParentHost(&driver->devs, > + def->name, > + def->parent, > + &parent_host) < 0) > + goto cleanup; > + } else { > + /* Try to find "a" vport capable scsi_host when no parent > supplied */ The quotes make me nervous.

> + if (virNodeDeviceFindVportParentHost(&driver->devs, > &parent_host) < 0) > + goto cleanup; > } ACK Jan

https://bugzilla.redhat.com/show_bug.cgi?id=1349696 When creating a vHBA, the process is to feed XML to nodeDeviceCreateXML that lists the <parent> scsi_hostX to use to create the vHBA. However, between reboots, it's possible that the <parent> changes its scsi_hostX to scsi_hostY and saved XML to perform the creation will either fail or create a vHBA using the wrong parent. So add the ability to provide <parent_wwnn> and <parent_wwpn> or <parent_fabric_wwn> in order to use those values as more consistent search parameters. For some providing the wwnn/wwpn pair will provide the most specific search option, while for others providing a fabric_wwn will at least ensure usage of the same SAN. This patch will add the new fields to the nodedev.rng for input purposes only since the input XML is essentially thrown away, no need to Format the values since they'd already be printed as part of the scsi_host data block. New API virNodeDeviceGetParentHostByWWNs will take the parent_wwnn and parent_wwpn in order to search the list of devices for matching capability data fields wwnn and wwpn. New API virNodeDeviceGetParentHostByFabricWWN will take the parent_fabric_wwn in order to search the list of devices for matching capability data field fabric_wwn. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/schemas/nodedev.rng | 15 +++++ src/conf/node_device_conf.c | 120 +++++++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 14 ++++ src/libvirt_private.syms | 2 + src/node_device/node_device_driver.c | 13 ++++ 5 files changed, 164 insertions(+) diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 93a88d8..6fe49a3 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -18,6 +18,21 @@ <optional> <element name="parent"><text/></element> </optional> + <optional> + <element name="parent_wwnn"> + <ref name='wwn'/> + </element> + </optional> + <optional> + <element name="parent_wwpn"> + <ref name='wwn'/> + </element> + </optional> + <optional> + <element name="parent_fabric_wwn"> + <ref name='wwn'/> + </element> + </optional> <optional> <element name="driver"> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 3aa77cf..cecd915 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -92,6 +92,30 @@ int virNodeDeviceHasCap(const virNodeDeviceObj *dev, const char *cap) } +/* virNodeDeviceFindFCCapDef: + * @dev: Pointer to current device + * + * Search the device object 'caps' array for fc_host capability. + * + * Returns: + * Pointer to the caps or NULL if not found + */ +static virNodeDevCapsDefPtr +virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev) +{ + virNodeDevCapsDefPtr caps = dev->def->caps; + + while (caps) { + if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST && + (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) + break; + + caps = caps->next; + } + return caps; +} + + /* virNodeDeviceFindVPORTCapDef: * @dev: Pointer to current device * @@ -152,6 +176,46 @@ virNodeDeviceObjPtr virNodeDeviceFindByName(virNodeDeviceObjListPtr devs, static virNodeDeviceObjPtr +virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, + const char *parent_wwnn, + const char *parent_wwpn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDevCapsDefPtr cap; + virNodeDeviceObjLock(devs->objs[i]); + if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) && + STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn)) + return devs->objs[i]; + virNodeDeviceObjUnlock(devs->objs[i]); + } + + return NULL; +} + + +static virNodeDeviceObjPtr +virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs, + const char *parent_fabric_wwn) +{ + size_t i; + + for (i = 0; i < devs->count; i++) { + virNodeDevCapsDefPtr cap; + virNodeDeviceObjLock(devs->objs[i]); + if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) && + STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn)) + return devs->objs[i]; + virNodeDeviceObjUnlock(devs->objs[i]); + } + + return NULL; +} + + +static virNodeDeviceObjPtr virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs, const char *cap) { @@ -177,6 +241,9 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def) VIR_FREE(def->name); VIR_FREE(def->parent); + VIR_FREE(def->parent_wwnn); + VIR_FREE(def->parent_wwpn); + VIR_FREE(def->parent_fabric_wwn); VIR_FREE(def->driver); VIR_FREE(def->sysfs_path); VIR_FREE(def->parent_sysfs_path); @@ -1652,6 +1719,10 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, /* Extract device parent, if any */ def->parent = virXPathString("string(./parent[1])", ctxt); + def->parent_wwnn = virXPathString("string(./parent_wwnn[1])", ctxt); + def->parent_wwpn = virXPathString("string(./parent_wwpn[1])", ctxt); + def->parent_fabric_wwn = virXPathString("string(./parent_fabric_wwn[1])", + ctxt); /* Parse device capabilities */ nodes = NULL; @@ -1847,6 +1918,55 @@ virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs, int +virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_wwnn, + const char *parent_wwpn, + int *parent_host) +{ + virNodeDeviceObjPtr parent = NULL; + int ret; + + if (!(parent = virNodeDeviceFindByWWNs(devs, parent_wwnn, parent_wwpn))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find parent device for '%s'"), + dev_name); + return -1; + } + + ret = virNodeDeviceFindFCParentHost(parent, parent_host); + + virNodeDeviceObjUnlock(parent); + + return ret; +} + + +int +virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_fabric_wwn, + int *parent_host) +{ + virNodeDeviceObjPtr parent = NULL; + int ret; + + if (!(parent = virNodeDeviceFindByFabricWWN(devs, parent_fabric_wwn))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find parent device for '%s'"), + dev_name); + return -1; + } + + ret = virNodeDeviceFindFCParentHost(parent, parent_host); + + virNodeDeviceObjUnlock(parent); + + return ret; +} + + +int virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs, int *parent_host) { diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 2b2aed7..1634483 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -200,6 +200,9 @@ struct _virNodeDeviceDef { char *sysfs_path; /* udev name/sysfs path */ char *parent; /* optional parent device name */ char *parent_sysfs_path; /* udev parent name/sysfs path */ + char *parent_wwnn; /* optional parent wwnn */ + char *parent_wwpn; /* optional parent wwpn */ + char *parent_fabric_wwn; /* optional parent fabric_wwn */ char *driver; /* optional driver name */ virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; @@ -273,6 +276,17 @@ int virNodeDeviceGetParentHost(virNodeDeviceObjListPtr devs, const char *parent_name, int *parent_host); +int virNodeDeviceGetParentHostByWWNs(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_wwnn, + const char *parent_wwpn, + int *parent_host); + +int virNodeDeviceGetParentHostByFabricWWN(virNodeDeviceObjListPtr devs, + const char *dev_name, + const char *parent_fabric_wwn, + int *parent_host); + int virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs, int *parent_host); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de14a7e..5673bda 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -701,6 +701,8 @@ virNodeDeviceFindByName; virNodeDeviceFindBySysfsPath; virNodeDeviceFindVportParentHost; virNodeDeviceGetParentHost; +virNodeDeviceGetParentHostByFabricWWN; +virNodeDeviceGetParentHostByWWNs; virNodeDeviceGetWWNs; virNodeDeviceHasCap; virNodeDeviceObjListExport; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0e091fe..b7bec65 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -590,6 +590,19 @@ nodeDeviceCreateXML(virConnectPtr conn, def->parent, &parent_host) < 0) goto cleanup; + } else if (def->parent_wwnn && def->parent_wwpn) { + if (virNodeDeviceGetParentHostByWWNs(&driver->devs, + def->name, + def->parent_wwnn, + def->parent_wwpn, + &parent_host) < 0) + goto cleanup; + } else if (def->parent_fabric_wwn) { + if (virNodeDeviceGetParentHostByFabricWWN(&driver->devs, + def->name, + def->parent_fabric_wwn, + &parent_host) < 0) + goto cleanup; } else { /* Try to find "a" vport capable scsi_host when no parent supplied */ if (virNodeDeviceFindVportParentHost(&driver->devs, &parent_host) < 0)

On Fri, Nov 18, 2016 at 09:26:30AM -0500, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1349696 > > When creating a vHBA, the process is to feed XML to nodeDeviceCreateXML > that lists the <parent> scsi_hostX to use to create the vHBA. However, > between reboots, it's possible that the <parent> changes its scsi_hostX > to scsi_hostY and saved XML to perform the creation will either fail or > create a vHBA using the wrong parent. > > So add the ability to provide <parent_wwnn> and <parent_wwpn> or > <parent_fabric_wwn> in order to use those values as more consistent These are all flattening the XML and duplicating the "parent" in their name. Can we no longer put subelements in <parent> after we've used its contents as text?

> search parameters. For some providing the wwnn/wwpn pair will provide > the most specific search option, while for others providing a fabric_wwn > will at least ensure usage of the same SAN. > > This patch will add the new fields to the nodedev.rng for input purposes > only since the input XML is essentially thrown away, no need to Format > the values since they'd already be printed as part of the scsi_host > data block. > > New API virNodeDeviceGetParentHostByWWNs will take the parent_wwnn and > parent_wwpn in order to search the list of devices for matching > capability > data fields wwnn and wwpn. > > New API virNodeDeviceGetParentHostByFabricWWN will take the > parent_fabric_wwn > in order to search the list of devices for matching capability data field > fabric_wwn. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > docs/schemas/nodedev.rng | 15 +++++ > src/conf/node_device_conf.c | 120 > +++++++++++++++++++++++++++++++++++ > src/conf/node_device_conf.h | 14 ++++ > src/libvirt_private.syms | 2 + > src/node_device/node_device_driver.c | 13 ++++ > 5 files changed, 164 insertions(+) > > diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng > index 93a88d8..6fe49a3 100644 > --- a/docs/schemas/nodedev.rng > +++ b/docs/schemas/nodedev.rng > @@ -18,6 +18,21 @@ > <optional> > <element name="parent"><text/></element> > </optional> > + <optional> > + <element name="parent_wwnn"> > + <ref name='wwn'/> > + </element> > + </optional> > + <optional> > + <element name="parent_wwpn"> > + <ref name='wwn'/> > + </element> > + </optional> This allows either of {nn,pn} to be present, but the code expects both. Please put them in a single <optional> block if omitting either does not make sense.

> + <optional> > + <element name="parent_fabric_wwn"> > + <ref name='wwn'/> > + </element> > + </optional> > > <optional> > <element name="driver"> > @@ -1652,6 +1719,10 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, > > /* Extract device parent, if any */ > def->parent = virXPathString("string(./parent[1])", ctxt); > + def->parent_wwnn = virXPathString("string(./parent_wwnn[1])", ctxt); > + def->parent_wwpn = virXPathString("string(./parent_wwpn[1])", ctxt); Should we error out if only one was provided?

> + def->parent_fabric_wwn = > virXPathString("string(./parent_fabric_wwn[1])", > + ctxt); > > /* Parse device capabilities */ > nodes = NULL; Weak ACK, I'd rather see the XML nested (if possible). Jan

Move the check for an already existing vHBA to the top of the function. No sense in first decoding a provided parent if the next thing we're going to do is fail if a provided wwnn/wwpn already exists. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_scsi.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)

@@ -736,6 +722,20 @@ createVport(virConnectPtr conn, goto cleanup; } + /* If a parent was provided, then let's make sure it's vhost capable */ + if (adapter->data.fchost.parent) { + if (virGetSCSIHostNumber(adapter->data.fchost.parent, &parent_host) < 0) + return -1; + + if (!virIsCapableFCHost(NULL, parent_host)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' specified for vHBA " + "is not vport capable"), + adapter->data.fchost.parent); + return -1;

Remove duplicated code - make one path through Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_scsi.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 9863880..df48b1a 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -722,39 +722,26 @@ createVport(virConnectPtr conn,

- if (virGetSCSIHostNumber(parent_hoststr, &parent_host) < 0) - goto cleanup; + if (virGetSCSIHostNumber(parent_hoststr, &parent_host) < 0) + goto cleanup; - /* NOTE: - * We do not save the parent_hoststr in adapter->data.fchost.parent - * since we could be writing out the 'def' to the saved XML config. - * If we wrote out the name in the XML, then future starts would - * always use the same parent rather than finding the "best available" - * parent. Besides we have a way to determine the parent based on - * the 'name' field. - */

+ if (adapter->data.fchost.parent && !virIsCapableFCHost(NULL, parent_host)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' specified for vHBA is not vport capable"), + parent_hoststr); + goto cleanup; }

On Fri, Nov 18, 2016 at 09:26:36AM -0500, John Ferlan wrote: > Create a utility routine in order to read the scsi_host fabric_name files > looking for a match to a passed fabric_name > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/virutil.c | 86 > ++++++++++++++++++++++++++++++++++++++++-------- > src/util/virutil.h | 4 +++ > 3 files changed, 78 insertions(+), 13 deletions(-) > > diff --git a/src/util/virutil.c b/src/util/virutil.c > index a135819..fb72f2d 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -2166,6 +2166,18 @@ virManageVport(const int parent_host, > return ret; > } > > +# define READ_WWN(wwn_path, buf) \ This macro either jumps to a label or alters a third variable. I don't think its scope should extend one function. > + do { \ > + if (virFileReadAll(wwn_path, 1024, &buf) < 0) \ > + goto cleanup; \ These two lines can be split out and with the STR{,N}EQ included, this macro can be COMPARE_WWN, removing the need for changing p. Or turned into a more complex function like virUSBSysReadFile.

> + if ((p = strchr(buf, '\n'))) \ > + *p = '\0'; \ > + if (STRPREFIX(buf, "0x")) \ > + p = buf + strlen("0x"); \ > + else \ > + p = buf; \ > + } while (0) > + > /* virGetFCHostNameByWWN: > * > * Iterate over the sysfs tree to get FC host name (e.g. host5) The rest looks good to me.

https://bugzilla.redhat.com/show_bug.cgi?id=1349696 As it turns out using only the 'parent' to achieve the goal of a consistent vHBA parent has issues with reboots where the scsi_hostX parent could change to scsi_hostY causing either failure to create the vHBA or usage of the wrong HBA for our vHBA. Thus add the ability to search for the "parent" by the parent wwnn/ wwpn values or just a fabric_name if someone only cares to ensure usage of the same SAN for the vHBA. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_scsi.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)

@@ -732,14 +750,15 @@ createVport(virConnectPtr conn, "cannot find one on this host")); goto cleanup; } + skip_capable_check = true; } if (virGetSCSIHostNumber(parent_hoststr, &parent_host) < 0) goto cleanup; - if (adapter->data.fchost.parent && !virIsCapableFCHost(NULL, parent_host)) { + if (!skip_capable_check && !virIsCapableFCHost(NULL, parent_host)) { virReportError(VIR_ERR_XML_ERROR, - _("parent '%s' specified for vHBA is not vport capable"), + _("parent '%s' is not vport capable"),

On 12/03/2016 07:09 AM, John Ferlan wrote: > > ping? Sorry, I seem to have lost your initial cover letter so responding here... > > On 11/18/2016 09:26 AM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1349696 >> >> Lots of details in the bz, but essentially the problem is that providing >> a "parent" scsi_hostX value has drawbacks on reboots because what was >> scsi_hostX could turn into scsi_hostY on subsequent reboots. >> >> So add the ability to use the parent wwnn/wwpn or fabric_wwn as 'search' >> criteria in order to create either non persistent vHBA's via nodedev or >> persistent vHBA's via storage pools. I've not looked at all the patches in detail, but I have been giving them a fair bit of testing and haven't noticed any problems thus far.

Once question I do have is related to volume detection in vport-based pools. Currently when there are multiple paths to a LUN only the active ones are displayed in the pool. Inactive paths are ignored since saferead() fails in src/storage/storage_backend.c:virStorageBackendDetectBlockVolFormatFD(). Without the inactive paths, there would be no path to the LUN if it was trespassed. I made the below hack to virStorageBackendUpdateVolTargetInfo() to show the inactive paths too, and was able to pass those to the guest allowing it to handle multipathing. Is there a reason the inactive paths are ignored?

Regards, Jim Index: libvirt-2.5.0/src/storage/storage_backend.c =================================================================== --- libvirt-2.5.0.orig/src/storage/storage_backend.c +++ libvirt-2.5.0/src/storage/storage_backend.c @@ -1941,8 +1941,11 @@ virStorageBackendUpdateVolTargetInfo(vir if (withBlockVolFormat) { if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd, - readflags)) < 0) + readflags)) < 0) { + if (ret == -2) + ret = 0; goto cleanup; + } } cleanup: