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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
