[PATCH 6/6] Require resize target sizes to yield aligned partitions. (#1120964)

Anne Mulhern amulhern at redhat.com
Wed Oct 29 16:30:54 UTC 2014





----- Original Message -----
> From: "David Lehman" <dlehman at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Wednesday, October 29, 2014 9:59:13 AM
> Subject: Re: [PATCH 6/6] Require resize target sizes to yield aligned	partitions. (#1120964)
> 
> On 10/29/2014 07:58 AM, Anne Mulhern wrote:
> >
> >
> > ----- Original Message -----
> > From: "David Lehman" <dlehman at redhat.com>
> > To: anaconda-patches at lists.fedorahosted.org
> > Sent: Tuesday, October 28, 2014 3:20:03 PM
> > Subject: [PATCH 6/6] Require resize target sizes to yield aligned
> > partitions.	(#1120964)
> >
> > New method StorageDevice.alignTargetSize can be used to modify target
> > sizes as needed to account for alignment.
> >
> > Modify minSize and maxSize to return values that will yield an aligned
> > partition.
> > ---
> >   blivet/devices/partition.py |  47 ++++++++++-
> >   blivet/devices/storage.py   |  17 +++-
> >   tests/devices_test.py       | 193
> >   ++++++++++++++++++++++++++++++++++++++++++++
> >   tests/storagetestcase.py    |   2 +
> >   4 files changed, 255 insertions(+), 4 deletions(-)
> >

<-- SNIP -->

> > +
> > +class PartitionDeviceTestCase(unittest.TestCase):
> > +
> > +    def testTargetSize(self):
> > +        with sparsetmpfile("targetsizetest", Size("10 MiB")) as disk_file:
> > +            disk = DiskFile(disk_file)
> > +            disk.format = getFormat("disklabel", device=disk.path)
> > +            grain_size = disk.format.alignment.grainSize
> > +            sector_size = disk.format.partedDevice.sectorSize
> >
> > !!! Could you turn the two above into Sizes(), to make their units
> > explicit?
> 
> Size is just a number underneath, but I suppose I can go through and
> make everything explicit if it will make things easier to read.
> 

My main goal is to make it explicit that disk.format.alignment.grainSize
is a value in bytes. Size(disk.format.alignment.grainSize) makes it
clear that's what the programmer believes.

> >
> > +            start = grain_size
> > +            orig_size = Size("6 MiB")
> > +            end = start + (orig_size / sector_size) - 1
> >
> > !!! This line is confusing to me, because of the mix of units.
> > It seems that start should be a Size, but orig_size / sector_size has no
> > units,
> > and I'm not sure about 1.
> 
> Start is a sector number, which is an integer.
> 

Ok. So is the fact that it's equal to grain_size, which is a Size() just a useful
choice for the test?

> >
> > +            disk.format.addPartition(start, end)
> > +            partition = disk.format.partedDisk.getPartitionBySector(start)
> > +            self.assertNotEqual(partition, None)
> > +            self.assertEqual(orig_size,
> > Size(partition.getLength(unit='B')))
> > +
> > +            device = PartitionDevice(os.path.basename(partition.path),
> > +                                     size=orig_size)
> > +            device.disk = disk
> > +            device.exists = True
> > +            device.partedPartition = partition
> > +
> > +            device.format = getFormat("ext4", device=device.path)
> > +            device.format.exists = True
> > +            # grain size should be 1 MiB
> > +            device.format._minInstanceSize = Size("2 MiB") + (grain_size /
> > 2)
> > +
> > +            # Make sure things are as expected to begin with.
> > +            self.assertEqual(device.size, orig_size)
> > +            self.assertEqual(device.minSize, Size("3 MiB"))
> > +            # start sector's at 1 MiB
> > +            self.assertEqual(device.maxSize, Size("9 MiB"))
> > +
> > +            # ValueError if not Size
> > +            with self.assertRaisesRegexp(ValueError,
> > +                                         "new size must.*type Size"):
> > +                device.targetSize = 22
> > +
> > +            self.assertEqual(device.targetSize, orig_size)
> > +
> > +            # ValueError if size smaller than minSize
> > +            with self.assertRaisesRegexp(ValueError,
> > +                                         "size.*smaller than the
> > minimum"):
> > +                device.targetSize = Size("1 MiB")
> > +
> > +            self.assertEqual(device.targetSize, orig_size)
> > +
> > +            # ValueError if size larger than maxSize
> > +            with self.assertRaisesRegexp(ValueError,
> > +                                         "size.*larger than the maximum"):
> > +                device.targetSize = Size("11 MiB")
> > +
> > +            self.assertEqual(device.targetSize, orig_size)
> > +
> > +            # ValueError if unaligned
> > +            with self.assertRaisesRegexp(ValueError, "new
> > size.*not.*aligned"):
> > +                device.targetSize = Size("3.1 MiB")
> > +
> > +            self.assertEqual(device.targetSize, orig_size)
> > +
> > +            # successfully set a new target size
> > +            new_target = device.maxSize
> > +            device.targetSize = new_target
> > +            self.assertEqual(device.targetSize, new_target)
> > +            self.assertEqual(device.size, new_target)
> > +            parted_size = Size(device.partedPartition.getLength(unit='B'))
> > +            self.assertEqual(parted_size, device.targetSize)
> > +
> > +            # reset target size to original size
> > +            device.targetSize = orig_size
> > +            self.assertEqual(device.targetSize, orig_size)
> > +            self.assertEqual(device.size, orig_size)
> > +            parted_size = Size(device.partedPartition.getLength(unit='B'))
> > +            self.assertEqual(parted_size, device.targetSize)
> > +
> > +    def testMinMaxSizeAlignment(self):
> > +        with sparsetmpfile("minsizetest", Size("10 MiB")) as disk_file:
> > +            disk = DiskFile(disk_file)
> > +            disk.format = getFormat("disklabel", device=disk.path)
> > +            grain_size = disk.format.alignment.grainSize
> > +            sector_size = disk.format.partedDevice.sectorSize
> >
> > !!! Also be explicit about sizes here.
> >
> > +            start = grain_size
> > +            end = start + (Size("6 MiB") / sector_size)
> >
> > !!! Here, too is a somewhat confusing mix of units.
> 
> Size is an integer byte count represented by decimal.Decimal under the
> covers. It's just integer math.
> 

Yup, I know how Size is represented.

If start is really a sector number and not a Size() this makes sense.

> >
> > +            disk.format.addPartition(start, end)
> > +            partition = disk.format.partedDisk.getPartitionBySector(start)
> > +            self.assertNotEqual(partition, None)
> > +
> > +            device = PartitionDevice(os.path.basename(partition.path))
> > +            device.disk = disk
> > +            device.exists = True
> > +            device.partedPartition = partition
> > +
> > +            # Typical sector size is 512 B.
> > +            # Default optimum alignment grain size is 2048 sectors, or 1
> > MiB.
> > +            device.format = getFormat("ext4", device=device.path)
> > +            device.format.exists = True
> > +            device.format._minInstanceSize = Size("2 MiB") + (grain_size /
> > 2)
> > +
> > +            ##
> > +            ## minSize
> > +            ##
> > +
> > +            # The end sector based only on format min size should be
> > unaligned.
> > +            min_sectors = int(device.format.minSize / sector_size)
> > +            min_end_sector = partition.geometry.start + min_sectors - 1
> >
> > !!! Again there is a mix of units here or units are unspecified.
> 
> Sectors are always integers (though they could probably be any type that
> can be interpreted as an integer).

I guess partition.geometry.start is a sector number as well, in which
case all this makes sense in terms of units.

The rest is all the same.

<-- SNIP --->

> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list