Change in vdsm[master]: XML helper for virt introduced

fromani at redhat.com fromani at redhat.com
Mon Feb 1 13:25:09 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: XML helper for virt introduced
......................................................................


Patch Set 1:

(6 comments)

nice job, this seems a nice way forward.

https://gerrit.ovirt.org/#/c/52821/1//COMMIT_MSG
Commit Message:

Line 11: 
Line 12: 1. Moving device XML processing out of VM class to device classes.
Line 13: 2. Switching from minidom to etree.
Line 14: 
Line 15: The rough plan is as follows:
Commenting here but I've seen all the topic branch. First thoughts, deeper discussion later.
Line 16: 
Line 17: 1. Abstract common XML operations used when handling device XML's into a
Line 18:    separate, DOM implementation independent, XML API.
Line 19: 2. Move XML processing out of DomainDescriptor and make DomainDescriptor


Line 14: 
Line 15: The rough plan is as follows:
Line 16: 
Line 17: 1. Abstract common XML operations used when handling device XML's into a
Line 18:    separate, DOM implementation independent, XML API.
this is a good way forward. In the long ran (= after the switch is made) we will not need this abstraction anymore, but during the switch this makes sense.
So I support this way forward.
Maybe we could try to bend our transitional abstract API as much as possible towards etree's API?
reasons:
1. it is what we want to use in the long run
2. it is just better than minidom's
3. there are no other API switches on the radar, so we could settle with that for a long time.
Line 19: 2. Move XML processing out of DomainDescriptor and make DomainDescriptor
Line 20:    use the new XML API.
Line 21: 3. Get rid of all XML/DOM processing in VM and closely related places
Line 22:    and make it all to use the new XML API.  This step will consist of


Line 19: 2. Move XML processing out of DomainDescriptor and make DomainDescriptor
Line 20:    use the new XML API.
Line 21: 3. Get rid of all XML/DOM processing in VM and closely related places
Line 22:    and make it all to use the new XML API.  This step will consist of
Line 23:    several smaller steps.
yes, this is is probably the best way to have many small patches.
Line 24: 4. Move device XML/DOM handling out of VM to particular device classes.
Line 25: 5. Switch XML API implementation from minidom to etree.
Line 26: 
Line 27: This patch implements the first step by providing new `XML' class.  The


Line 23:    several smaller steps.
Line 24: 4. Move device XML/DOM handling out of VM to particular device classes.
Line 25: 5. Switch XML API implementation from minidom to etree.
Line 26: 
Line 27: This patch implements the first step by providing new `XML' class.  The
vmxml.py was meant to hold all the XML-related code we need to handle VM needs. So feel free to merge your code on it, and perhaps rename it as you see most fit.

Unfortunately we are out of good names. *vm* and *util* in particular are quite popular.
Line 28: new helper class is not used anywhere in this patch, but all of its
Line 29: functionality will be used in the followup patches.  We prepare the
Line 30: functionality in order to define complete clean API in advance, rather
Line 31: than making ad-hoc changes as needed afterwards.


Line 27: This patch implements the first step by providing new `XML' class.  The
Line 28: new helper class is not used anywhere in this patch, but all of its
Line 29: functionality will be used in the followup patches.  We prepare the
Line 30: functionality in order to define complete clean API in advance, rather
Line 31: than making ad-hoc changes as needed afterwards.
This is a good plan, but we usually add new API as close as possible to its clients, to demonstrate the usage and to highlight the need for a change.

We could make everyone happy (enough) just adding unit tests
once you un-draft this patch.
Line 32: 
Line 33: Change-Id: Ic177df4ef193c59b77220a6073cd35d28daa3cf1


https://gerrit.ovirt.org/#/c/52821/1/vdsm/virt/xmlutil.py
File vdsm/virt/xmlutil.py:

Line 22: 
Line 23: 
Line 24: class XML(object):
Line 25: 
Line 26:     @classmethod
not sure I understand why we need so many classmethods.
Culd you please highlight the need for them in followup patches?
Line 27:     def parse_string(cls, xml_str):
Line 28:         return xml.dom.minidom.parseString(xml_str)
Line 29: 
Line 30:     @classmethod


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic177df4ef193c59b77220a6073cd35d28daa3cf1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list