Martin Polednik has posted comments on this change. Change subject: vm: graphics: fix settings after restore ......................................................................
Patch Set 11: Code-Review+1 (4 comments) A bit of naming issues that I do not consider 'critical', the logic is fine by me at this point https://gerrit.ovirt.org/#/c/44842/11/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1929: devXml.get('type')) Line 1930: except LookupError: Line 1931: self.log.warning('configuration mismatch: graphic device ' Line 1932: 'type %s found in domain XML, but not among ' Line 1933: 'VM devices' % devXml.get('type')) read after reading comments below the type is correct here because it explicitly refers to 'graphic' device. ________________________________________________^ _______________________________________possibly missing 's' Line 1934: else: Line 1935: devObj.setupPassword(devXml) Line 1936: return ET.tostring(domObj) Line 1937: Line 2016: return response.error('hotplugNic', e.message) Line 2017: Line 2018: return {'status': doneCode, 'vmList': self.status()} Line 2019: Line 2020: def _lookupDeviceByClass(self, devType, device): (unsure) does this work for all devices? interface comes to mind Line 2021: for dev in self._devices[devType][:]: Line 2022: try: Line 2023: if dev.device == device: Line 2024: return dev Line 2024: return dev Line 2025: except AttributeError: Line 2026: continue Line 2027: raise LookupError('Device instance for device identified by class %s ' Line 2028: 'not found' % device) The error text is quite inaccurate, because 'device' for e.g. hostdev (but really, any other device) isn't the type but rather identification. If I understand correctly, devType would be better match. That said, I think both of the variables could be displayed. Line 2029: Line 2030: def _lookupDeviceByAlias(self, devType, alias): Line 2031: for dev in self._devices[devType][:]: Line 2032: try: https://gerrit.ovirt.org/#/c/44842/11/vdsm/virt/vmdevices/graphics.py File vdsm/virt/vmdevices/graphics.py: Line 131: graphics.setAttrs(listen='0') Line 132: Line 133: return graphics Line 134: Line 135: def _setPasswd(self, attrs): Not sure I like the inconsistence in this case :) setPassword and setupPassword (or passwd) seems better on paper, depends on how you feel about setupPassword calling setPassword (or passwd) Line 136: if not utils.tobool(self.specParams.get('disableTicketing', False)): Line 137: attrs['passwd'] = '*****' Line 138: attrs['passwdValidTo'] = '1970-01-01T00:00:01' Line 139: -- To view, visit https://gerrit.ovirt.org/44842 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic9f11e2d87cc715c77d305c005c1cd7f502d506d Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
