On Fri, Nov 18, 2016 at 09:26:27AM -0500, John Ferlan wrote:

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@redhat.com> --- 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))

Is this needed after patch 3/11? I presume we do not store objects with NULL names in devs. In that case the callers should avoid calling this with NULL name - what is the point of iterating over the array in that case? Jan

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

On 01/02/2017 09:18 AM, Ján Tomko wrote:

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@redhat.com> --- 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?

OK - I'll alter this to: const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); and use "cap" instead of "vports". NB: I cannot search on the virNodeDevCapType because it's actually a "subtype" of the 'scsi_host' type (as seen in virNodeDeviceHasCap) The virNodeDeviceHasCap should also alter those hardcoded STREQ values to use the virNodeDevCapTypeToString for VIR_NODE_DEV_CAP_FC_HOST and VIR_NODE_DEV_CAP_VPORTS... I'll squash the following in (sorry - inline and will mess with mail window widths, but I tested this it works): diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 42f6593..b13cb6b 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -75,14 +75,19 @@ virNodeDevCapsDefParseString(const char *xpath, int virNodeDeviceHasCap(const virNodeDeviceObj *dev, const char *cap) { virNodeDevCapsDefPtr caps = dev->def->caps; + const char *fc_host_cap = + virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST); + const char *vports_cap = + virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); + while (caps) { if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) return 1; else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) - if ((STREQ(cap, "fc_host") && + if ((STREQ(cap, fc_host_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || - (STREQ(cap, "vports") && + (STREQ(cap, vports_cap) && (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) return 1; @@ -1851,9 +1856,10 @@ virNodeDeviceFindVportParentHost(virNodeDeviceObjListPtr devs, int *parent_host) { virNodeDeviceObjPtr parent = NULL; + const char *cap = virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); int ret; - if (!(parent = virNodeDeviceFindByCap(devs, "vports"))) { + if (!(parent = virNodeDeviceFindByCap(devs, cap))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find any vport capable device")); return -1;

...

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

Removed... I think the point of them was it'll be capable, but there's no check if there's enough ports remaining for use. That'll fail differently though. John

...

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

ACK

Jan

John, I have a concern regarding usage of the parent_fabric_name. As far I have been told there are a lot of fc_host attributes in Linux that are optional and left to the low-level driver to decide if implemented. fabric_name apparently happens to be one of these attributes and I know an architecture that has a driver (zfcp) that does not provide it. Up until this patch the fabric_name was collected on the host during detection of the fc_host capability and provided during a nodedev dumpxml. I would like to propose making the existence of fabric_name option in the detection of the fc_host capability. In doing so it would also be required make the fabric_name xml tag optional in the xml of the nodedev dump. I am aware that this is a creating a slightly incompatible output but how else could that be fixed than making the tag optional? I can send a patch for this if you agree to the solution. The implications regarding the search by parent_fabric_wwn should be obvious. On 11/18/2016 03:26 PM, 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 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@redhat.com> --- 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)

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 01/02/2017 09:25 AM, Ján Tomko wrote:

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?

