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(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes