Milan Zamazal has posted comments on this change.

Change subject: virt: Make DomainDescriptor use XML helpers

Patch Set 8:


Thank you all for the comments and suggestions, I hope things get clarified and 
we can make a progress. If we can agree on using default value argument in 
*find* then the major concept issues are probably resolved.

Let's also not forget we can either discard the API or continue using it after 
the final switch to etree. I prefer making a clean API, because in case we 
decide to keep it or we stop before the final switch to etree for some serious 
reason, we can use the API without worries. But even then the API needn't be 
engraved into a stone at this moment, I think making progress beyond this patch 
is more important.
File vdsm/virt/

Line 78:         result = [element] + result
Line 79:     return result
Line 80: 
Line 81: 
Line 82: def dom_find_tag(element, tag, index=0):
> I'm not convinced by the argument about common patterns in client code.
I also don't like having both find_xxx and require_xxx, although for slightly 
different reasons: 1. It's unnecessary to double the number of helpers when we 
can simply use an argument; 2. it's easy to miss that find_xxx may return None.

I like the suggestion about having a default argument. It solves all the 
problems (and obsoletes the need for `required' argument).
Line 83:     """
Line 84:     Find a DOM element specified by given arguments.
Line 85: 
Line 86:     :param element: DOM object to be searched for given `tag`

Line 98:     except IndexError:
Line 99:         return None
Line 100: 
Line 101: 
Line 102: def dom_find_attr(element, tag, attribute=True, index=0):
> Why have an api accepting element when we want to find the attribute of a s

  dom_find_attr(element, 'foo', 'bar')

is easier to use than

  dom_attr(dom_find_tag(element, 'foo'), 'bar')


  tag_element = dom_find_tag(element, 'foo')
  dom_attr(tag_element, 'bar')

or even

  tag_element = dom_find_tag(element, 'foo', default=None)
  if tag_element is not None:
       dom_attr(tag_element, 'bar')
(consider something like

  if dom_find_attr(element, 'foo', 'bar', default=None) == 'baz':
Line 103:     """
Line 104:     Find attribute values of a DOM element specified by given 
Line 105: 
Line 106:     :param element: DOM object to be searched for given `tag`

Line 125:     if subelement is None:
Line 126:         if isinstance(attribute, basestring):
Line 127:             return ''
Line 128:         else:
Line 129:             return {}
> Why we should care about argument type? Why should be return different retu
It's too complicated, I should use separate methods here instead of switching 
on the argument type.
Line 130:     return dom_attr(subelement, attribute)
Line 131: 
Line 132: 
Line 133: def dom_tag(element):

Line 139:     :returns: tag of the element
Line 140:     :rtype: basestring
Line 141: 
Line 142:     """
Line 143:     return element.tagName
> Why do we need this? what is the etree way that this should hide?
We need it for the minidom->etree transition period (at least), because it's 
different in minidom and etree.
Line 144: 
Line 145: 
Line 146: def dom_attr(element, attribute):
Line 147:     """

Line 151:     :type element: DOM object
Line 152:     :param attribute: attribute name to look for, or a sequence of 
Line 153:       names to look for, or True to return all attribute values of 
the given
Line 154:       element
Line 155:     :type attribute: basestring, or sequence of basestrings, or True
> Please accept only one type for attribute, this does not make any sense. Ac
I agree, it should be split into two helpers: 1. for retrieving just a simple 
attribute; 2. for retrieving multiple attributes, perhaps in the way you 
suggest in another comment.
Line 156:     :returns: the corresponding attribute values or empty string (if
Line 157:       `attribute` is basestring and no corresponding attribute is 
Line 158:     :rtype: basestring if `attribute` is string; dictionary of
Line 159:       attribute names as keys and their corresponding values otherwise

Line 180:     :rtype: basestring or NoneType
Line 181: 
Line 182:     """
Line 183:     if element is None:
Line 184:         return None
> This is very bad idea, result of having functions returning None. The calle
OK, I think it's used in only two places now so it doesn't matter much and we 
can add the extra condition to that places.
Line 185:     child_node = element.firstChild
Line 186:     if child_node is None:
Line 187:         return None
Line 188:     return child_node.nodeValue

Line 184:         return None
Line 185:     child_node = element.firstChild
Line 186:     if child_node is None:
Line 187:         return None
Line 188:     return child_node.nodeValue
> Why not return always a string? If there is not child node, return empty st
Yes, let's always return string from this method.
Line 189: 
Line 190: 
Line 191: def has_channel(domXML, name):
Line 192:     domObj = etree.fromstring(domXML)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib169735936d19171ff8b8d127666d7258c308f34
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <>
Gerrit-Reviewer: Francesco Romani <>
Gerrit-Reviewer: Martin Polednik <>
Gerrit-Reviewer: Milan Zamazal <>
Gerrit-Reviewer: Nir Soffer <>
Gerrit-Reviewer: gerrit-hooks <>
Gerrit-HasComments: Yes
vdsm-patches mailing list --
To unsubscribe send an email to

Reply via email to