Francesco Romani has posted comments on this change. Change subject: vdsm: [wip] modeling graphics as a device ......................................................................
Patch Set 8: (10 comments) Initial review http://gerrit.ovirt.org/#/c/23555/8/tests/vmTests.py File tests/vmTests.py: Line 646: @MonkeyPatch(libvirtconnection, 'get', lambda x: ConnectionMock()) Line 647: @MonkeyPatch(utils, 'getHostUUID', Line 648: lambda: "fc25cbbe-5520-4f83-b82e-1541914753d9") Line 649: def testBuildCmdLineX86_64(self): Line 650: self.assertBuildCmdLine(vmTestsData.CONF_TO_DOMXML_X86_64) nice, but unrelated Line 651: Line 652: @MonkeyPatch(caps, 'getTargetArch', lambda: caps.Architecture.PPC64) Line 653: @MonkeyPatch(caps, 'osversion', lambda: { Line 654: 'release': '1', 'version': '18', 'name': 'Fedora'}) Line 655: @MonkeyPatch(libvirtconnection, 'get', lambda x: ConnectionMock()) Line 656: @MonkeyPatch(utils, 'getHostUUID', Line 657: lambda: "fc25cbbe-5520-4f83-b82e-1541914753d9") Line 658: def testBuildCmdLinePPC64(self): Line 659: self.assertBuildCmdLine(vmTestsData.CONF_TO_DOMXML_PPC64) ditto Line 660: Line 661: Line 662: @contextmanager Line 663: def FakeVM(params=None): http://gerrit.ovirt.org/#/c/23555/8/tests/vmTestsData.py File tests/vmTestsData.py: Line 56 Line 57 Line 58 Line 59 Line 60 Why this was removed from the generic XML? Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: # list of tripples (json, legacy json, xml) typo :) Line 22: CONF_TO_DOMXML_GRAPHICS = [({ Line 23: 'type': 'graphics', Line 24: 'device': 'spice', Line 25: 'specParams': {'port': '-1', http://gerrit.ovirt.org/#/c/23555/8/vdsm/API.py File vdsm/API.py: Line 275: """ Line 276: def adjustDisplayParamsInternal(obj): Line 277: LIBVIRT_PORT_AUTOSELECT = '-1' Line 278: obj['displayIp'] = \ Line 279: self._getNetworkIp(vmParams.get('displayNetwork')) (disclaimer: same potential issue in the old code) we are sure here 'displayNetwork' is present? other wise .get() will return None. Line 280: for i in ('port', 'tlsPort', 'displayPort', 'displaySecurePort'): Line 281: if i in obj: Line 282: obj[i] = LIBVIRT_PORT_AUTOSELECT Line 283: 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 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 1329: Line 1330: Line 1331: class GraphicsDevice(VmDevice): Line 1332: Line 1333: # Mapping between Vm.conf and specParams of GD. minor nit: please expand to GraphicsDevice Line 1334: # Used for converting data from GraphicsDevice to legacy Vm.conf fields. Line 1335: confToSpecParsMapping = { Line 1336: 'displayPort': 'port', Line 1337: 'displaySecurePort': 'tlsPort', Line 1358: <listen type='address' address='0'/> Line 1359: </graphics> Line 1360: Line 1361: """ Line 1362: specParams = self.specParams Do we really need this? Usually shortcuts aren't much used in VDSM Line 1363: graphicsAttrs = {'type': self.device, Line 1364: 'port': specParams.get('port', '-1'), Line 1365: 'autoport': 'yes'} Line 1366: Line 1374: Line 1375: graphics = XMLElement('graphics', **graphicsAttrs) Line 1376: Line 1377: # handle deprecated channel name in a smart way, Line 1378: # not just chop 1st char! I'm a bit confused here: is this a TODO? Line 1379: if graphicsAttrs['type'] == 'spice': Line 1380: if specParams.get('spiceSecureChannels'): Line 1381: for channel in specParams['spiceSecureChannels'].split(','): Line 1382: graphics.appendChildWithArgs('channel', name=channel[1:], Line 2888: graphDevs = [d for d in self.conf.get('devices', []) Line 2889: if d['type'] == GRAPHICS_DEVICES] Line 2890: Line 2891: if len(graphDevs) > 1 and graphDevs[1]['device'] == 'spice': Line 2892: graphDevs[0], graphDevs[1] = graphDevs[1], graphDevs[0] Not sure I understand the code here. What we want to achieve? And which case are we fixing here? Line 2893: Line 2894: return graphDevs Line 2895: Line 2896: def _getStatsInternal(self): -- 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: 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
