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 <fromani(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkobzik(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes