Francesco Romani has posted comments on this change. Change subject: vmdevices: Initial support for device updates with etree's ......................................................................
Patch Set 1: (10 comments) https://gerrit.ovirt.org/#/c/46525/1//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2015-09-22 14:23:09 +0200 Line 4: Commit: Milan Zamazal <[email protected]> Line 5: CommitDate: 2015-09-22 14:46:07 +0200 Line 6: Line 7: vmdevices: Initial support for device updates with etree's let's rename this like: virt: devices: initial support ... Line 8: Line 9: VM device information update is received from libvirt in the form of XML Line 10: data. The data is parsed using xml.dom.minidom. VDSM device objects Line 11: are then updated from outside with device parameters obtained from the https://gerrit.ovirt.org/#/c/46525/1/tests/deviceTests.py File tests/deviceTests.py: Line 295: <address type='pci' domain='0x0000' bus='0x00' slot='0x05' Line 296: function='0x0'/> Line 297: </sound>''' Line 298: for etreeElement in (etree.fromstring(xml), Line 299: etree.fromstring('<wrap>%s</wrap>' % (xml,)),): testing using one snippet is OK. Please add a test which uses a complete XML domain. Line 300: soundDevice.update(etreeElement) Line 301: parsedAddress = {'type': 'pci', 'domain': '0x0000', 'bus': '0x00', Line 302: 'slot': '0x05', 'function': '0x0'} Line 303: self.assertEqual(soundDevice.address, parsedAddress) Line 299: etree.fromstring('<wrap>%s</wrap>' % (xml,)),): Line 300: soundDevice.update(etreeElement) Line 301: parsedAddress = {'type': 'pci', 'domain': '0x0000', 'bus': '0x00', Line 302: 'slot': '0x05', 'function': '0x0'} Line 303: self.assertEqual(soundDevice.address, parsedAddress) please inline parsedAddress here, so we can get rid of the temporary. Line 304: self.assertEqual(soundDevice.alias, 'sound0') Line 305: Line 306: def testVideoXML(self): Line 307: videoXML = """ https://gerrit.ovirt.org/#/c/46525/1/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py: Line 52: return self._devices Line 53: Line 54: def get_device_elements(self, tagName, etree=False): Line 55: return self._elements_by_tag_name(tagName, devices_only=True, Line 56: etree=etree) let's evaluate adding a new different API instead of adding one parameter to this one. Line 57: Line 58: @property Line 59: def devices_hash(self): Line 60: return self._devices_hash Line 80: elements = dom.getElementsByTagName(tagName) Line 81: return elements Line 82: Line 83: def _first_element_by_tag_name(self, tagName, etree=False): Line 84: elements = self._elements_by_tag_name(tagName, etree=etree) do we really need this change? Line 85: return elements[0] if elements else None Line 86: Line 87: def get_memory_size(self): Line 88: """ https://gerrit.ovirt.org/#/c/46525/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 4217: break Line 4218: else: Line 4219: # Nothing found? It's suspicious and no parsed info is Line 4220: # available for vm's conf in such a case, so let's give up. Line 4221: return worth adding a log here. Line 4222: # Update vm's conf with address Line 4223: for dev in self.conf['devices']: Line 4224: if ((dev['type'] == hwclass.SOUND) and Line 4225: (not dev.get('address') or not dev.get('alias'))): https://gerrit.ovirt.org/#/c/46525/1/vdsm/virt/vmdevices/core.py File vdsm/virt/vmdevices/core.py: Line 36: Line 37: class Base(vmxml.Device): Line 38: __slots__ = ('deviceType', 'device', 'alias', 'specParams', 'deviceId', Line 39: 'conf', 'log', '_deviceXML', 'type', 'custom') Line 40: _XML_TAG = None please see reuse one of 'deviceType' or 'device', should carry the very same information Line 41: Line 42: def __init__(self, conf, log, **kwargs): Line 43: self.conf = conf Line 44: self.log = log Line 56: attrs = [':'.join((a, str(getattr(self, a, None)))) for a in dir(self) Line 57: if not a.startswith('__')] Line 58: return ' '.join(attrs) Line 59: Line 60: def _getUnderlyingDeviceAddress(self, etreeElement, index=0): this doesn't need 'self', so it could be made a module-level function Line 61: """ Line 62: Get and return device address dictionary from 'etreeElement'. Line 63: Line 64: :param etreeElement: element containing <address> element(s) describing Line 81: address[key.strip()] = value.strip() Line 82: return address Line 83: Line 84: def _update_parameters(self, deviceElement): Line 85: # To be extended / updated in subclasses as needed. we probably do not need this helper, we can inline it on update() Line 86: self.address = self._getUnderlyingDeviceAddress(deviceElement) Line 87: self.alias = deviceElement.find('alias').get('name') Line 88: Line 89: def update(self, etreeElement): Line 257: def getXML(self): Line 258: """ Line 259: Create domxml for sound device Line 260: """ Line 261: sound = self.createXmlElem(self._XML_TAG, None, ['address']) unrelated. May be good idea, but let's save for another change. Line 262: sound.setAttrs(model=self.device) Line 263: return sound Line 264: Line 265: -- To view, visit https://gerrit.ovirt.org/46525 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7afe81af334ab9d8e7da6626cb902b95202cc05c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
