[PATCH 3/5] Improve handling of device removals/additions from the devicetree.

David Lehman dlehman at redhat.com
Tue Sep 16 21:01:51 UTC 2014


Move device-type-specific code for managing relationships between devices
and the devices that contain them into the appropriate classes. This will
ensure, among other things, that canceling a destroy action for an lv
correctly adds the lv back into the LVMVolumeGroupDevice's lvs list.

(cherry picked from commit bd1cb17ae95682c82b318da3ce8077a23d1f7dca)

Fedora:
Resolves: rhbz#1121383

RHEL:
Resolves: rhbz#1085201
Resolves: rhbz#1077906
Resolves: rhbz#1129595
Resolves: rhbz#1075671
---
 blivet/__init__.py     |   2 +-
 blivet/devices.py      | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
 blivet/devicetree.py   |  43 +++++---------------
 blivet/partitioning.py |   4 +-
 4 files changed, 122 insertions(+), 35 deletions(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index de76353..c809f55 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -878,7 +878,7 @@ class Blivet(object):
                         # We can't schedule the magic partition for removal
                         # because parted will not allow us to remove it from the
                         # disk. Still, we need it out of the devicetree.
-                        self.devicetree._removeDevice(part, moddisk=False)
+                        self.devicetree._removeDevice(part, modparent=False)
 
             if len(disk.format.partitions) > expected:
                 raise ValueError("cannot initialize a disk that has partitions")
diff --git a/blivet/devices.py b/blivet/devices.py
index a0f8f56..ba8de63 100644
--- a/blivet/devices.py
+++ b/blivet/devices.py
@@ -1123,6 +1123,33 @@ class StorageDevice(Device):
             return -1
         return 0
 
+    # pylint: disable=unused-argument
+    def removeHook(self, modparent=True):
+        """ Perform actions related to removing a device from the devicetree.
+
+            :keyword bool modparent: whether to account for removal in parents
+
+            Parent child counts are adjusted regardless of modparent's value.
+            The intended use of modparent is to prevent doing things like
+            removing a parted.Partition from the disk that contains it as part
+            of msdos extended partition management. In general, you should not
+            override the default value of modparent in new code.
+        """
+        for parent in self.parents:
+            parent.removeChild()
+
+    def addHook(self, new=True):
+        """ Perform actions related to adding a device to the devicetree.
+
+            :keyword bool new: whether this device is new to the devicetree
+
+            The only intended use case for new=False is when unhiding a device
+            from the devicetree. Additional operations are performed when new is
+            False that are normally performed as part of the device constructor.
+        """
+        if not new:
+            map(lambda p: p.addChild(), self.parents)
+
     def populateKSData(self, data):
         # the common pieces are basically the formatting
         self.format.populateKSData(data)
@@ -1657,6 +1684,43 @@ class PartitionDevice(StorageDevice):
         magic = self.disk.format.magicPartitionNumber
         return (number == magic)
 
+    def removeHook(self, modparent=True):
+        if modparent:
+            # if this partition hasn't been allocated it could not have
+            # a disk attribute
+            if not self.disk:
+                return
+
+            if self.partedPartition.type == parted.PARTITION_EXTENDED and \
+                    len(self.disk.format.logicalPartitions) > 0:
+                raise ValueError("Cannot remove extended partition %s.  "
+                        "Logical partitions present." % self.name)
+
+            self.disk.format.removePartition(self.partedPartition)
+
+        super(PartitionDevice, self).removeHook(modparent=modparent)
+
+    def addHook(self, new=True):
+        super(PartitionDevice, self).addHook(new=new)
+        if new:
+            return
+
+        if not self.disk or not self.partedPartition or \
+           self.partedPartition in self.disk.format.partitions:
+            return
+
+        self.disk.format.addPartition(self.partedPartition)
+
+        # Look up the path by start sector to deal with automatic renumbering of
+        # logical partitions on msdos disklabels.
+        if self.isExtended:
+            partition = self.disk.format.extendedPartition
+        else:
+            start = self.partedPartition.geometry.start
+            partition = self.disk.format.partedDisk.getPartitionBySector(start)
+
+        self.partedPartition = partition
+
     def probe(self):
         """ Probe for any missing information about this device.
 
@@ -3087,6 +3151,20 @@ class LVMLogicalVolumeDevice(DMDevice):
             return -1
         return 0
 
+    def removeHook(self, modparent=True):
+        if modparent:
+            self.vg._removeLogVol(self)
+
+        super(LVMLogicalVolumeDevice, self).removeHook(modparent=modparent)
+
+    def addHook(self, new=True):
+        super(LVMLogicalVolumeDevice, self).addHook(new=new)
+        if new:
+            return
+
+        if self not in self.vg.lvs:
+            self.vg._addLogVol(self)
+
     def populateKSData(self, data):
         super(LVMLogicalVolumeDevice, self).populateKSData(data)
         data.vgname = self.vg.name
@@ -3472,6 +3550,22 @@ class LVMThinLogicalVolumeDevice(LVMLogicalVolumeDevice):
         lvm.thinlvcreate(self.vg.name, self.pool.lvname, self.lvname,
                          self.size)
 
+    def removeHook(self, modparent=True):
+        if modparent:
+            self.pool._removeLogVol(self)
+
+        # pylint: disable=bad-super-call
+        super(LVMLogicalVolumeDevice, self).removeHook(modparent=modparent)
+
+    def addHook(self, new=True):
+        # pylint: disable=bad-super-call
+        super(LVMLogicalVolumeDevice, self).addHook(new=new)
+        if new:
+            return
+
+        if self not in self.pool.lvs:
+            self.pool._addLogVol(self)
+
     def populateKSData(self, data):
         super(LVMThinLogicalVolumeDevice, self).populateKSData(data)
         data.thin_volume = True
@@ -5358,6 +5452,20 @@ class BTRFSSubVolumeDevice(BTRFSDevice):
         btrfs.delete_subvolume(mountpoint, self.name)
         self.volume._undo_temp_mount()
 
+    def removeHook(self, modparent=True):
+        if modparent:
+            self.volume._removeSubVolume(self.name)
+
+        super(BTRFSSubVolumeDevice, self).removeHook(modparent=modparent)
+
+    def addHook(self, new=True):
+        super(BTRFSSubVolumeDevice, self).addHook(new=new)
+        if new:
+            return
+
+        if self not in self.volume.subvolumes:
+            self.volume._addSubVolume(self)
+
     def populateKSData(self, data):
         super(BTRFSSubVolumeDevice, self).populateKSData(data)
         data.subvol = True
diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 7c92288..ea45b2a 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -374,7 +374,7 @@ class DeviceTree(object):
 
         self._postProcessActions()
 
-    def _addDevice(self, newdev):
+    def _addDevice(self, newdev, new=True):
         """ Add a device to the tree.
 
             :param newdev: the device to add
@@ -392,6 +392,7 @@ class DeviceTree(object):
             if parent not in self._devices:
                 raise DeviceTreeError("parent device not in tree")
 
+        newdev.addHook(new=new)
         self._devices.append(newdev)
 
         # don't include "req%d" partition names
@@ -404,15 +405,15 @@ class DeviceTree(object):
                                                        newdev.name,
                                                        newdev.id)
 
