Milan Zamazal has uploaded a new change for review.

Change subject: virt: Make DomainDescriptor use XML helpers
......................................................................

virt: Make DomainDescriptor use XML helpers

We are going to stop using xml.dom.minidom and to use xml.etree instead.
XML processing is currently spread across several places and we use both
minidom and etree, so we must be careful with the conversion.  The
overall strategy is as follows:

1. Create XML helpers abstracting common XML operations.
2. Use the helpers everywhere where minidom is currently used.  Avoid
   using minidom anywhere outside the XML helpers.
3. Switch from minidom to etree in the XML helpers.

We avoid using XML directly in many places as a side effect.  It is
debatable whether it is a good or bad thing.  If we decide it is a good
thing, we may want to replace direct etree base XML processing with the
helpers as well in a followup patch series.  If we decide it is a bad
thing, we can unfold the helpers in a followup patch series (note we
need the helpers as an intermediate step and can't avoid them from the
beginning).  Keeping status quo after this series is also an option.

There's also duplicate XML code and processing present in some places,
especially in core devices.  We keep it duplicated as it is in this
series, in order not to mix etree conversion with refactoring.  Device
XML processing refactoring is planned in a followup patch series.

This patch implements the first step of the whole process.  It converts
DomainDescriptor class from minidom to XML helpers.  The reason for this
start is that DomainDescriptor class is simple while it requires most of
the needed XML helpers.  So we can start with a small change of the
existent code while becoming equipped with a nice set XML helpers for
the following, perhaps more complicated, patches.

Change-Id: Ib169735936d19171ff8b8d127666d7258c308f34
Signed-off-by: Milan Zamazal <mzama...@redhat.com>
---
M tests/vmXmlTests.py
M vdsm/virt/domain_descriptor.py
M vdsm/virt/vm.py
M vdsm/virt/vmxml.py
4 files changed, 216 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/69/55769/9

diff --git a/tests/vmXmlTests.py b/tests/vmXmlTests.py
index c3e8f05..3051c63 100644
--- a/tests/vmXmlTests.py
+++ b/tests/vmXmlTests.py
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 #
 # Copyright 2014, 2016 Red Hat, Inc.
 #
@@ -26,7 +27,7 @@
 from virt import vmxml
 
 from testlib import VdsmTestCase as TestCaseBase
-from testlib import permutations, expandPermutations
+from testlib import XMLTestCase, permutations, expandPermutations
 
 import vmfakelib as fake
 
@@ -59,6 +60,58 @@
 
 
 @expandPermutations
+class TestVmXmlHelpers(XMLTestCase):
+
+    _XML = u'''<?xml version="1.0" ?>
+    <topelement>
+      <hello lang="english">hello</hello>
+      <hello cyrillic="yes" lang="русский">здра́вствуйте</hello>
+      <bye>good bye<hello lang="čeština">dobrý den</hello></bye>
+      <empty/>
+    </topelement>
+    '''
+
+    def setUp(self):
+        self._dom = vmxml.parse_xml(self._XML)
+
+    def test_import_export(self):
+        xml = vmxml.export_xml(self._dom)
+        self.assertXMLEqual(xml, self._XML)
+
+    @permutations([[None, 'topelement', 1],
+                   ['topelement', 'topelement', 1],
+                   [None, 'hello', 3],
+                   ['topelement', 'bye', 1],
+                   [None, 'none', 0]])
+    def test_find_all(self, start_tag, tag, number):
+        dom = self._dom
+        if start_tag is not None:
+            dom = vmxml.find_first(self._dom, 'topelement')
+        elements = vmxml.find_all(dom, tag)
+        matches = [vmxml.tag(e) == tag for e in elements]
+        self.assertTrue(all(matches))
+        self.assertEqual(len(matches), number)
+
+    def test_find_first_not_found(self):
+        self.assertRaises(vmxml.NotFound, vmxml.find_first, self._dom, 'none')
+
+    @permutations([['hello', 'lang', 'english'],
+                   ['hello', 'none', ''],
+                   ['none', 'lang', ''],
+                   ])
+    def test_find_attr(self, tag, attribute, result):
+        value = vmxml.find_attr(self._dom, tag, attribute)
+        self.assertEqual(value, result)
+
+    @permutations([['hello', 'hello'],
+                   ['empty', '']])
+    def test_text(self, tag, result):
+        element = vmxml.find_first(self._dom, tag)
+        text = vmxml.text(element)
+        self.assertEqual(text, result)
+
+
+@expandPermutations
 class TestDomainDescriptor(VmXmlTestCase):
 
     @permutations([[cpuarch.X86_64], [cpuarch.PPC64]])
diff --git a/vdsm/virt/domain_descriptor.py b/vdsm/virt/domain_descriptor.py
index 2c2057f..c6f84cd 100644
--- a/vdsm/virt/domain_descriptor.py
+++ b/vdsm/virt/domain_descriptor.py
@@ -17,16 +17,16 @@
 #
 # Refer to the README and COPYING files for full details of the license
 #
-import xml.dom.minidom
+from . import vmxml
 
 
 class DomainDescriptor(object):
 
     def __init__(self, xmlStr):
         self._xml = xmlStr
-        self._dom = xml.dom.minidom.parseString(xmlStr)
-        self._devices = self._first_element_by_tag_name('devices')
-        self._devices_hash = hash(self._devices.toxml()
+        self._dom = vmxml.parse_xml(xmlStr)
+        self._devices = vmxml.find_first(self._dom, 'devices', None)
+        self._devices_hash = hash(vmxml.export_xml(self._devices)
                                   if self._devices is not None else '')
 
     @classmethod
@@ -42,31 +42,22 @@
         return self._devices
 
     def get_device_elements(self, tagName):
-        return self._devices.getElementsByTagName(tagName)
+        return vmxml.find_all(self._devices, tagName)
 
     @property
     def devices_hash(self):
         return self._devices_hash
 
     def all_channels(self):
-        for channel in self.get_device_elements('channel'):
-            try:
-                name = channel.getElementsByTagName('target')[0].\
-                    getAttribute('name')
-                path = channel.getElementsByTagName('source')[0].\
-                    getAttribute('path')
-            except IndexError:
-                continue
-            else:
+        for channel in vmxml.find_all(self._devices, 'channel'):
+            name = vmxml.find_attr(channel, 'target', 'name')
+            path = vmxml.find_attr(channel, 'source', 'path')
+            if name and path:
                 yield name, path
-
-    def _first_element_by_tag_name(self, tagName):
-        elements = self._dom.childNodes[0].getElementsByTagName(tagName)
-        return elements[0] if elements else None
 
     def get_memory_size(self):
         """
         Return the vm memory from xml in MiB
         """
-        memory = self._first_element_by_tag_name("memory")
-        return int(memory.firstChild.nodeValue) // 1024 if memory else None
+        memory = vmxml.find_first(self._dom, "memory")
+        return (int(vmxml.text(memory)) // 1024 if memory else None)
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index e24a90a..2b9e03e 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -3761,8 +3761,8 @@
         use updateDevice to select the device.
         """
         try:
-            graphics = self._domain.get_device_elements('graphics')[0]
-        except IndexError:
+            graphics = next(self._domain.get_device_elements('graphics'))
+        except StopIteration:
             return response.error('ticketErr',
                                   'no graphics devices configured')
         return self._setTicketForGraphicDev(
diff --git a/vdsm/virt/vmxml.py b/vdsm/virt/vmxml.py
index b868a25..02927d9 100644
--- a/vdsm/virt/vmxml.py
+++ b/vdsm/virt/vmxml.py
@@ -33,6 +33,155 @@
 
 _BOOT_MENU_TIMEOUT = 10000  # milliseconds
 
+_UNSPECIFIED = object()
+
+
+class NotFound(Exception):
+    """
+    Raised when vmxml helpers can't find some requested entity.
+    """
+    pass
+
+
+def parse_xml(xml_string):
+    """
+    Parse given XML string to DOM element and return the element.
+
+    :param xml_string: XML string to parse
+    :type xml_string: str
+    :returns: DOM element created by parsing `xml_string`
+    :rtype: DOM element
+
+    """
+    return xml.dom.minidom.parseString(xml_string)
+
+
+def export_xml(element):
+    """
+    Export given DOM element to XML string.
+
+    :param element: DOM element to export
+    :type element: DOM element
+    :returns: XML corresponding to `element` content
+    :rtype: basestring
+
+    """
+    return element.toxml(encoding='UTF-8')
+
+
+def find_all(element, tag_):
+    """
+    Return an iterator over all DOM elements with given `tag`.
+
+    `element` may is included in the result if it is of `tag`.
+
+    :param element: DOM element to be searched for given `tag`
+    :type element: DOM element
+    :param tag: tag to look for
+    :type tag: basestring
+    :returns: all elements with given `tag`
+    :rtype: sequence of DOM elements
+
+    """
+    if isinstance(element, xml.dom.minidom.Element) and \
+       tag(element) == tag_:
+        yield element
+    for elt in element.getElementsByTagName(tag_):
+        yield elt
+
+
+def find_first(element, tag, default=_UNSPECIFIED):
+    """
+    Find the first DOM element with the given tag.
+
+    :param element: DOM element to be searched for given `tag`
+    :type element: DOM element
+    :param tag: tag to look for
+    :type tag: basestring
+    :param default: any object to return when no element with `tag` is found;
+      if not given then `NotFound` is raised in such a case
+    :returns: the matching element or default
+    :raises: NotFound -- when no element with `tag` is found and `default` is
+      not given
+
+    """
+    try:
+        return next(find_all(element, tag))
+    except StopIteration:
+        if default is _UNSPECIFIED:
+            raise NotFound((element, tag,))
+        else:
+            return default
+
+
+def find_attr(element, tag, attribute):
+    """
+    Find `attribute` value of the first DOM element with `tag`.
+
+    :param element: DOM element to be searched for given `tag`
+    :type element: DOM element
+    :param tag: tag to look for
+    :type tag: basestring
+    :param attribute: attribute name to look for
+    :type attribute: basestring
+    :returns: the attribute value or an empty string if no element with given
+      tag is found or the found element doesn't contain `attribute`
+    :rtype: basestring
+
+    """
+    try:
+        subelement = find_first(element, tag)
+    except NotFound:
+        return ''
+    return attr(subelement, attribute)
+
+
+def tag(element):
+    """
+    Return tag of the given DOM element.
+
+    :param element: element to get the tag of
+    :type element: DOM element
+    :returns: tag of the element
+    :rtype: basestring
+
+    """
+    return element.tagName
+
+
+def attr(element, attribute):
+    """
+    Return attribute values of `element`.
+
+    :param element: the element to look the attributes in
+    :type element: DOM element
+    :param attribute: attribute name to look for
+    :type attribute: basestring
+    :returns: the corresponding attribute value or empty string (if no element
+      with `tag` or exists or `attribute` is not present)
+    :rtype: basestring
+
+    """
+    # Minidom returns unicodes, except for empty strings.
+    return element.getAttribute(attribute)
+
+
+def text(element):
+    """
+    Return text of the given DOM element.
+
+    :param element: element to get the text from
+    :type element: DOM element
+    :returns: text of the element (empty string if it the element doesn't
+      contain any text)
+    :rtype: basestring
+
+    """
+    child_node = element.firstChild
+    if child_node is None:
+        return ''
+    return child_node.nodeValue
+
 
 def has_channel(domXML, name):
     domObj = etree.fromstring(domXML)


-- 
To view, visit https://gerrit.ovirt.org/55769
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib169735936d19171ff8b8d127666d7258c308f34
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to