I took the easy way out, but it seems the desire is one of the following: <parent>scsi_hostN</parent> <parent wwnn='' wwpn=''/> <parent fabric_name=''/> This appears to be "doable" (again apologies in advance for the cut-n-paste and email reader issues... I can repost the entire series, but figured I'd at least take a stab first to make sure we're on the same page): diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index f76204e..4666c9c 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -16,20 +16,7 @@ <element name="path"><text/></element> </optional> <optional> - <element name="parent"><text/></element> - </optional> - <optional> - <element name="parent_wwnn"> - <ref name='wwn'/> - </element> - <element name="parent_wwpn"> - <ref name='wwn'/> - </element> - </optional> - <optional> - <element name="parent_fabric_wwn"> - <ref name='wwn'/> - </element> + <ref name="parent"/> </optional> <optional> @@ -44,6 +31,28 @@ </element> </define> + <element name='parent'> + <optional> + <attribute name='wwnn'> + <ref name='wwn'/> + </attribute> + <attribute name='wwpn'> + <ref name='wwn'/> + </attribute> + </optional> + <optional> + <attribute name='fabric_wwn'> + <ref name='wwn'/> + </attribute> + </optional> + <choice> + <text/> + <empty/> + </choice> + </element> + </define> + <define name='capability'> <element name="capability"> <choice> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index ad7b699..4d3268f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1724,15 +1724,15 @@ 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_wwnn = virXPathString("string(./parent[1]/@wwnn)", ctxt); + def->parent_wwpn = virXPathString("string(./parent[1]/@wwpn)", ctxt); if ((def->parent_wwnn && !def->parent_wwpn) || (!def->parent_wwnn && def->parent_wwpn)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("must supply both wwnn and wwpn for parent")); goto error; } - def->parent_fabric_wwn = virXPathString("string(./parent_fabric_wwn[1])", + def->parent_fabric_wwn = virXPathString("string(./parent[1]/@fabric_wwn)", ctxt); /* Parse device capabilities */

...

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@redhat.com> --- 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.

OK

...

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

Sure - I can add that - I did it in a separate local commit before making the above change... John

...

+ 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

On Fri, Nov 18, 2016 at 09:26:33AM -0500, John Ferlan wrote:

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@redhat.com> --- 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;

These returns leak name, but the following patch fixes that. ACK Jan

On Fri, Nov 18, 2016 at 09:26:34AM -0500, John Ferlan wrote:

Remove duplicated code - make one path through

Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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. - */

Don't we need to preserve this note for future generations?

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

ACK Jan

On 01/02/2017 09:51 AM, Ján Tomko wrote:

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@redhat.com> --- 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.

Not quite sure why it's felt 'p' wouldn't need changing. On my test system the "port_name", "node_name", and "fabric_wwn" files each have "0x######\n", but the stored/XML format will be ###### Maybe it's just late and I'm overthinking it.

...

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

So I went with a function: /* virReadCompareWWN * @prefix: path to the wwn file * @d_name: name of the current directory * @f_name: file name to read * * Read/compare the on-disk file with the passed wwn value. * * Returns: * -1 : Error * 0 : No match * 1 : Match */ static int virReadCompareWWN(const char *prefix, const char *d_name, const char *f_name, const char *wwn) { char *path; char *buf = NULL; char *p; int ret = -1; if (virAsprintf(&path, "%s/%s/%s", prefix, d_name, f_name) < 0) return -1; if (!virFileExists(path)) { ret = 0; goto cleanup; } if (virFileReadAll(path, 1024, &buf) < 0) goto cleanup; if ((p = strchr(buf, '\n'))) *p = '\0'; if (STRPREFIX(buf, "0x")) p = buf + strlen("0x"); else p = buf; if (STRNEQ(wwn, p)) ret = 0; else ret = 1; cleanup: VIR_FREE(path); VIR_FREE(buf); return ret; } It should be obvious what the callers do, but again I can post an updated series as long as you're generally fine with this. Tks - John

On Fri, Nov 18, 2016 at 09:26:37AM -0500, John Ferlan wrote:

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@redhat.com> --- 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"),

Why the error message change? ACK to the rest. Jan

On 12/12/2016 07:50 PM, Jim Fehlig wrote:

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.

Thanks - that's good to know...

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?

I suppose for NPIV/vHBA LUN's it perhaps makes sense to have inactive paths. Been a while since I went down that path... Seems like virStorageBackendSCSINewLun should be able to receive a -2 in order to ignore VIR_STORAGE_VOL_READ_NOERROR. There's a sequence of changes I made starting with commit id '22346003d' which allow/support a -2 return. Those lead to a linked bz in the commit message describing a problem with multipath device with 'active' and 'ghost' devices. I'd have to dig more on that to jiggle the memory. Perhaps it's a path that needs a bit more code to more properly determine details of the LUN returning -2... IOW: Something that would dig on the failed device to see if it was mpath related. John

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: