Change in vdsm[master]: domainMonitor: Fix unsafe status handling
nsoffer at redhat.com
nsoffer at redhat.com
Sat Jun 21 00:11:26 UTC 2014
Nir Soffer has uploaded a new change for review.
Change subject: domainMonitor: Fix unsafe status handling
......................................................................
domainMonitor: Fix unsafe status handling
DomainMonitor status was updated in a unsafe way, assuming that another
thread will never ask the status in the middle of an update. The code
was written as if creating a new status object is expensive operation
that should be avoided.
Instead of having long living status object in the monitor, I create new
forozen copy each monitor internal (10 seconds). The frozen copy ensure
that careless developers trying to modify a status will get what they
deserve.
Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
M tests/Makefile.am
A tests/domainMonitorTests.py
M vdsm/storage/domainMonitor.py
3 files changed, 59 insertions(+), 14 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/14/29014/1
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6507165..e35ae1c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -30,6 +30,7 @@
capsTests.py \
clientifTests.py \
configNetworkTests.py \
+ domainMonitorTests.py \
fileVolumeTests.py \
fileUtilTests.py \
fuserTests.py \
diff --git a/tests/domainMonitorTests.py b/tests/domainMonitorTests.py
new file mode 100644
index 0000000..b068ec8
--- /dev/null
+++ b/tests/domainMonitorTests.py
@@ -0,0 +1,43 @@
+#
+# Copyright 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+from storage import domainMonitor
+from testrunner import VdsmTestCase
+
+
+class FrozenStatusTests(VdsmTestCase):
+
+ def setUp(self):
+ self.status = domainMonitor.DomainMonitorStatus()
+ self.frozen = domainMonitor.FrozenStatus(self.status)
+
+ def test_copy_attributes(self):
+ for name in self.status.__slots__:
+ value = getattr(self.status, name)
+ expected = getattr(self.frozen, name)
+ self.assertEquals(value, expected)
+
+ def test_setting_attribute_raises(self):
+ for name in self.status.__slots__:
+ self.assertRaises(AssertionError, setattr, self.frozen, name, 1)
+
+ def test_deleting_attribute_raises(self):
+ for name in self.status.__slots__:
+ self.assertRaises(AssertionError, delattr, self.frozen, name)
diff --git a/vdsm/storage/domainMonitor.py b/vdsm/storage/domainMonitor.py
index 8a50b4f..baa5588 100644
--- a/vdsm/storage/domainMonitor.py
+++ b/vdsm/storage/domainMonitor.py
@@ -38,9 +38,6 @@
)
def __init__(self):
- self.clear()
-
- def clear(self):
self.error = None
self.checkTime = time()
self.valid = True
@@ -62,14 +59,18 @@
self.isoPrefix = None
self.version = -1
- def update(self, st):
- for attr in self.__slots__:
- setattr(self, attr, getattr(st, attr))
- def copy(self):
- res = DomainMonitorStatus()
- res.update(self)
- return res
+class FrozenStatus(DomainMonitorStatus):
+
+ def __init__(self, other):
+ for name in other.__slots__:
+ value = getattr(other, name)
+ super(FrozenStatus, self).__setattr__(name, value)
+
+ def __setattr__(self, *args):
+ raise AssertionError('%s is readonly' % self)
+
+ __delattr__ = __setattr__
class DomainMonitor(object):
@@ -141,8 +142,8 @@
self.hostId = hostId
self.interval = interval
self.firstChange = True
- self.status = DomainMonitorStatus()
self.nextStatus = DomainMonitorStatus()
+ self.status = FrozenStatus(self.nextStatus)
self.isIsoDomain = None
self.isoPrefix = None
self.lastRefresh = time()
@@ -158,7 +159,7 @@
self.thread.join()
def getStatus(self):
- return self.status.copy()
+ return self.status
@utils.traceback(on=log.name)
def _monitorLoop(self):
@@ -184,7 +185,7 @@
"%s", self.hostId, self.sdUUID, exc_info=True)
def _monitorDomain(self):
- self.nextStatus.clear()
+ self.nextStatus = DomainMonitorStatus()
if time() - self.lastRefresh > self.refreshTime:
# Refreshing the domain object in order to pick up changes as,
@@ -265,7 +266,7 @@
"request for domain %s", self.hostId,
self.sdUUID, exc_info=True)
- self.status.update(self.nextStatus)
+ self.status = FrozenStatus(self.nextStatus)
def _statusDidChange(self):
return self.firstChange or self.status.valid != self.nextStatus.valid
--
To view, visit http://gerrit.ovirt.org/29014
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
More information about the vdsm-patches
mailing list