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 <[email protected]> --- 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 <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
