Dan Kenigsberg has posted comments on this change. Change subject: virt: graphdev: add the GraphicsDevice class ......................................................................
Patch Set 19: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/26895/19//COMMIT_MSG Commit Message: Line 14: The GraphicDevice is built internally automatically Line 15: if no graphic device is passed into the device list, Line 16: but if the display{,Type,Network,...} parameters are passed. Line 17: Line 18: Backward compatibility with old unaware engines is preserved We must maintain backward compat with old unaware Vdsms, too. Line 19: both in the creation and reporting; however, in all VDSM<=>VDSM Line 20: communications, e.g. migrations and state saving for recovery, Line 21: the new graphic device representation is used. Line 22: http://gerrit.ovirt.org/#/c/26895/19/lib/vdsm/netinfo.py File lib/vdsm/netinfo.py: Line 937: if linkDict.get('device') == iface: Line 938: yield linkDict['name'] Line 939: Line 940: Line 941: def getNetworkIp(network): I wish the movement of this function had a dedicated commit message. This function is loaded with ancient backward compatibility, display network specifics (the fallback to guests_gateway_ip and to '0'), and an evil logless bare "except" clause. As it is, it must not be moved to netinfo.py, where it begs for more users. Line 942: try: Line 943: nets = networks() Line 944: device = nets[network].get('iface', network) Line 945: ip = getaddr(device) http://gerrit.ovirt.org/#/c/26895/19/vdsm/API.py File vdsm/API.py: Line 206: return {'status': {'code': errCode['MissParam'] Line 207: ['status']['code'], Line 208: 'message': 'Missing required ' Line 209: 'parameter %s' % (param)}} Line 210: if 'display' not in vmParams and 'migrationDest' not in vmParams: would you remind me why this relaxation of requiredParams is needed, and why in this patch? (best reminder would be a commit message of a separate patch, if that is possible) Line 211: return {'status': {'code': errCode['MissParam'] Line 212: ['status']['code'], Line 213: 'message': 'Missing required ' Line 214: 'parameter display'}} http://gerrit.ovirt.org/#/c/26895/19/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 191: self._vm.setDownStatus(NORMAL, vmexitreason.SAVE_STATE_SUCCEEDED) Line 192: self.status['status']['message'] = 'SaveState done' Line 193: Line 194: def _patchConfigForGraphics(self): Line 195: if 'display' in self._machineParams: 'display' is dropped for all clusterLevels - as far as I understand this breaks migration from ovirt-3.5's vdsm to earlier ones? Line 196: del self._machineParams['display'] Line 197: if 'displayIp' in self._machineParams: Line 198: del self._machineParams['displayIp'] Line 199: -- 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: 19 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
