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