-    def _removeDevice(self, dev, force=None, moddisk=True):
+    def _removeDevice(self, dev, force=None, modparent=True):
         """ Remove a device from the tree.
 
             :param dev: the device to remove
             :type dev: a subclass of :class:`~.devices.StorageDevice`
             :keyword force: whether to force removal of a non-leaf device
             :type force: bool
-            :keyword moddisk: update parent disk's format (partitions only)
-            :type moddisk: bool
+            :keyword modparent: update parent device to account for removal
+            :type modparent: bool
 
             .. note::
 
@@ -425,18 +426,10 @@ class DeviceTree(object):
             log.debug("%s has %d kids", dev.name, dev.kids)
             raise ValueError("Cannot remove non-leaf device '%s'" % dev.name)
 
-        if moddisk:
+        dev.removeHook(modparent=modparent)
+        if modparent:
             # if this is a partition we need to remove it from the parted.Disk
             if isinstance(dev, PartitionDevice) and dev.disk is not None:
-                # if this partition hasn't been allocated it could not have
-                # a disk attribute
-                if dev.partedPartition.type == parted.PARTITION_EXTENDED and \
-                        len(dev.disk.format.logicalPartitions) > 0:
-                    raise ValueError("Cannot remove extended partition %s.  "
-                            "Logical partitions present." % dev.name)
-
-                dev.disk.format.removePartition(dev.partedPartition)
-
                 # adjust all other PartitionDevice instances belonging to the
                 # same disk so the device name matches the potentially altered
                 # name of the parted.Partition
@@ -444,12 +437,6 @@ class DeviceTree(object):
                     if isinstance(device, PartitionDevice) and \
                        device.disk == dev.disk:
                         device.updateName()
-            elif hasattr(dev, "pool"):
-                dev.pool._removeLogVol(dev)
-            elif hasattr(dev, "vg"):
-                dev.vg._removeLogVol(dev)
-            elif hasattr(dev, "volume"):
-                dev.volume._removeSubVolume(dev.name)
 
         self._devices.remove(dev)
         if dev.name in self.names and getattr(dev, "complete", True):
@@ -458,20 +445,15 @@ class DeviceTree(object):
                                                            dev.name,
                                                            dev.id)
 
-        for parent in dev.parents:
-            # Will this cause issues with garbage collection?
-            #   Do we care about garbage collection? At all?
-            parent.removeChild()
-
     def _removeChildrenFromTree(self, device):
         devs_to_remove = self.getDependentDevices(device)
         while devs_to_remove:
             leaves = [d for d in devs_to_remove if d.isleaf]
             for leaf in leaves:
-                self._removeDevice(leaf, moddisk=False)
+                self._removeDevice(leaf, modparent=False)
                 devs_to_remove.remove(leaf)
             if len(devs_to_remove) == 1 and devs_to_remove[0].isExtended:
-                self._removeDevice(devs_to_remove[0], force=True, moddisk=False)
+                self._removeDevice(devs_to_remove[0], force=True, modparent=False)
                 break
 
     def registerAction(self, action):
@@ -521,7 +503,7 @@ class DeviceTree(object):
             self._removeDevice(action.device)
         elif action.isDestroy and action.isDevice:
             # add the device back into the tree
-            self._addDevice(action.device)
+            self._addDevice(action.device, new=False)
 
         action.cancel()
         self._actions.remove(action)
@@ -1969,7 +1951,7 @@ class DeviceTree(object):
         if not device.exists:
             return
 
-        self._removeDevice(device, force=True, moddisk=False)
+        self._removeDevice(device, force=True, modparent=False)
 
         self._hidden.append(device)
         lvm.lvm_cc_addFilterRejectRegexp(device.name)
@@ -2002,9 +1984,6 @@ class DeviceTree(object):
                 self._hidden.remove(hidden)
                 self._devices.append(hidden)
                 lvm.lvm_cc_removeFilterRejectRegexp(hidden.name)
-                for parent in hidden.parents:
-                    parent.addChild()
-
                 if isinstance(device, DASDDevice):
                     self.dasd.append(device)
 
diff --git a/blivet/partitioning.py b/blivet/partitioning.py
index 957ce04..82a3fcb 100644
--- a/blivet/partitioning.py
+++ b/blivet/partitioning.py
@@ -805,7 +805,7 @@ def updateExtendedPartitions(storage, disks):
                     if part.exists:
                         storage.destroyDevice(part)
                     else:
-                        storage.devicetree._removeDevice(part, moddisk=False)
+                        storage.devicetree._removeDevice(part, modparent=False)
             continue
 
         extendedName = devicePathToName(extended.getDeviceNodeName())
@@ -822,7 +822,7 @@ def updateExtendedPartitions(storage, disks):
                 if part.exists:
                     storage.destroyDevice(part)
                 else:
-                    storage.devicetree._removeDevice(part, moddisk=False)
+                    storage.devicetree._removeDevice(part, modparent=False)
 
         if device:
             continue
-- 
1.9.3



More information about the anaconda-patches mailing list