3:46 p.m.
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(a)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