Dan Kenigsberg has posted comments on this change. Change subject: vdsm: [wip] modeling graphics as a device ......................................................................
Patch Set 8: (11 comments) http://gerrit.ovirt.org/#/c/23555/8/vdsm/API.py File vdsm/API.py: Line 253: Line 254: if 'nicModel' not in vmParams: Line 255: vmParams['nicModel'] = config.get('vars', 'nic_model') Line 256: Line 257: self._adjustDisplayParams(vmParams) This code better fits into vm.py's conversion of old-style graphics args to new-style ones (not that I like adding code to vm.py). Line 258: Line 259: return self._cif.createVm(vmParams) Line 260: Line 261: except OSError as e: Line 272: Adjusts parameters of graphics on vm start: Line 273: - resets ports to be autoselected by libvirt, Line 274: - sets displayNetwork value according to vm display network. Line 275: """ Line 276: def adjustDisplayParamsInternal(obj): "obj" is a very vague name, please avoid it. Line 277: LIBVIRT_PORT_AUTOSELECT = '-1' Line 278: obj['displayIp'] = \ Line 279: self._getNetworkIp(vmParams.get('displayNetwork')) Line 280: for i in ('port', 'tlsPort', 'displayPort', 'displaySecurePort'): Line 286: Line 287: # new approach - graphics is a device Line 288: for dev in vmParams.get('devices', []): Line 289: if dev.get('type') == 'graphics': Line 290: specPars = dev.get('specParams', []) > minor nit: I'd use 'specParams' to be coherent with the rest of the code and even more importantly, specParams is a dictionary, not a list. Line 291: adjustDisplayParamsInternal(specPars) Line 292: Line 293: def desktopLock(self): Line 294: """ http://gerrit.ovirt.org/#/c/23555/8/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 373: startTime) Line 374: self._monitorThread.start() Line 375: Line 376: try: Line 377: # todo take graphics from conf['devices'] into account Indeed. It would be nice to have a very simple patch defining a function named "hasSpice", and later make the function work for old-style and new-style graphics alike. Line 378: if ('qxl' in self._vm.conf['display'] and Line 379: self._vm.conf.get('clientIp')): Line 380: SPICE_MIGRATION_HANDOVER_TIME = 120 Line 381: self._vm._reviveTicket(SPICE_MIGRATION_HANDOVER_TIME) Line 977: Line 978: <clock offset="variable" adjustment="-3600"> Line 979: <timer name="rtc" tickpolicy="catchup"> Line 980: </clock> Line 981: """ please avoid needless whitespace noise. Line 982: m = XMLElement('clock', offset='variable', Line 983: adjustment=str(self.conf.get('timeOffset', 0))) Line 984: m.appendChildWithArgs('timer', name='rtc', tickpolicy='catchup') Line 985: m.appendChildWithArgs('timer', name='pit', tickpolicy='delay') Line 1228: vmc = XMLElement('channel', type='spicevmc') Line 1229: vmc.appendChildWithArgs('target', type='virtio', Line 1230: name='com.redhat.spice.0') Line 1231: # self.dom.appendChild(vmc) Line 1232: self._devices.appendChild(vmc) ? Line 1233: Line 1234: def toxml(self): Line 1235: return self.doc.toprettyxml(encoding='utf-8') Line 1236: Line 2161: 'index': 0, Line 2162: 'truesize': 0}) Line 2163: return removables Line 2164: Line 2165: def _getGraphicsDeviceFromConf(self): it would make sense to follow the __legacyDrives naming convension for this function. Line 2166: """ Line 2167: Derives graphics device (if it's missing in vm.devices) from vm.conf. Line 2168: Used for legacy engines (that don't use graphics framebuffer as a Line 2169: device). Line 2882: Line 2883: def _getGraphicsDevices(self): Line 2884: """ Line 2885: Returns graphics devices of a VM. If the VM has multiple devices, then Line 2886: SPICE dev is returned as a first one. Why? And would there never be more than two? Line 2887: """ Line 2888: graphDevs = [d for d in self.conf.get('devices', []) Line 2889: if d['type'] == GRAPHICS_DEVICES] Line 2890: Line 2926: 'network': {}, 'disks': {}, Line 2927: 'monitorResponse': str(self._monitorResponse), Line 2928: 'elapsedTime': str(int(time.time() - self._startTime)), } Line 2929: Line 2930: graphics = self._getGraphicsDevices() Please encapsulate this in a separate properly-named function, maybe _getLegcayGraphicsStats(). Line 2931: if len(graphics) > 0: Line 2932: fstGraphics = graphics[0] Line 2933: confToSpecParsMapping = GraphicsDevice.confToSpecParsMapping Line 2934: Line 2940: Line 2941: displayType = fstGraphics['device'] Line 2942: if displayType == 'spice': Line 2943: displayType = 'qxl' Line 2944: stats.update({'displayType': displayType}) this is a very complex way of writing stats['displayType'] = displayType Line 2945: Line 2946: if 'cdrom' in self.conf: Line 2947: stats['cdrom'] = self.conf['cdrom'] Line 2948: if 'boot' in self.conf: Line 3119: _QEMU_GA_DEVICE_NAME) Line 3120: domxml.appendInput() Line 3121: Line 3122: # todo (this is just workaround for creating spicevmc channel) Line 3123: graphics = self._getGraphicsDevices() if [for dev in self._getGraphicsDevices() if dev['device'] == 'spice']: domxml.appendSpiceVmcChannel() would be cleaner. Line 3124: if len(graphics) > 0 and graphics[0]['device'] == 'spice': Line 3125: domxml.appendSpiceVmcChannel() Line 3126: Line 3127: if self.arch == caps.Architecture.PPC64: -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: [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
