Hello Nir Soffer, Dan Kenigsberg, Michal Skrivanek, Martin Polednik, I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/34982 to review the following change. Change subject: vm: return full copy of the conf data on status() ...................................................................... vm: return full copy of the conf data on status() It is unsafe for status() to return a shallow copy of the Vm.conf data. Many modules (e.g. copy, json) of the python standard libary make use of the iteritems() method when dealing with dicts, so they can explode while iterating on data being mutated by the creation thread. This can be triggered just if someone (Engine) queries for Vm status while the creation thread is running, like Traceback (most recent call last): File "/usr/lib/python2.6/site-packages/yajsonrpc/__init__.py", line 242, in sendReply encodedObjects.append(response.encode()) File "/usr/lib/python2.6/site-packages/yajsonrpc/__init__.py", line 140, in encode return json.dumps(res, 'utf-8') File "/usr/lib64/python2.6/json/__init__.py", line 237, in dumps **kw).encode(obj) File "/usr/lib64/python2.6/json/encoder.py", line 367, in encode chunks = list(self.iterencode(o)) File "/usr/lib64/python2.6/json/encoder.py", line 309, in _iterencode for chunk in self._iterencode_dict(o, markers): File "/usr/lib64/python2.6/json/encoder.py", line 275, in _iterencode_dict for chunk in self._iterencode(value, markers): [...] RuntimeError: dictionary changed size during iteration This patch makes it always return a full copy. Change-Id: I604f8534c9380d289633df4a8151699ab0d97e34 Bug-Url: https://bugzilla.redhat.com/1148583 Signed-off-by: Francesco Romani <from...@redhat.com> Reviewed-on: http://gerrit.ovirt.org/34147 Reviewed-by: Nir Soffer <nsof...@redhat.com> Reviewed-by: Michal Skrivanek <michal.skriva...@redhat.com> Reviewed-by: Martin Polednik <mpoled...@redhat.com> Reviewed-by: Dan Kenigsberg <dan...@redhat.com> --- M vdsm/virt/vm.py 1 file changed, 9 insertions(+), 8 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/82/34982/1 diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index e8613f2..f53b4b9 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -2399,8 +2399,7 @@ def _saveStateInternal(self): if self.destroyed: return - with self._confLock: - toSave = deepcopy(self.status()) + toSave = self.status() toSave['startTime'] = self._startTime if self.lastStatus != vmstatus.DOWN and \ self._vmStats and self.guestAgent: @@ -2856,12 +2855,14 @@ def status(self): # used by API.Global.getVMList self.conf['status'] = self.lastStatus - # Filter out any internal keys - status = dict((k, v) for k, v in self.conf.iteritems() - if not k.startswith("_")) - status['guestDiskMapping'] = (self.guestAgent.guestDiskMapping - if self.guestAgent is not None else {}) - return status + with self._confLock: + # Filter out any internal keys + status = dict((k, v) for k, v in self.conf.iteritems() + if not k.startswith("_")) + status['guestDiskMapping'] = (self.guestAgent.guestDiskMapping + if self.guestAgent is not None + else {}) + return deepcopy(status) def getStats(self): """ -- To view, visit http://gerrit.ovirt.org/34982 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I604f8534c9380d289633df4a8151699ab0d97e34 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.5 Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches