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 <vfeen...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Peter V. Saveliev <p...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches