VirtualAddress: Convert to new style XML properties

This adds a bunch of extra XML parsing infrastructure to make everything
work the same.

tests/xmlparse-xml/change-addr-in.xml

@@ -22,6 +22,11 @@
       <address type='drive' controller='3' bus='5' unit='33'/>
       <alias name='foo2'/>
     </disk>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest2'/>
+      <target dev='hdb' bus='ide'/>
+      <address type='drive' controller='4' bus='5' unit='33'/>
+    </disk>
     <controller type='scsi' index='8'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/>
     </controller>

tests/xmlparse-xml/change-addr-out.xml

@@ -21,8 +21,12 @@
       <target dev="hda" bus="ide"/>
       <address type="drive" controller="1" bus="4" unit="32"/>
     </disk>
+    <disk type="block" device="disk">
+      <source dev="/dev/HostVG/QEMUGuest2"/>
+      <target dev="hdb" bus="ide"/>
+    </disk>
     <controller type="scsi" index="8">
-      <address type="pci" domain="0x0001" bus="4" slot="10" function="0x6"/>
+      <address type="pci" domain="1" bus="4" slot="10" function="6"/>
       <alias name="frob"/>
     </controller>
     <channel type="pty">

tests/xmlparse.py

@@ -634,33 +634,38 @@ class XMLParseTest(unittest.TestCase):
         dev1 = guest.get_devices("disk")[0]
         dev2 = guest.get_devices("controller")[0]
         dev3 = guest.get_devices("channel")[0]
+        dev4 = guest.get_devices("disk")[1]
 
         check = self._make_checker(dev1.address)
         check("type", "drive", "pci")
         check("type", "pci", "drive")
-        check("controller", "3", "1")
+        check("controller", 3, 1)
-        check("bus", "5", "4")
+        check("bus", 5, 4)
-        check("unit", "33", "32")
+        check("unit", 33, 32)
         check = self._make_checker(dev1.alias)
         check("name", "foo2", None)
 
         check = self._make_checker(dev2.address)
+        dev2.address.domain = "0x0010"
+        self.assertEqual(dev2.address.domain, 16)
         check("type", "pci")
-        check("domain", "0x0000", "0x0001")
+        check("domain", 16, 1)
-        check("bus", "0x00", "4")
+        check("bus", 0, 4)
-        check("slot", "0x04", "10")
+        check("slot", 4, 10)
-        check("function", "0x7", "0x6")
+        check("function", 7, 6)
         check = self._make_checker(dev2.alias)
         check("name", None, "frob")
 
         check = self._make_checker(dev3.address)
         check("type", "virtio-serial")
-        check("controller", "0")
+        check("controller", 0)
-        check("bus", "0")
+        check("bus", 0)
-        check("port", "2", "4")
+        check("port", 2, 4)
         check = self._make_checker(dev3.alias)
         check("name", "channel0", "channel1")
 
+        dev4.address.clear()
+
         self._alter_compare(guest.get_xml_config(), outfile)
 
     def testAlterSmartCard(self):

virtinst/VirtualController.py

@@ -141,7 +141,8 @@ class VirtualController(VirtualDevice):
             xml += " model='%s'" % self.model
         xml += extra
         childxml = self.indent(self._master.get_xml_config(), 6)
-        childxml += self.indent(self.address.get_xml_config(), 6)
+        if childxml:
+            childxml += "\n"
         if len(childxml) == 0:
             return xml + "/>"
         xml += ">\n"

virtinst/VirtualDevice.py

@@ -70,6 +70,7 @@ class VirtualDevice(XMLBuilder):
 
     # General device type (disk, interface, etc.)
     _virtual_device_type = None
+    _XML_INDENT = 4
 
     def __init__(self, conn, parsexml=None, parsexmlnode=None):
         """
@@ -78,13 +79,11 @@ class VirtualDevice(XMLBuilder):
         @param conn: libvirt connection to validate device against
         """
         XMLBuilder.__init__(self, conn, parsexml, parsexmlnode)
+        self._XML_ROOT_NAME = self._virtual_device_type
 
-        self.alias = VirtualDeviceAlias(conn,
+        self.alias = VirtualDeviceAlias(conn, parsexmlnode=parsexmlnode)
-                                        parsexml=parsexml,
+        self.address = VirtualDeviceAddress(conn, parsexmlnode=parsexmlnode)
-                                        parsexmlnode=parsexmlnode)
+        self._XML_SUB_ELEMENTS = ["alias", "address"]
-        self.address = VirtualDeviceAddress(conn,
-                                            parsexml=parsexml,
-                                            parsexmlnode=parsexmlnode)
 
         if not self._virtual_device_type:
             raise ValueError(_("Virtual device type must be set in subclass."))
@@ -98,10 +97,6 @@ class VirtualDevice(XMLBuilder):
         return self._virtual_device_type
     virtual_device_type = property(get_virtual_device_type)
 
-    def _get_xml_config(self):
-        # See XMLBuilder for docs
-        raise NotImplementedError()
-
     def setup(self, meter=None):
         """
         Perform potentially hazardous device initialization, like
@@ -114,27 +109,24 @@ class VirtualDevice(XMLBuilder):
         return
 
     def set_address(self, addrstr):
-        self.address = VirtualDeviceAddress(self.conn, addrstr=addrstr)
+        self.address.set_addrstr(addrstr)
 
 
 class VirtualDeviceAlias(XMLBuilder):
-    def __init__(self, conn, parsexml=None, parsexmlnode=None):
+    _XML_ROOT_NAME = "alias"
-        XMLBuilder.__init__(self, conn, parsexml, parsexmlnode)
+    _XML_INDENT = 0
 
-        self._name = None
+    name = XMLProperty(xpath="./alias/@name")
-
-
-    def _get_name(self):
-        return self._name
-    def _set_name(self, val):
-        self._name = val
-    name = XMLProperty(_get_name, _set_name, xpath="./alias/@name")
-
-    def _get_xml_config(self):
-        return ""
 
 
 class VirtualDeviceAddress(XMLBuilder):
+    """
+    Examples:
+    <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+    <address type='drive' controller='0' bus='0' unit='0'/>
+    <address type='ccid' controller='0' slot='0'/>
+    <address type='virtio-serial' controller='1' bus='0' port='4'/>
+    """
 
     ADDRESS_TYPE_PCI           = "pci"
     ADDRESS_TYPE_DRIVE         = "drive"
@@ -146,36 +138,12 @@ class VirtualDeviceAddress(XMLBuilder):
              ADDRESS_TYPE_VIRTIO_SERIAL, ADDRESS_TYPE_CCID,
              ADDRESS_TYPE_SPAPR_VIO]
 
-    def __init__(self, conn, parsexml=None, parsexmlnode=None,
+    _XML_ROOT_NAME = "address"
-                 addrstr=None):
+    _XML_INDENT = 0
-        XMLBuilder.__init__(self, conn, parsexml, parsexmlnode)
+    _XML_XPATH_RELATIVE = True
+    _XML_PROP_ORDER = ["type", "domain", "bus", "slot", "function"]
 
-        self._type = None
+    def set_addrstr(self, addrstr):
-
-        # PCI address:
-        # <address type='pci' domain='0x0000' bus='0x00' slot='0x04' \
-        #                     function='0x0'/>
-        self._bus = None
-        self._domain = None
-        self._slot = None
-        self._function = None
-
-        # Drive address:
-        # <address type='drive' controller='0' bus='0' unit='0'/>
-        self._controller = None
-        self._unit = None
-
-        # VirtioSerial address:
-        # <address type='virtio-serial' controller='1' bus='0' port='4'/>
-        self._port = None
-
-        # CCID address:
-        # <address type='ccid' controller='0' slot='0'/>
-
-        if addrstr:
-            self.parse_friendly_address(addrstr)
-
-    def parse_friendly_address(self, addrstr):
         try:
             if addrstr.count(":") in [1, 2] and addrstr.count("."):
                 self.type = self.ADDRESS_TYPE_PCI
@@ -187,90 +155,29 @@ class VirtualDeviceAddress(XMLBuilder):
             elif addrstr == "spapr-vio":
                 self.type = self.ADDRESS_TYPE_SPAPR_VIO
             else:
-                raise ValueError(_("Could not determine or unsupported format of '%s'") % addrstr)
+                raise ValueError(_("Could not determine or unsupported "
+                                   "format of '%s'") % addrstr)
         except:
             logging.exception("Error parsing address.")
             return None
 
 
     def clear(self):
-        self._type = None
+        self.type = None
-        self._bus = None
+        self.bus = None
-        self._domain = None
+        self.domain = None
-        self._slot = None
+        self.slot = None
-        self._function = None
+        self.function = None
-        self._controller = None
+        self.controller = None
-        self._unit = None
+        self.unit = None
-        self._port = None
+        self.port = None
 
-        if self._is_parse():
-            self._remove_child_xpath("./address")
 
-    def _get_type(self):
+    type = XMLProperty(xpath="./address/@type")
-        return self._type
+    domain = XMLProperty(xpath="./address/@domain", is_int=True)
-    def _set_type(self, val):
+    bus = XMLProperty(xpath="./address/@bus", is_int=True)
-        self._type = val
+    slot = XMLProperty(xpath="./address/@slot", is_int=True)
-    type = XMLProperty(_get_type, _set_type, xpath="./address/@type")
+    function = XMLProperty(xpath="./address/@function", is_int=True)
-
+    controller = XMLProperty(xpath="./address/@controller", is_int=True)
-    def _get_domain(self):
+    unit = XMLProperty(xpath="./address/@unit", is_int=True)
-        return self._domain
+    port = XMLProperty(xpath="./address/@port", is_int=True)
-    def _set_domain(self, val):
-        self._domain = val
-    domain = XMLProperty(_get_domain, _set_domain, xpath="./address/@domain")
-
-    def _get_bus(self):
-        return self._bus
-    def _set_bus(self, val):
-        self._bus = val
-    bus = XMLProperty(_get_bus, _set_bus, xpath="./address/@bus")
-
-    def _get_slot(self):
-        return self._slot
-    def _set_slot(self, val):
-        self._slot = val
-    slot = XMLProperty(_get_slot, _set_slot, xpath="./address/@slot")
-
-    def _get_function(self):
-        return self._function
-    def _set_function(self, val):
-        self._function = val
-    function = XMLProperty(_get_function, _set_function,
-                             xpath="./address/@function")
-
-    def _get_controller(self):
-        return self._controller
-    def _set_controller(self, val):
-        self._controller = val
-    controller = XMLProperty(_get_controller, _set_controller,
-                               xpath="./address/@controller")
-
-    def _get_unit(self):
-        return self._unit
-    def _set_unit(self, val):
-        self._unit = val
-    unit = XMLProperty(_get_unit, _set_unit, xpath="./address/@unit")
-
-    def _get_port(self):
-        return self._port
-    def _set_port(self, val):
-        self._port = val
-    port = XMLProperty(_get_port, _set_port, xpath="./address/@port")
-
-    def _get_xml_config(self):
-        if not self.type:
-            return
-
-        def format_props(*args):
-            return "".join([" %s='%s'" % (k, getattr(self, k)) for k in args if getattr(self, k, None) is not None])
-
-        xml = "<address type='%s'" % self.type
-        if self.type == self.ADDRESS_TYPE_PCI:
-            xml += format_props("domain", "bus", "slot", "function")
-        elif self.type == self.ADDRESS_TYPE_DRIVE:
-            xml += format_props("controller", "bus", "unit")
-        elif self.type == self.ADDRESS_TYPE_VIRTIO_SERIAL:
-            xml += format_props("controller", "bus", "port")
-        elif self.type == self.ADDRESS_TYPE_CCID:
-            xml += format_props("controller", "slot")
-        xml += "/>"
-        return xml

virtinst/VirtualDisk.py

@@ -389,9 +389,11 @@ class VirtualDisk(VirtualDevice):
             raise ValueError(_("Couldn't lookup volume object: %s" % str(e)))
 
 
-    _XMLELEMENTORDER = ["driver", "source", "target"]
+
-    _XMLPROPORDER = ["target", "bus", "type", "device",
+
-                     "driver_name", "driver_type"]
+    _XML_ELEMENT_ORDER = ["driver", "source", "target"]
+    _XML_PROP_ORDER = ["target", "bus", "type", "device",
+                       "driver_name", "driver_type"]
 
     def __init__(self, conn, parsexml=None, parsexmlnode=None):
         VirtualDevice.__init__(self, conn, parsexml, parsexmlnode)
@@ -715,14 +717,6 @@ class VirtualDisk(VirtualDevice):
                              if "<driver" not in l])
         return xml
 
-    def _get_xml_config(self):
-        ret = "    <disk>\n"
-        addr = self.indent(self.address.get_xml_config(), 6)
-        if addr:
-            ret += addr
-        ret += "    </disk>"
-        return ret
-
     def is_size_conflict(self):
         """
         reports if disk size conflicts with available space

virtinst/VirtualNetworkInterface.py

@@ -353,7 +353,6 @@ class VirtualNetworkInterface(VirtualDevice):
         src_xml = ""
         model_xml = ""
         target_xml = ""
-        addr_xml = ""
         if self.type == self.TYPE_BRIDGE:
             src_xml     = "      <source bridge='%s'/>\n" % self.bridge
         elif self.type == self.TYPE_VIRTUAL:
@@ -366,9 +365,6 @@ class VirtualNetworkInterface(VirtualDevice):
         if self.model:
             model_xml   = "      <model type='%s'/>\n" % self.model
 
-        if self.address:
-            addr_xml = self.indent(self.address.get_xml_config(), 6)
-
         if self.target_dev:
             target_xml  = "      <target dev='%s'/>\n" % self.target_dev
 
@@ -377,6 +373,5 @@ class VirtualNetworkInterface(VirtualDevice):
         xml += "      <mac address='%s'/>\n" % self.macaddr
         xml += target_xml
         xml += model_xml
-        xml += addr_xml
         xml += "    </interface>"
         return xml

virtinst/xmlbuilder.py

@@ -26,6 +26,9 @@ import libxml2
 
 from virtinst import util
 
+# pylint: disable=W0212
+# This whole file is calling around into non-public functions that we
+# don't want regular API users to touch
 
 _trackprops = bool("VIRTINST_TEST_TRACKPROPS" in os.environ)
 _allprops = []
