[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