[PATCH 4/5] Change management of Device parents to use a simple list interface.
David Lehman
dlehman at redhat.com
Mon Mar 24 18:50:51 UTC 2014
The parent list is an instance of class ManagedList, which provides a
simplified list interface with the addition of pluggable pre-add and
pre-remove callbacks.
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 | 205 ++++++++++++++++++++++++++++++--------------------
tests/action_test.py | 4 +-
tests/devices_test.py | 12 ++-
3 files changed, 134 insertions(+), 87 deletions(-)
diff --git a/blivet/devices.py b/blivet/devices.py
index d7335c1..68230a9 100644
--- a/blivet/devices.py
+++ b/blivet/devices.py
@@ -113,6 +113,76 @@ def deviceNameToDiskByPath(deviceName=None):
return ret
raise DeviceNotFoundError(deviceName)
+class ManagedList(object):
+ """ A list with auditing and side-effects for additions and removals.
+
+ This class allows specification of callbacks to run before adding or
+ removing an item. It provides a subset of the functionality provided by
+ :class:`list`, making it easy to ensure the callbacks are run.
+
+ The following operations are implemented:
+
+ 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 run before adding an item
+ :type appendfunc: callable
+ :keyword removefunc: a function to run 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 functions
+ will be called immediately before adding or removing the item.
+ """
+ args = []
+ if items:
+ args.append(items)
+
+ self.items = list(*args)
+
+ self.appendfunc = appendfunc
+ self.removefunc = removefunc
+
+ 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")
+
+ if self.appendfunc:
+ 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")
+
+ if self.removefunc:
+ self.removefunc(y)
+
+ self.items.remove(y)
+
class Device(util.ObjectID):
""" A generic device.
@@ -159,16 +229,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 +273,30 @@ class Device(util.ObjectID):
s = "%s %s (%d)" % (self.type, self.name, self.id)
return s
+ def _addParent(self, parent):
+ parent.addChild()
+
+ def _removeParent(self, parent):
+ parent.removeChild()
+
+ def _initParentList(self):
+ # when this is called initially the device has no parents attribute
+ for parent in getattr(self, "parents", []):
+ parent.removeChild()
+
+ self._parents = ManagedList(appendfunc=self._addParent,
+ removefunc=self._removeParent)
+
+ def _setParentList(self, parents):
+ self._initParentList()
+ for parent in parents:
+ self._parents.append(parent)
+
+ def _getParentList(self):
+ return self._parents
+
+ parents = property(_getParentList, _setParentList)
+
@property
def dict(self):
d = {"type": self.type, "name": self.name,
@@ -1137,9 +1229,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 +1773,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 +2122,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:`~.ManagedList.append` and :meth:`~.ManagedList.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 +2143,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 +2154,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 +2194,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 +2227,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 +2270,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 +2282,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 +2474,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 +2487,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 +2593,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 +3164,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 +3403,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 +3430,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 +4628,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 +4645,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..1d30f21 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 ManagedList
+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, ManagedList, 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, ManagedList, 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, ManagedList, m),
"partitionable" : self.assertFalse,
"path" : lambda x, m: self.assertRegexpMatches(x, "^/dev", m),
"resizable" : lambda x, m: self.assertFalse,
--
1.8.5.3
More information about the anaconda-patches
mailing list