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

vfeenstr at redhat.com vfeenstr at redhat.com
Mon Mar 24 11:40:41 UTC 2014


Vinzenz Feenstra has posted comments on this change.

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


Patch Set 13:

(8 comments)

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

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 u
correct, it has to be renamed
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 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()))
Line 491:         return self._hash or '0'
> Why the or in return? Just compute the hash and save it.
You might not have devices, and this keeps the original behaviour that it is initially returning '0'
Line 492: 
Line 493: 
Line 494: def eventToString(event):
Line 495:     return _EVENT_STRINGS[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?
Yeah, I hate it as well ;-)
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 3105:         self._getUnderlyingWatchdogDeviceInfo()
Line 3106:         self._getUnderlyingSmartcardDeviceInfo()
Line 3107:         self._getUnderlyingConsoleDeviceInfo()
Line 3108:         # Obtain info of all unknown devices. Must be last!
Line 3109:         self._getUnderlyingUnknownDeviceInfo()
> For another patch: get rid of the "Underlying" in these names.
In the long run we want to get rid of them inside of the VM class and refactor them out of there, then we'd get rid of this name. For now I tend to agree with Michal
Line 3110: 
Line 3111:     def _updateAgentChannels(self):
Line 3112:         """
Line 3113:         We moved the naming of guest agent channel sockets. To keep backwards


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 
True, I just tried to use the convenience functions of it. It's probably not really worth it
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?!
The return value was never used anywhere, that's why I dropped it. It's not really obvious though.

I am not sure why I would add an explaining name for this and we use this everywhere else without it.
XMLDesc is an API of libvirt. We don't really document that usage anywhere else either, the only part which is not quite clear to someone might be the 0 parameter.
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.
Well and before it was an out of index error in that case. Not sure how relevant that really is
Line 5113:         port = graphics.getAttribute('port')
Line 5114:         if port:
Line 5115:             self.conf['displayPort'] = port
Line 5116:         port = graphics.getAttribute('tlsPort')


Line 5120:     def _getUnderlyingNetworkInterfaceInfo(self):
Line 5121:         """
Line 5122:         Obtain network interface info from libvirt.
Line 5123:         """
Line 5124:         # TODO use xpath instead of parseString (here and elsewhere)
> This is not valid here anymore, is it?
Probably not
Line 5125:         for x in self._lastXMLDesc.getDeviceElements('interface'):
Line 5126:             devType = x.getAttribute('type')
Line 5127:             mac = x.getElementsByTagName('mac')[0].getAttribute('address')
Line 5128:             alias = x.getElementsByTagName('alias')[0].getAttribute('name')


-- 
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