Francesco Romani has posted comments on this change. Change subject: virt: devices: initial support for device updates with etree's ......................................................................
Patch Set 1: (5 comments) 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,)),): > Sound.update is not expected to receive a complete XML domain as it may con This is absolutely correct. update() expects the right etree element to be given, and the logic to find indeed the right etree element, in the case more than one device is present, doesn't belong here. But still, we should add a test using a complete XML domain, with just one sound device, and a clear warning that testing with more than one device is out of the scope for the aforementioned reasons. But now I'm wondering if I actually got your intentions right: what is the purpose of the test using the xml wrapped in <wrap>%s</wrap>, here? 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) 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) > Do you mean get_device_elements() should be reworked as such or just consid yes, this is what I meant. I didn't particulary loved the get_device_elements() API, and the more fitting 'devices' properties is used just once in just one code path, so it is a good candidate for replacement. I do see your point about etree argument. But why do you think the 'etree option' approach is clearly superior? Let's discuss. My take is the following: I'm all to avoid flags, especially boolean ones, and here the abstraction leaks anyway (this is an existing problem, not this patch's fault). I believe that adding this argument goes -albeit slightly- in the direction of the leaky abstraction, hence my request. 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) > It depends on how we decide about get_device_elements(). agreed. 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/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 > I agree adding _XML_TAG is ugly and most likely unnecessary. But I couldn't Indeed the right field would be 'deviceType'. The values that 'deviceType' may have are listed on the schema documentation: vdsm/rpc/vdsmapi-schema.json It is possible that the tests take some shortcuts and doesn't properly initialize all the fields. Will get back on this later. Line 41: Line 42: def __init__(self, conf, log, **kwargs): Line 43: self.conf = conf Line 44: self.log = log 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. > I'd prefer keeping the methods separate as they perform somewhat different makes sense, but we usually (try to) introduce changes when we have a clear case for them, and at least two different users. I agree with your reasoning, but here either clients or strong evidence we'll have them, so I'd rather avoid to foresee the future, and adopt the simplest solution. Line 86: self.address = self._getUnderlyingDeviceAddress(deviceElement) Line 87: self.alias = deviceElement.find('alias').get('name') Line 88: Line 89: def update(self, etreeElement): -- 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: Milan Zamazal <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
