Change in vdsm[master]: Introduction for caching the parsed domain XML

nsoffer at redhat.com nsoffer at redhat.com
Thu Mar 20 19:03:35 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: Introduction for caching the parsed domain XML
......................................................................


Patch Set 13: Code-Review-1

(8 comments)

This is a very nice patch, but it needs some more work.

http://gerrit.ovirt.org/#/c/17694/13/vdsm/vm.py
File vdsm/vm.py:

Line 457:                   "Shutdown",
Line 458:                   "PM-Suspended")
Line 459: 
Line 460: 
Line 461: class ParsedDomXML(object):
While this name is clear, a name like LibvirtDomain or DomainDescriptor may be nicer.
Line 462:     def __init__(self, xml):
Line 463:         self._hash = None
Line 464:         self._xml = xml
Line 465:         self._dom = _domParseStr(xml)


Line 462:     def __init__(self, xml):
Line 463:         self._hash = None
Line 464:         self._xml = xml
Line 465:         self._dom = _domParseStr(xml)
Line 466:         self._devices = self.firstElementByTagName('devices')
Since we assume that self._xml, self._dom, and self._devices never change, we can simply create self._hash here and return it in hash(). Since hashing is cheap, this should be good enough.
Line 467: 
Line 468:     @property
Line 469:     def xml(self):
Line 470:         return self._xml


Line 484:     def getDeviceElements(self, tagName):
Line 485:         return self._devices.getElementsByTagName(tagName)
Line 486: 
Line 487:     @property
Line 488:     def hash(self):
This should be named devicesHash - this is  not a hash of the xml. Can we use the much cheaper hash(self._xml)
Line 489:         if self.devices and not self._hash:
Line 490:             self._hash = str(hash(self._devices.toxml()))
Line 491:         return self._hash or '0'
Line 492: 


Line 486: 
Line 487:     @property
Line 488:     def hash(self):
Line 489:         if self.devices and not self._hash:
Line 490:             self._hash = str(hash(self._devices.toxml()))
For another patch: I'm not sure how we use this hash - but it is not good for detecting changes in the xml. For this we should use md5 or sha1.
Line 491:         return self._hash or '0'
Line 492: 
Line 493: 
Line 494: def eventToString(event):


Line 2033:         if 'vmName' not in self.conf:
Line 2034:             self.conf['vmName'] = 'n%s' % self.id
Line 2035:         self._guestSocketFile = self._makeChannelPath(_VMCHANNEL_DEVICE_NAME)
Line 2036:         self._qemuguestSocketFile = self._makeChannelPath(_QEMU_GA_DEVICE_NAME)
Line 2037:         self._lastXMLDesc = ParsedDomXML(
For next patch - rename lastXMLDesc to something nicer?
Line 2038:             '<domain><uuid>%s</uuid></domain>' % self.id)
Line 2039:         self._released = False
Line 2040:         self._releaseLock = threading.Lock()
Line 2041:         self.saveState()


Line 3292: 
Line 3293:         for snappableDiskDeviceXmlElement in snappableDiskDeviceXmlElements:
Line 3294:             self._changeDisk(snappableDiskDeviceXmlElement)
Line 3295: 
Line 3296:         return parsedSrcDomXML.dom.toxml()
This usage is bad - we take a read-only object (ParsedDomXML), modify it's dom behind it's back, and then accessing the dom to create a new xml.

I think we should leave the old code as is, and remove the dom property, used only by this function.
Line 3297: 
Line 3298:     def _changeDisk(self, diskDeviceXmlElement):
Line 3299:         diskType = diskDeviceXmlElement.getAttribute('type')
Line 3300: 


Line 4634:             pass
Line 4635:         return pid
Line 4636: 
Line 4637:     def _getUnderlyingVmInfo(self):
Line 4638:         self._lastXMLDesc = ParsedDomXML(self._dom.XMLDesc(0))
This function used to return self._lastXMLDesc?!

If this should not have a return value, add another patch removing the return value and renaming this to something like initUnderlyingVmInfo.

Minor thing: it is not clear what is self._dom.XMLDesc(0). An explaining variable may help here:

    explaining_name = self._dom.XMLDesc(0)
    self._lastXMLDesc = ParsedDomXML(explaining_name)
Line 4639: 
Line 4640:     def _ejectFloppy(self):
Line 4641:         if 'volatileFloppy' in self.conf:
Line 4642:             utils.rmFile(self.conf['floppy'])


Line 5108:     def _getUnderlyingDisplayPort(self):
Line 5109:         """
Line 5110:         Obtain display port info from libvirt.
Line 5111:         """
Line 5112:         graphics = self._lastXMLDesc.firstElementByTagName('graphics')
This can return None - will raise AttributeError on next line.
Line 5113:         port = graphics.getAttribute('port')
Line 5114:         if port:
Line 5115:             self.conf['displayPort'] = port
Line 5116:         port = graphics.getAttribute('tlsPort')


-- 
To view, visit http://gerrit.ovirt.org/17694
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e106b2f2d3f4160d4e882f1a2880cb1b52fbb22
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list