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

David Lehman dlehman at redhat.com
Wed Oct 29 17:31:07 UTC 2014


On 10/29/2014 11:30 AM, Anne Mulhern wrote:
>
>
>
>
> ----- 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.

I changed sector_size and grain_size to be Size and then used int when 
using pyparted's API.

>
>>>
>>> +            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?

grain_size is the expected start sector of the first partition on any 
disk. You can't use 0 because the disklabel itself occupies a sector and 
the rest is just easier if the partition is start-aligned.

>
>>>
>>> +            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.

Done.

>>>
>>> +            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.

Right. It'd probably be worthwhile to keep a clone of pyparted handy:

   https://git.fedorahosted.org/git/pyparted.git

>
> 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
>>
> _______________________________________________
> 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