Re: [PATCH 04/11] nodedev: Add the ability to create vHBA by parent wwnn/wwpn or fabric_wwn

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux