Change in vdsm[master]: vm: graphics: fix settings after restore

mpolednik at redhat.com mpolednik at redhat.com
Mon Sep 14 12:47:10 UTC 2015


Martin Polednik has posted comments on this change.

Change subject: vm: graphics: fix settings after restore
......................................................................


Patch Set 8: Code-Review-1

(6 comments)

https://gerrit.ovirt.org/#/c/44842/8//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: vm: graphics: fix settings after restore
Line 8: 
Line 9: When restoring a VM from hibernation (aka migration from file),
Line 10: if we don't reset the some details of the SPICE configuration.
then what? (weird grammar / missing sentence)
Line 11: 
Line 12: This behaviour is easily reproduced on Centos 7.1/2 by:
Line 13: a. booting a VM with SPICE graphic device
Line 14: b. do a snapshot, make sure to check 'snapshot memory'


Line 8: 
Line 9: When restoring a VM from hibernation (aka migration from file),
Line 10: if we don't reset the some details of the SPICE configuration.
Line 11: 
Line 12: This behaviour is easily reproduced on Centos 7.1/2 by:
Could be omitted due to dependency, nothing major/possibly bad thing though.
Line 13: a. booting a VM with SPICE graphic device
Line 14: b. do a snapshot, make sure to check 'snapshot memory'
Line 15: c. preview the snapshot
Line 16: d. run the VM


Line 9: When restoring a VM from hibernation (aka migration from file),
Line 10: if we don't reset the some details of the SPICE configuration.
Line 11: 
Line 12: This behaviour is easily reproduced on Centos 7.1/2 by:
Line 13: a. booting a VM with SPICE graphic device
boot (--img)
Line 14: b. do a snapshot, make sure to check 'snapshot memory'
Line 15: c. preview the snapshot
Line 16: d. run the VM
Line 17: e. check the VM parameters (e.g. qemu)


https://gerrit.ovirt.org/#/c/44842/8/tests/vmTests.py
File tests/vmTests.py:

Line 966:             u'type': u'graphics',
Line 967:             'port': u'5900'
Line 968:         }
Line 969: 
Line 970:         with fake.VM(devices=[conf], create_device_objects=True)as testvm:
) as
 ^
missing space
Line 971:             out_dom_xml = testvm._correctSpiceConfiguration(
Line 972:                 _load_xml('vm_restore_spice_before.xml'))
Line 973: 
Line 974:         self.assertXMLEqual(out_dom_xml,


https://gerrit.ovirt.org/#/c/44842/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1920:         """
Line 1921:         Fix the configuration of SPICE after resume.
Line 1922:         Make sure the ticketing settings are right
Line 1923:         """
Line 1924:         for dev in self._devices[hwclass.GRAPHICS]:
see graphics.py :(
Line 1925:             domXML = dev.setupSpicePassword(domXML)
Line 1926:         return domXML
Line 1927: 
Line 1928:     def _changeDisk(self, diskDeviceXmlElement):


https://gerrit.ovirt.org/#/c/44842/8/vdsm/virt/vmdevices/graphics.py
File vdsm/virt/vmdevices/graphics.py:

Line 137:         if not utils.tobool(self.specParams.get('disableTicketing', False)):
Line 138:             attrs['passwd'] = '*****'
Line 139:             attrs['passwdValidTo'] = '1970-01-01T00:00:01'
Line 140: 
Line 141:     def setupSpicePassword(self, domXML):
This is very sneaky and evil without at least a docstring. The behavior is O(n^2) - for each graphics device, you're iterating all of the graphics devices (similar to getUnderlying*). _correctSpiceConfiguration should only send XML of each device or do all of the setupSpicePassword's in a single pass.
Line 142:         if self.device == 'spice':
Line 143:             domObj = etree.fromstring(domXML)
Line 144:             devices = domObj.findall('devices')
Line 145: 


-- 
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: 8
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