Francesco Romani has posted comments on this change. Change subject: vdsm: [wip] modeling graphics as a device ......................................................................
Patch Set 4: (3 comments) Preliminary notes, more to come. http://gerrit.ovirt.org/#/c/23555/4/tests/vmTestsData.py File tests/vmTestsData.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: CONF_TO_DOMXML_GRAPHICS = [({ # list of tripples (json, legacy json, xml) documenting the format is a good idea but I'd not place here the comment. The line above may be better We're dangerously near the kingdom of personal taste, so take this with a grain of salt. Line 22: 'type': 'graphics', Line 23: 'specParams': {'type': 'spice', Line 24: 'port': '-1', Line 25: 'tlsPort': '-1', http://gerrit.ovirt.org/#/c/23555/4/vdsm/API.py File vdsm/API.py: Line 273: def _adjustDisplayParams(self, vmParams): Line 274: # legacy Line 275: vmParams['displayIp'] = self._getNetworkIp(vmParams.get( Line 276: 'displayNetwork')) Line 277: vmParams['displayPort'] = '-1' # selected by libvirt Here and the remainder of the function: better to switch to a constant, which will also make the code self-documenting and avoid the need for the comment. Something like LIBVIRT_PORT_AUTOSELECT would be ok but feel absolutely free to pick any suitable name you like most Line 278: vmParams['displaySecurePort'] = '-1' Line 279: Line 280: # new approach - graphics is a device Line 281: for dev in vmParams.get('devices', []): Line 283: specPars = dev.get('specParams', []) Line 284: specPars['displayIp'] = self._getNetworkIp(dev.get( Line 285: 'displayNetwork')) Line 286: specPars['port'] = '-1' # selected by libvirt Line 287: specPars['tlsPort'] = '-1' I think those three lines above can be factored in a common helper. Line 288: Line 289: def desktopLock(self): Line 290: """ Line 291: Lock user session in guest operating system using guest agent. -- To view, visit http://gerrit.ovirt.org/23555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia59b933f4cd1e3ab562ad2ec1c237007c83f214c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
