Change in vdsm[master]: vm: graphics: fix settings after restore
mpolednik at redhat.com
mpolednik at redhat.com
Tue Sep 15 12:49:23 UTC 2015
Martin Polednik has posted comments on this change.
Change subject: vm: graphics: fix settings after restore
......................................................................
Patch Set 11: Code-Review+1
(4 comments)
A bit of naming issues that I do not consider 'critical', the logic is fine by me at this point
https://gerrit.ovirt.org/#/c/44842/11/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1929: devXml.get('type'))
Line 1930: except LookupError:
Line 1931: self.log.warning('configuration mismatch: graphic device '
Line 1932: 'type %s found in domain XML, but not among '
Line 1933: 'VM devices' % devXml.get('type'))
read after reading comments below
the type is correct here because it explicitly refers to 'graphic' device.
________________________________________________^
_______________________________________possibly missing 's'
Line 1934: else:
Line 1935: devObj.setupPassword(devXml)
Line 1936: return ET.tostring(domObj)
Line 1937:
Line 2016: return response.error('hotplugNic', e.message)
Line 2017:
Line 2018: return {'status': doneCode, 'vmList': self.status()}
Line 2019:
Line 2020: def _lookupDeviceByClass(self, devType, device):
(unsure) does this work for all devices? interface comes to mind
Line 2021: for dev in self._devices[devType][:]:
Line 2022: try:
Line 2023: if dev.device == device:
Line 2024: return dev
Line 2024: return dev
Line 2025: except AttributeError:
Line 2026: continue
Line 2027: raise LookupError('Device instance for device identified by class %s '
Line 2028: 'not found' % device)
The error text is quite inaccurate, because 'device' for e.g. hostdev (but really, any other device) isn't the type but rather identification. If I understand correctly, devType would be better match.
That said, I think both of the variables could be displayed.
Line 2029:
Line 2030: def _lookupDeviceByAlias(self, devType, alias):
Line 2031: for dev in self._devices[devType][:]:
Line 2032: try:
https://gerrit.ovirt.org/#/c/44842/11/vdsm/virt/vmdevices/graphics.py
File vdsm/virt/vmdevices/graphics.py:
Line 131: graphics.setAttrs(listen='0')
Line 132:
Line 133: return graphics
Line 134:
Line 135: def _setPasswd(self, attrs):
Not sure I like the inconsistence in this case :)
setPassword and setupPassword (or passwd) seems better on paper, depends on how you feel about setupPassword calling setPassword (or passwd)
Line 136: if not utils.tobool(self.specParams.get('disableTicketing', False)):
Line 137: attrs['passwd'] = '*****'
Line 138: attrs['passwdValidTo'] = '1970-01-01T00:00:01'
Line 139:
--
To view, visit https://gerrit.ovirt.org/44842
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9f11e2d87cc715c77d305c005c1cd7f502d506d
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list