Change in vdsm[master]: virt: graphdev: add the GraphicsDevice class
danken at redhat.com
danken at redhat.com
Fri May 9 11:55:36 UTC 2014
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 <fromani at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkobzik at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list