@@ -124,18 +127,18 @@ def _build_xpath_node(ctx, xpath, addnode=None):
             if node_is_text(prevsib):
                 sib = libxml2.newText(prevsib.content)
             else:
-                sib = libxml2.newText("")
+                sib = libxml2.newText("\n")
             parentnode.addChild(sib)
 
-        # This is case is adding a child element to an already properly
+        # This case is adding a child element to an already properly
         # spaced element. Example:
         # <features>
-        #  <acpi/>
+        #   <acpi/>
         # </features>
         # to
         # <features>
-        #  <acpi/>
+        #   <acpi/>
-        #  <apic/>
+        #   <apic/>
         # </features>
         sib = parentnode.get_last()
         content = sib.content
@@ -346,14 +349,23 @@ class XMLProperty(property):
             ret = self._xpath_for_getter_cb(xmlbuilder)
         if ret is None:
             raise RuntimeError("%s: didn't generate any setter xpath." % self)
-        return ret
+        return self._xpath_fix_relative(xmlbuilder, ret)
     def _xpath_for_setter(self, xmlbuilder):
         ret = self._xpath
         if self._xpath_for_setter_cb:
             ret = self._xpath_for_setter_cb(xmlbuilder)
         if ret is None:
             raise RuntimeError("%s: didn't generate any setter xpath." % self)
-        return ret
+        return self._xpath_fix_relative(xmlbuilder, ret)
+    def _xpath_fix_relative(self, xmlbuilder, xpath):
+        if not getattr(xmlbuilder, "_xml_fixup_relative_xpath"):
+            return xpath
+        root = "./%s" % getattr(xmlbuilder, "_XML_ROOT_NAME")
+        if not xpath.startswith(root):
+            raise RuntimeError("%s: xpath did not start with root=%s" %
+                               (str(self), root))
+        return "." + xpath[len(root):]
+
 
     def _xpath_list_for_setter(self, xpath, setval, nodelist):
         if not self._is_multi:
@@ -418,7 +430,10 @@ class XMLProperty(property):
                 return None
             return bool(val)
         elif self._is_int and val is not None:
-            return int(val)
+            base = 10
+            if "0x" in str(val):
+                base = 16
+            return int(val, base=base)
         elif self._convert_value_for_getter_cb:
             return self._convert_value_for_getter_cb(xmlbuilder, val)
         elif self._is_multi and val is None:
@@ -562,16 +577,31 @@ class XMLBuilder(object):
         xml = ""
         if not xmlstr:
             return xml
-
+        if not level:
-        for l in iter(xmlstr.splitlines()):
+            return xmlstr
-            xml += " " * level + l + "\n"
+        return "\n".join((" " * level + l) for l in xmlstr.splitlines())
-        return xml
 
     # Specify a list of tag values here and we will arrange them when
     # outputing XML. They will be put before every other element. This
     # is strictly to keep test suite happy.
-    _XMLELEMENTORDER = []
+    _XML_ELEMENT_ORDER = []
-    _XMLPROPORDER = []
+    _XML_PROP_ORDER = []
+
+    # Root element name of this function, used to populate a default
+    # _get_xml_config
+    _XML_ROOT_NAME = None
+
+    # Integer indentation level for generated XML.
+    _XML_INDENT = None
+
+    # If XML xpaths are relative to a different element, like
+    # device addresses.
+    _XML_XPATH_RELATIVE = False
+
+    # List of property names that point to a manually tracked
+    # XMLBuilder that alters our device xml, like self.address for
+    # VirtualDevice
+    _XML_SUB_ELEMENTS = []
 
     _dumpxml_xpath = "."
     def __init__(self, conn, parsexml=None, parsexmlnode=None):
