Francesco Romani has posted comments on this change. Change subject: vdsm: [wip] modeling graphics as a device ......................................................................
Patch Set 3: (13 comments) preliminary review; not voting since it is clearly and explicitely marked as draft http://gerrit.ovirt.org/#/c/23555/3//COMMIT_MSG Commit Message: Line 13: Line 14: to be done: Line 15: - spice vmc channel Line 16: - the test data should be probably shared between legacy test and the Line 17: new one Yes, if the XML test data is identical, I think it is better as it was done with the PPC patch. see tests/vmTestData.py Line 18: Line 19: Signed-off-by: Frantisek Kobzik <[email protected]> http://gerrit.ovirt.org/#/c/23555/3/tests/vmTests.py File tests/vmTests.py: Line 333: """ Line 334: <graphics autoport="yes" keymap="en-us" passwd="*****" Line 335: passwdValidTo="1970-01-01T00:00:01" port="-1" type="vnc"> Line 336: <listen network="vdsm-vmDisplay" type="network"/> Line 337: </graphics>""", Indeed the XML looks identical (well at least this snippet), if it is not please highlight the differences Line 338: Line 339: """ Line 340: <graphics autoport="yes" listen="0" passwd="*****" Line 341: passwdValidTo="1970-01-01T00:00:01" port="-1" Line 359: 'spiceSecureChannels': Line 360: "smain,sinputs,scursor,splayback,sdisplay" Line 361: }}] Line 362: for dev, xml in zip(devs, expectedXMLs): Line 363: genxml = vm.GraphicsDevice(self.conf, self.log, **dev) nit: genxml is misleading here Line 364: self.assertXML(genxml.getXML(), xml) Line 365: Line 366: def testBalloonXML(self): Line 367: balloonXML = '<memballoon model="virtio"/>' http://gerrit.ovirt.org/#/c/23555/3/vdsm/API.py File vdsm/API.py: Line 266: # legacy Line 267: vmParams['displayIp'] = self._getNetworkIp(vmParams.get( Line 268: 'displayNetwork')) Line 269: vmParams['displayPort'] = '-1' # selected by libvirt Line 270: vmParams['displaySecurePort'] = '-1' since those three lines above seems to be used verbatim even in the new approach, better to factor them out in a common method Line 271: Line 272: # new approach - graphics is a device Line 273: for dev in vmParams.get('devices', []): Line 274: if dev.get('type') == 'graphics': Line 268: 'displayNetwork')) Line 269: vmParams['displayPort'] = '-1' # selected by libvirt Line 270: vmParams['displaySecurePort'] = '-1' Line 271: Line 272: # new approach - graphics is a device I don't get yet two things: are the two approaches mutually exclusive? there is some fallback to put in place (e.g try the old way first, overwrite with the new one, or vice versa) worth a comment, I think. Line 273: for dev in vmParams.get('devices', []): Line 274: if dev.get('type') == 'graphics': Line 275: # adjust my stuff Line 276: dev['displayIp'] = self._getNetworkIp(dev.get( http://gerrit.ovirt.org/#/c/23555/3/vdsm/vm.py File vdsm/vm.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: from pprint import pprint ok since it is a draft :) Line 22: Line 23: # stdlib imports Line 24: from contextlib import contextmanager Line 25: from copy import deepcopy Line 1190: </channel> Line 1191: """ Line 1192: for dev in self.conf.get('devices', []): Line 1193: if dev['type'] == GRAPHICS_DEVICES: Line 1194: return # probably could be written more sane I think this should be handled in the caller, not here. Line 1195: Line 1196: graphicsAttrs = {'port': self.conf['displayPort'], 'autoport': 'yes'} Line 1197: if self.conf['display'] == 'vnc': Line 1198: graphicsAttrs['type'] = 'vnc' Line 1346: self.log.error("=========== Using new GraphicsDevice") Line 1347: pprint(self.conf) Line 1348: self.log.error("*********** ") Line 1349: pprint(self.specParams) Line 1350: self.log.error("=========== ") To avoid this I think you just need more unit tests; VmDevices are reasonnably easy to test and exercise in isolation in the unit tests. Line 1351: Line 1352: specParams = self.specParams Line 1353: graphicsAttrs = {'type': specParams['type'], Line 1354: 'port': specParams['port'], Line 1352: specParams = self.specParams Line 1353: graphicsAttrs = {'type': specParams['type'], Line 1354: 'port': specParams['port'], Line 1355: 'autoport': 'yes' Line 1356: } nit: shouldn't pep8 complain here? Line 1357: Line 1358: for key in ('tlsPort', 'keymap'): Line 1359: if key in specParams: Line 1360: graphicsAttrs[key] = specParams[key] Line 1358: for key in ('tlsPort', 'keymap'): Line 1359: if key in specParams: Line 1360: graphicsAttrs[key] = specParams[key] Line 1361: Line 1362: if specParams.get('disableTicketing', '').lower() != 'true': maybe utils.tobool? Line 1363: graphicsAttrs['passwd'] = '*****' Line 1364: graphicsAttrs['passwdValidTo'] = '1970-01-01T00:00:01' Line 1365: Line 1366: graphics = XMLElement('graphics', **graphicsAttrs) Line 1372: for channel in specParams['spiceSecureChannels'].split(','): Line 1373: graphics.appendChildWithArgs('channel', name=channel[1:], Line 1374: mode='secure') Line 1375: Line 1376: # this MUST be certainly handled as a separate device Agreed, just not sure it is in scope here. Line 1377: #vmc = XMLElement('channel', type='spicevmc') Line 1378: #vmc.appendChildWithArgs('target', type='virtio', Line 1379: # name='com.redhat.spice.0') Line 1380: #self._devices.appendChild(vmc) Line 1380: #self._devices.appendChild(vmc) Line 1381: Line 1382: if specParams.get('displayNetwork'): Line 1383: graphics.appendChildWithArgs('listen', type='network', Line 1384: network=netinfo.LIBVIRT_NET_PREFIX + Is this a network address? Line 1385: specParams.get('displayNetwork')) Line 1386: else: Line 1387: graphics.setAttrs(listen='0') Line 1388: Line 5107: graphicsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \ Line 5108: getElementsByTagName('devices')[0].getElementsByTagName('graphics') Line 5109: Line 5110: for gxml in graphicsxml: Line 5111: graphics_type = gxml.getAttribute('type') nit: note that VDSM coding style require a different namingRuleForIdentifiers Line 5112: Line 5113: for dev in self.conf['devices']: Line 5114: specParams = dev.get('specParams', {}) Line 5115: if specParams.get('type', '') == graphics_type: -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[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
