[PATCH 4/5] Change management of Device parents to use a simple list interface.
David Lehman
dlehman at redhat.com
Tue Mar 25 21:28:14 UTC 2014
The parent list is an instance of class ParentList, which provides a
simplified list interface with the addition of an element uniqueness
constraint and optional user-defined pre-add and pre-remove functions.
Before, to modify a device's parent set, you'd do this:
device.parents.append(newparent)
newparent.addChild()
or this, if device is a container type:
device._addMember(newparent)
Now, regardless of the type of device, you do this:
device.parents.append(newparent)
Any checking or accounting is handled by the callbacks registered to
the parent list.
---
blivet/devices.py | 220 ++++++++++++++++++++++++++++++-----------------
tests/action_test.py | 4 +-
tests/devices_test.py | 12 ++-
tests/parentlist_test.py | 60 +++++++++++++
4 files changed, 209 insertions(+), 87 deletions(-)
create mode 100644 tests/parentlist_test.py
diff --git a/blivet/devices.py b/blivet/devices.py
index d7335c1..eaef4cf 100644
--- a/blivet/devices.py
+++ b/blivet/devices.py
@@ -113,6 +113,80 @@ def deviceNameToDiskByPath(deviceName=None):
return ret
raise DeviceNotFoundError(deviceName)
+class ParentList(object):
+ """ A list with auditing and side-effects for additions and removals.
+
+ The class provides an ordered list with guaranteed unique members and
+ optional functions to run before adding or removing a member. It
+ provides a subset of the functionality provided by :class:`list`,
+ making it easy to ensure that changes pass through the check functions.
+
+ The following operations are implemented:
+
+ .. code::
+
+ ml.append(x)
+ ml.remove(x)
+ iter(ml)
+ len(ml)
+ x in ml
+ x = ml[i] # not ml[i] = x
+ """
+ def __init__(self, items=None, appendfunc=None, removefunc=None):
+ """
+ :keyword items: initial contents
+ :type items: any iterable
+ :keyword appendfunc: a function to call before adding an item
+ :type appendfunc: callable
+ :keyword removefunc: a function to call before removing an item
+ :type removefunc: callable
+
+ appendfunc and removefunc should take the item to be added or
+ removed and perform any checks or other processing. The appropriate
+ function will be called immediately before adding or removing the
+ item. The function should raise an exception if the addition/removal
+ should not take place. :class:`~.ParentList` instance is not passed
+ to the function. While this is not optimal for general-purpose use,
+ it is ideal for the intended use as part of :class:`~.Device`
+ """
+ self.items = list()
+ if items:
+ self.items.extend(items)
+
+ self.appendfunc = appendfunc or (lambda i: True)
+ """ a function to call before adding an item """
+
+ self.removefunc = removefunc or (lambda i: True)
+ """ a function to call before removing an item """
+
+ def __iter__(self):
+ return iter(self.items)
+
+ def __contains__(self, y):
+ return y in self.items
+
+ def __getitem__(self, i):
+ return self.items[i]
+
+ def __len__(self):
+ return len(self.items)
+
+ def append(self, y):
+ """ Add an item to the list after running a callback. """
+ if y in self.items:
+ raise ValueError("item is already in the list")
+
+ self.appendfunc(y)
+ self.items.append(y)
+
+ def remove(self, y):
+ """ Remove an item from the list after running a callback. """
+ if y not in self.items:
+ raise ValueError("item is not in the list")
+
+ self.removefunc(y)
+ self.items.remove(y)
+
class Device(util.ObjectID):
""" A generic device.
@@ -159,16 +233,14 @@ class Device(util.ObjectID):
:type parents: list of :class:`Device` instances
"""
util.ObjectID.__init__(self)
+ self.kids = 0
self._name = name
- if parents is None:
- parents = []
- elif not isinstance(parents, list):
+ self.parents = []
+ if parents and not isinstance(parents, list):
raise ValueError("parents must be a list of Device instances")
- self.parents = parents
- self.kids = 0
- for parent in self.parents:
- parent.addChild()
+ if parents:
+ self.parents = parents
def __deepcopy__(self, memo):
""" Create a deep copy of a Device instance.
@@ -205,6 +277,41 @@ class Device(util.ObjectID):
s = "%s %s (%d)" % (self.type, self.name, self.id)
return s
+ def _addParent(self, parent):
+ """ Called before adding a parent to this device.
+
+ See :attr:`~.ParentList.appendfunc`.
+ """
+ parent.addChild()
+
+ def _removeParent(self, parent):
+ """ Called before removing a parent from this device.
+
+ See :attr:`~.ParentList.removefunc`.
+ """
+ parent.removeChild()
+
+ def _initParentList(self):
+ """ Initialize this instance's parent list. """
+ if not hasattr(self, "_parents"):
+ self._parents = ParentList(appendfunc=self._addParent,
+ removefunc=self._removeParent)
+
+ for parent in self._parents:
+ self._parents.remove(parent)
+
+ def _setParentList(self, parents):
+ """ Set this instance's parent list. """
+ self._initParentList()
+ for parent in parents:
+ self._parents.append(parent)
+
+ def _getParentList(self):
+ return self._parents
+
+ parents = property(_getParentList, _setParentList,
+ doc="devices upon which this device is built")
+
@property
def dict(self):
d = {"type": self.type, "name": self.name,
@@ -1137,9 +1244,7 @@ class PartitionDevice(StorageDevice):
if not exists:
# this is a request, not a partition -- it has no parents
- self.req_disks = self.parents[:]
- for dev in self.parents:
- dev.removeChild()
+ self.req_disks = list(self.parents)
self.parents = []
# FIXME: Validate partType, but only if this is a new partition
@@ -1683,14 +1788,9 @@ class PartitionDevice(StorageDevice):
"""
log_method_call(self, self.name, old=getattr(self.disk, "name", None),
new=getattr(disk, "name", None))
- if self.disk:
- self.disk.removeChild()
-
+ self.parents = []
if disk:
- self.parents = [disk]
- disk.addChild()
- else:
- self.parents = []
+ self.parents.append(disk)
disk = property(lambda p: p._getDisk(), lambda p,d: p._setDisk(d))
@@ -2037,11 +2137,9 @@ class ContainerDevice(StorageDevice):
of member devices -- one set for modifying the member set of the
python objects, and one for writing the changes to disk.
- :meth:`_addMember` and :meth:`_removeMember` operate on the object, but
- do not change anything on the disk(s). They are used when detecting
- devices (as in :class:`~.devicetree.DeviceTree`) and when manipulating
- the container objects (as in :class:`~.devicefactory.DeviceFactory` and
- :class:`~.deviceaction.ActionAddMember`).
+ The member set of the instance can be manipulated using the methods
+ :meth:`~.ParentList.append` and :meth:`~.ParentList.remove` of the
+ instance's :attr:`~.Device.parents` attribute.
:meth:`add` and :meth:`remove` remove a member from the container's on-
disk representation. These methods should normally only be called from
@@ -2060,19 +2158,9 @@ class ContainerDevice(StorageDevice):
if not self.formatClass:
raise StorageError("cannot find '%s' class" % self._formatClassName)
- # Instantiate the superclass without any parents so we can do some
- # validation on them before adding them as parents.
- parents = kwargs.pop("parents", [])
super(ContainerDevice, self).__init__(*args, **kwargs)
- # could be None
- for parent in parents or []:
- self._addMember(parent)
-
- # restore kwargs in case another class wants to use them further
- kwargs["parents"] = parents
-
- def _addMember(self, member):
+ def _addParent(self, member):
""" Add a member device to the container.
:param member: the member device to add
@@ -2081,41 +2169,15 @@ class ContainerDevice(StorageDevice):
This operates on the in-memory model and does not alter disk
contents at all.
"""
- log_method_call(self,
- self.name,
- member=member.name,
- status=self.status)
+ log_method_call(self, self.name, member=member.name)
if not isinstance(member.format, self.formatClass):
raise ValueError("member has wrong format")
- if member in self.parents:
- raise ValueError("member is already part of this container")
-
if member.format.exists and self.uuid and self._formatUUIDAttr and \
getattr(member.format, self._formatUUIDAttr) != self.uuid:
raise ValueError("cannot add member with mismatched UUID")
- self.parents.append(member)
- member.addChild()
-
- def _removeMember(self, member):
- """ Remove a member device from the container.
-
- :param member: the member device to add
- :type member: :class:`.StorageDevice`
-
- This operates on the in-memory model and does not alter disk
- contents at all.
- """
- log_method_call(self,
- self.name,
- member=member.name,
- status=self.status)
- if member not in self.parents:
- raise ValueError("member is not part of this container")
-
- self.parents.remove(member)
- member.removeChild()
+ super(ContainerDevice, self)._addParent(member)
@abc.abstractmethod
def _add(self, member):
@@ -2147,7 +2209,7 @@ class ContainerDevice(StorageDevice):
self._add(member)
if member not in self.parents:
- self._addMember(member)
+ self.parents.append(member)
@abc.abstractmethod
def _remove(self, member):
@@ -2180,7 +2242,7 @@ class ContainerDevice(StorageDevice):
self._remove(member)
if member in self.parents:
- self._removeMember(member)
+ self.parents.remove(member)
class LVMVolumeGroupDevice(ContainerDevice):
""" An LVM Volume Group
@@ -2223,7 +2285,7 @@ class LVMVolumeGroupDevice(ContainerDevice):
:keyword uuid: the VG UUID
:type uuid: str
"""
- # These attributes are used by _addMember, so they must be initialized
+ # These attributes are used by _addParent, so they must be initialized
# prior to instantiating the superclass.
self._lvs = []
self.hasDuplicate = False
@@ -2235,7 +2297,6 @@ class LVMVolumeGroupDevice(ContainerDevice):
super(LVMVolumeGroupDevice, self).__init__(name, parents=parents,
exists=exists, sysfsPath=sysfsPath)
- self.uuid = uuid
self.free = util.numeric_type(free)
self.peSize = util.numeric_type(peSize)
self.peCount = util.numeric_type(peCount)
@@ -2428,8 +2489,8 @@ class LVMVolumeGroupDevice(ContainerDevice):
if self.poolMetaData and not self.thinpools:
self.poolMetaData = 0
- def _addMember(self, member):
- super(LVMVolumeGroupDevice, self)._addMember(member)
+ def _addParent(self, member):
+ super(LVMVolumeGroupDevice, self)._addParent(member)
# now see if the VG can be activated
## XXX TODO: remove this activation code
@@ -2441,17 +2502,14 @@ class LVMVolumeGroupDevice(ContainerDevice):
len(self.parents) == self.pvCount):
self._complete = True
- def _removeMember(self, member):
+ def _removeParent(self, member):
# XXX It would be nice to raise an exception if removing this member
# would not leave enough space, but the devicefactory relies on it
# being possible to _temporarily_ overcommit the VG.
#
# Maybe removeMember could be a wrapper with the checks and the
# devicefactory could call the _ versions to bypass the checks.
- super(LVMVolumeGroupDevice, self)._removeMember(member)
-
- # and update our pv count
- self.pvCount = len(self.parents)
+ super(LVMVolumeGroupDevice, self)._removeParent(member)
# We can't rely on lvm to tell us about our size, free space, &c
# since we could have modifications queued, unless the VG and all of
@@ -2550,12 +2608,12 @@ class LVMVolumeGroupDevice(ContainerDevice):
@property
def pvs(self):
""" A list of this VG's PVs """
- return self.parents[:] # we don't want folks changing our list
+ return self.parents
@property
def lvs(self):
""" A list of this VG's LVs """
- return self._lvs[:] # we don't want folks changing our list
+ return self._lvs
@property
def thinpools(self):
@@ -3121,7 +3179,7 @@ class MDRaidArrayDevice(ContainerDevice):
:keyword minor: the device minor (obsolete?)
:type minor: int
"""
- # These attributes are used by _addMember, so they must be initialized
+ # These attributes are used by _addParent, so they must be initialized
# prior to instantiating the superclass.
self._memberDevices = 0
self._totalDevices = 0
@@ -3360,8 +3418,8 @@ class MDRaidArrayDevice(ContainerDevice):
else:
self.sysfsPath = ''
- def _addMember(self, member):
- super(MDRaidArrayDevice, self)._addMember(member)
+ def _addParent(self, member):
+ super(MDRaidArrayDevice, self)._addParent(member)
## XXX TODO: remove this whole block of activation code
if self.exists and member.format.exists and flags.installer_mode:
@@ -3387,11 +3445,11 @@ class MDRaidArrayDevice(ContainerDevice):
self.memberDevices += 1
- def _removeMember(self, member):
+ def _removeParent(self, member):
if self.level.name == "raid0" and self.exists and member.format.exists:
raise DeviceError("cannot remove members from existing raid0")
- super(MDRaidArrayDevice, self)._removeMember(member)
+ super(MDRaidArrayDevice, self)._removeParent(member)
self.memberDevices -= 1
@property
@@ -4585,7 +4643,7 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
return size
- def _removeMember(self, member):
+ def _removeParent(self, member):
# btrfs won't let you degrade it
limits = []
levels = raid.RAIDLevels()
@@ -4602,7 +4660,7 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
raise DeviceError("cannot remove member due to raid level "
"constraints")
- super(BTRFSVolumeDevice, self)._removeMember(member)
+ super(BTRFSVolumeDevice, self)._removeParent(member)
def _addSubVolume(self, vol):
if vol.name in [v.name for v in self.subvolumes]:
diff --git a/tests/action_test.py b/tests/action_test.py
index 2f207c0..0468c6e 100644
--- a/tests/action_test.py
+++ b/tests/action_test.py
@@ -575,7 +575,7 @@ class DeviceActionTestCase(StorageTestCase):
sda2 = self.newDevice(device_class=PartitionDevice, name="sda2",
size=Size(spec="99.5 GiB"), parents=[sda])
- sda2_format = self.newFormat("lvmpv", device=sda1.path)
+ sda2_format = self.newFormat("lvmpv", device=sda2.path)
self.scheduleCreateDevice(device=sda2)
self.scheduleCreateFormat(device=sda2, format=sda2_format)
@@ -1016,7 +1016,7 @@ class DeviceActionTestCase(StorageTestCase):
self.assertEqual(remove_sdb1.requires(add_sdc1), True)
- vg._addMember(sdb1)
+ vg.parents.append(sdb1)
remove_sdb1_2 = ActionRemoveMember(vg, sdb1)
self.assertEqual(remove_sdb1_2.obsoletes(remove_sdb1), False)
self.assertEqual(remove_sdb1.obsoletes(remove_sdb1_2), True)
diff --git a/tests/devices_test.py b/tests/devices_test.py
index c69389d..38bcb04 100644
--- a/tests/devices_test.py
+++ b/tests/devices_test.py
@@ -15,7 +15,8 @@ from blivet.devices import BTRFSVolumeDevice
from blivet.devices import MDRaidArrayDevice
from blivet.devices import OpticalDevice
from blivet.devices import StorageDevice
-from blivet.devices import mdraid
+from blivet.devices import ParentList
+from blivet.devicelibs import mdraid
from blivet.devicelibs import btrfs
from blivet.size import Size
@@ -65,7 +66,8 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
"createBitmap" : lambda x,m: self.assertTrue(x, m),
"currentSize" : lambda x, m: self.assertEqual(x, Size(bytes=0), m),
"description" : self.assertIsNotNone,
- "devices" : lambda x, m: self.assertEqual(x, [], m),
+ "devices" : lambda x, m: self.assertEqual(len(x), 0, m) and
+ self.assertIsInstance(x, ParentList, m),
"exists" : self.assertFalse,
"format" : self.assertIsNotNone,
"formatArgs" : lambda x, m: self.assertEqual(x, [], m),
@@ -77,7 +79,8 @@ class MDRaidArrayDeviceTestCase(DeviceStateTestCase):
"mediaPresent" : self.assertFalse,
"metadataVersion" : lambda x, m: self.assertEqual(x, "default", m),
"minor" : lambda x, m: self.assertEqual(x, 0, m),
- "parents" : lambda x, m: self.assertEqual(x, [], m),
+ "parents" : lambda x, m: self.assertEqual(len(x), 0, m) and
+ self.assertIsInstance(x, ParentList, m),
"path" : lambda x, m: self.assertRegexpMatches(x, "^/dev", m),
"partitionable" : self.assertFalse,
"rawArraySize" : lambda x, m: self.assertEqual(x, Size(bytes=0), m),
@@ -447,7 +450,8 @@ class BTRFSDeviceTestCase(DeviceStateTestCase):
"maxSize" : lambda x, m: self.assertEqual(x, Size(bytes=0), m),
"mediaPresent" : self.assertTrue,
"minor" : lambda x, m: self.assertEqual(x, 0, m),
- "parents" : lambda x, m: self.assertEqual(x, [], m),
+ "parents" : lambda x, m: self.assertEqual(len(x), 0, m) and
+ self.assertIsInstance(x, ParentList, m),
"partitionable" : self.assertFalse,
"path" : lambda x, m: self.assertRegexpMatches(x, "^/dev", m),
"resizable" : lambda x, m: self.assertFalse,
diff --git a/tests/parentlist_test.py b/tests/parentlist_test.py
new file mode 100644
index 0000000..fa874a8
--- /dev/null
+++ b/tests/parentlist_test.py
@@ -0,0 +1,60 @@
+
+import unittest
+from blivet.devices import ParentList
+
+class ParentListTest(unittest.TestCase):
+ def testParentList(self):
+ items = range(5)
+ length = len(items)
+ pl = ParentList(items=items)
+ self.assertEqual(len(pl), length)
+ self.assertEqual(list(pl), items)
+
+ self.assertEqual(hasattr(pl, "index"), False)
+ self.assertEqual(hasattr(pl, "insert"), False)
+ self.assertEqual(hasattr(pl, "pop"), False)
+
+ with self.assertRaises(TypeError):
+ pl[2] = 99
+
+ newval = 99
+ self.assertEqual(99 in pl, False)
+ pl.append(newval)
+ length += 1
+ self.assertEqual(len(pl), length)
+ self.assertEqual(newval in pl, True)
+
+ val = 3
+ self.assertEqual(val in pl, True)
+ pl.remove(val)
+ length -= 1
+ self.assertEqual(len(pl), length)
+ self.assertEqual(val in pl, False)
+
+ #
+ # verify that add/remove functions work as expected
+ #
+ def pre_add(item):
+ if item > 32:
+ raise ValueError("only numbers less than 32 are allowed")
+
+ def pre_remove(item):
+ if len(pl) - 1 < 3:
+ raise RuntimeError("list can never have fewer than 3 items")
+
+ pl = ParentList(items=items, appendfunc=pre_add, removefunc=pre_remove)
+
+ self.assertRaises(ValueError, pl.append, 33)
+
+ pl.remove(4)
+ pl.remove(3)
+ self.assertRaises(RuntimeError, pl.remove, 2)
+
+
+def suite():
+ return unittest.TestLoader().loadTestsFromTestCase(TopologicalSortTestCase)
+
+
+if __name__ == "__main__":
+ unittest.main()
+
--
1.8.5.3
More information about the anaconda-patches
mailing list