[PATCH 2/2] Update partitions' numbers and names when adding new partition (#1166598)

Vratislav Podzimek vpodzime at redhat.com
Thu Dec 4 08:50:53 UTC 2014


On Mon, 2014-12-01 at 08:29 -0500, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: anaconda-patches at 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 at 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



More information about the anaconda-patches mailing list