@@ -589,6 +619,7 @@ class XMLBuilder(object):
         self._xml_node = None
         self._xml_ctx = None
         self._xml_root_doc = None
+        self._xml_fixup_relative_xpath = False
         self._propstore = {}
         self._proporder = []
 
@@ -601,8 +632,6 @@ class XMLBuilder(object):
     ##############
 
     def copy(self):
-        # pylint: disable=W0212
-        # Access to protected member, needed to unittest stuff
         ret = copy.copy(self)
         ret._propstore = ret._propstore.copy()
         ret._proporder = ret._proporder[:]
@@ -634,7 +663,19 @@ class XMLBuilder(object):
             else:
                 ret = _sanitize_libxml_xml(node.serialize())
         else:
-            ret = self._add_parse_bits(self._get_xml_config(*args, **kwargs))
+            try:
+                self._xml_fixup_relative_xpath = self._XML_XPATH_RELATIVE
+                xmlstub = self._make_xml_stub(fail=False)
+                ret = self._get_xml_config(*args, **kwargs)
+                ret = self._add_parse_bits(ret)
+
+                for propname in self._XML_SUB_ELEMENTS:
+                    ret = getattr(self, propname)._add_parse_bits(ret)
+
+                if ret == xmlstub:
+                    ret = ""
+            finally:
+                self._xml_fixup_relative_xpath = False
 
         ret = self._order_xml_elements(ret)
         return self._cleanup_xml(ret)
@@ -666,7 +707,7 @@ class XMLBuilder(object):
         """
         Internal XML building function. Must be overwritten by subclass
         """
-        raise NotImplementedError()
+        return self._make_xml_stub(fail=True)
 
     def _cleanup_xml(self, xml):
         """
@@ -679,6 +720,20 @@ class XMLBuilder(object):
     # Internal XML parsers #
     ########################
 
+    def _make_xml_stub(self, fail=True):
+        if self._XML_ROOT_NAME is None:
+            if not fail:
+                return None
+            raise RuntimeError("Must specify _XML_ROOT_NAME.")
+        if self._XML_INDENT is None:
+            if not fail:
+                return None
+            raise RuntimeError("Must specify _XML_INDENT.")
+        if self._XML_ROOT_NAME == "":
+            return ""
+
+        return self.indent("<%s/>" % (self._XML_ROOT_NAME), self._XML_INDENT)
+
     def _add_child_node(self, parent_xpath, newnode):
         ret = _build_xpath_node(self._xml_ctx, parent_xpath, newnode)
         return ret
@@ -717,12 +772,20 @@ class XMLBuilder(object):
         if not self._propstore:
             return xml
 
+        # Unindent XML
+        indent = 0
+        for c in xml:
+            if c != " ":
+                break
+            indent += 1
+        xml = "\n".join([l[indent:] for l in xml.splitlines()])
+
         # Parse the XML into our internal state
         self._parsexml(xml, None)
 
         # Set up preferred XML ordering
         do_order = self._proporder[:]
-        for key in reversed(self._XMLPROPORDER):
+        for key in reversed(self._XML_PROP_ORDER):
             if key in do_order:
                 do_order.remove(key)
                 do_order.insert(0, key)
@@ -730,14 +793,7 @@ class XMLBuilder(object):
         # Alter the XML
         for key in do_order:
             setattr(self, key, self._propstore[key])
-
+        return self.indent(self.get_xml_config().strip("\n"), indent)
-        # Fix initial indentation
-        ret = self.get_xml_config()
-        for c in xml:
-            if c != " ":
-                break
-            ret = " " + ret
-        return ret.strip("\n")
 
     def _add_parse_bits(self, xml):
         """
@@ -763,7 +819,7 @@ class XMLBuilder(object):
     def _order_xml_elements(self, xml):
         # This whole thing is reeeally hacky but it saves us some
         # unittest churn.
-        if not self._XMLELEMENTORDER:
+        if not self._XML_ELEMENT_ORDER:
             return xml
 
         split = xml.splitlines()
@@ -781,7 +837,7 @@ class XMLBuilder(object):
             baseindent += 1
 
         neworder = []
-        for prio in reversed(self._XMLELEMENTORDER):
+        for prio in reversed(self._XML_ELEMENT_ORDER):
             tag = "%s<%s " % ((baseindent + 2) * " ", prio)
             for idx in range(len(split)):
                 if split[idx].startswith(tag):