References: fate#319621

Subject: xmlbuilder: More comments for _build_xpath_node
From: Cole Robinson crobinso@redhat.com Mon Jul 18 13:50:25 2016 -0400
Date: Mon Jul 18 14:46:50 2016 -0400:
Git: 835ddc5f7710d195a8d069358693712e68f2b353


Index: virt-manager-1.4.0/virtinst/xmlbuilder.py
===================================================================
--- virt-manager-1.4.0.orig/virtinst/xmlbuilder.py
+++ virt-manager-1.4.0/virtinst/xmlbuilder.py
@@ -158,44 +158,56 @@ def _add_pretty_child(parentnode, newnod
 
 def _build_xpath_node(ctx, xpath):
     """
-    Build all nodes required to set an xpath. If we have XML <foo/>, and want
-    to set xpath /foo/bar/baz@booyeah, we create node 'bar' and 'baz'
-    returning the last node created.
-    """
-    parentpath = ""
-    parentnode = None
+    Build all nodes for the passed xpath. For example, if 'ctx' xml=<foo/>,
+    and xpath=./bar/@baz, after this function the 'ctx' XML will be:
 
-    nodelist = xpath.split("/")
-    for nodename in nodelist:
-        if not nodename:
-            continue
+      <foo>
+        <bar baz=''/>
+      </foo>
+
+    And the node pointing to @baz will be returned, for the caller to
+    do with as they please.
+    """
+    def _handle_node(nodename, parentnode, parentpath):
+        # If the passed xpath snippet (nodename) exists, return the node
+        # If it doesn't exist, create it, and return the new node
 
-        # If xpath is a node property, set it and move on
+        # If nodename is a node property, we can handle it up front
         if nodename.startswith("@"):
             nodename = nodename.strip("@")
-            parentnode = parentnode.setProp(nodename, "")
-            continue
+            return parentnode.setProp(nodename, ""), parentpath
 
         if not parentpath:
             parentpath = nodename
         else:
             parentpath += "/%s" % nodename
 
-        # Node found, nothing to create for now
+        # See if the xpath node already exists
         node = _get_xpath_node(ctx, parentpath)
         if node:
-            parentnode = node
-            continue
+            # xpath node already exists, so we don't need to create anything
+            return node, parentpath
 
+        # If we don't have a parentnode by this point, the root of the
+        # xpath didn't find anything. Usually a coding error
         if not parentnode:
             raise RuntimeError("Could not find XML root node")
 
-        # Remove conditional xpath elements for node creation
+        # Remove conditional xpath elements for node creation. We preserved
+        # them up until this point since it was needed for proper xpath
+        # lookup, but they aren't valid syntax when creating the node
         if nodename.count("["):
             nodename = nodename[:nodename.index("[")]
 
         newnode = libxml2.newNode(nodename)
-        parentnode = _add_pretty_child(parentnode, newnode)
+        return _add_pretty_child(parentnode, newnode), parentpath
+
+
+    # Split the xpath and lookup/create each individual piece
+    parentpath = None
+    parentnode = None
+    for nodename in xpath.split("/"):
+        parentnode, parentpath = _handle_node(nodename, parentnode, parentpath)
 
     return parentnode