On Mon, 2014-12-01 at 08:29 -0500, Anne Mulhern wrote:
----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime(a)redhat.com>
> To: anaconda-patches(a)lists.fedorahosted.org
> Sent: Wednesday, November 26, 2014 8:37:32 AM
> Subject: [PATCH 2/2] Update partitions' numbers and names when adding
new partition (#1166598)
>
> Adding a new partition may change partitions' order (e.g. adding sda1 after
> sda2) so we need to handle that correctly.
>
> Signed-off-by: Vratislav Podzimek <vpodzime(a)redhat.com>
> ---
> blivet/devicetree.py | 9 +++++++++
> blivet/formats/disklabel.py | 26 ++++++++++++++++++++++++--
> python-blivet.spec | 2 +-
> 3 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index f6cf600..98b1251 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -26,6 +26,7 @@ import re
> import shutil
> import pprint
> import copy
> +import itertools
>
> from .errors import CryptoError, DeviceError, DeviceTreeError,
> DiskLabelCommitError, DMError, FSError, InvalidDiskLabelError, LUKSError,
> MDRaidError, StorageError
> from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice,
> BTRFSSnapShotDevice
> @@ -402,6 +403,14 @@ class DeviceTree(object):
> newdev.addHook(new=new)
> self._devices.append(newdev)
>
> + # adding a partition may change partedPartition's paths which are
> used
> + # to construct names which thus should be updated
> + if isinstance(newdev, PartitionDevice):
> + sibling_parts_lists = [self.getChildren(parent) for parent in
> newdev.parents]
Any reason not to make the above lazy, instead of a list?
The reason was that
itertools.chain() is a bit weird and combination of
list + the * expansion was finally a working one. But a generator would
work the same.
> + sibling_parts = itertools.chain(*sibling_parts_lists)
> + for part in sibling_parts:
> + part.updateName()
> +
> # don't include "req%d" partition names
> if ((newdev.type != "partition" or
> not newdev.name.startswith("req")) and
> diff --git a/blivet/formats/disklabel.py b/blivet/formats/disklabel.py
> index c69aa04..590b58c 100644
> --- a/blivet/formats/disklabel.py
> +++ b/blivet/formats/disklabel.py
> @@ -311,8 +311,30 @@ class DiskLabel(DeviceFormat):
> geometry=geometry)
>
> constraint = parted.Constraint(exactGeom=geometry)
> - self.partedDisk.addPartition(partition=new_partition,
> - constraint=constraint)
> +
> + # adding a partition the start of which is before some already added
> + # partition requires change of partitions' order because parted just
> + # increments the number while partitions are added
> + if any(part.geometry.start > start for part in
> self.partedDisk.partitions):
> + parts = self.partedDisk.partitions[:]
> +
> + # first remove all partitions
> + for part in parts:
> + self.partedDisk.removePartition(part)
> +
> + # then add them back sorted together with the new one
> + parts.append(new_partition)
> + parts.sort(key=lambda p: p.geometry.start)
> + for part in parts:
> + # reset the partition's number so that it gets a new one
> when
> + # added to partedDisk
> + part.resetNumber()
It feels like it would be better to handle this number resetting within pyparted,
probably in Disk.addPartition().
It definitely feels like that, but parted/pyparted
has very little
information about the partitions. It has no idea if the given partition
exists on a disk or not, if we are putting it back or it's completely
new or if we are doing something weird like reverting actions or just we
just want to add a new partition.
> + constr = parted.Constraint(exactGeom=part.geometry)
> + self.partedDisk.addPartition(partition=part,
> + constraint=constr)
> + else:
> + self.partedDisk.addPartition(partition=new_partition,
> +
overall, the above code would be simpler if partitions were always kept sorted
by start, and this method could rely on that holding true.
Cannot agree more. But
as it turned out yesterday, we cannot even rely
on partitions existing on a disk having their numbers in the ascending
order when ordered by start.
This patch had to be reverted and we need something better for master
(and rhel7-branch too, I'm afraid).
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic