Change in vdsm[ovirt-3.5]: vm: return full copy of the conf data on status()

fromani at redhat.com fromani at redhat.com
Mon Nov 10 08:17:13 UTC 2014


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 <fromani at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/34147
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-by: Michal Skrivanek <michal.skrivanek at redhat.com>
Reviewed-by: Martin Polednik <mpolednik at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at 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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list