Dan Kenigsberg has posted comments on this change. Change subject: virt: graphdev: add the GraphicsDevice class ......................................................................
Patch Set 21: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/26895/21/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1387: graphicsAttrs['keymap'] = self.specParams['keyMap'] Line 1388: Line 1389: graphics = XMLElement('graphics', **graphicsAttrs) Line 1390: Line 1391: # handle deprecated channel name in a smart way, add "TODO" string (I do not see added smartness). Line 1392: # not just chop 1st char! Line 1393: if graphicsAttrs['type'] == 'spice': Line 1394: if self.specParams.get('spiceSecureChannels'): Line 1395: for chan in self.specParams['spiceSecureChannels'].split(','): Line 1862: Normalize graphics device provided by conf. Line 1863: """ Line 1864: return [{ Line 1865: 'type': GRAPHICS_DEVICES, Line 1866: 'device': 'spice' if self.conf.get('display', 'qxl') == 'qxl' if you want to drop support for qxlnc (and I won't mind that, if you verify that engine >= 3.0 does not send it), do it properly, in a separate patch. Line 1867: else 'vnc', Line 1868: 'specParams': dict( Line 1869: (v, self.conf[k]) Line 1870: for k, v in GraphicsDevice.LEGACY_MAPPING.iteritems() Line 2482: params['devices'] = newDevices Line 2483: params.update(dispParams) Line 2484: return params Line 2485: Line 2486: def getDisplayParams(self, dev): used once, and does not use "self". better be declared as private staticmethod, or a local function in getRemoteMachineParams() Line 2487: """ Line 2488: rebuild display* parameters as old engine would send them. Line 2489: symmetric to getConfGraphics. Line 2490: This is needed because all the configuration VDSM should accept Line 2493: way to achieve this goal. Line 2494: """ Line 2495: params = { Line 2496: 'display': 'qxl' if dev['device'] == 'spice' else dev['device']} Line 2497: for k, v in GraphicsDevice.LEGACY_MAPPING.iteritems(): renaming (k, v) to (legacyName, newName) would make this function more readable. Line 2498: try: Line 2499: params[k] = dev['specParams'][v] Line 2500: except KeyError: Line 2501: # just go ahead: either no specParams or no param specified Line 2497: for k, v in GraphicsDevice.LEGACY_MAPPING.iteritems(): Line 2498: try: Line 2499: params[k] = dev['specParams'][v] Line 2500: except KeyError: Line 2501: # just go ahead: either no specParams or no param specified My credo does not allow me to accept this. A missing 'specParams' in a graphics_device is a bug that we should never encounter, and as such - never swallow. The following code makes the comment redundant: if v in dev['specParams']: params[k] = dev['specParams'][v] Line 2502: pass Line 2503: return params Line 2504: Line 2505: def getStats(self): -- To view, visit http://gerrit.ovirt.org/26895 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I82e4414aa3efe7c756ba8ef9b7b47e591613a717 Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[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
