[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