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