[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