This fixes several minor problems and adds some unit tests to ensure the correct behaviors are preserved in the future.
We shouldn't be setting the target size from the size setter under any circumstances. The size reflects reality for existing devices and the planned reality for non-existent devices. Target size is only for resize of existing devices. The setter for one should not modify the other.
From: David Lehman dlehman@redhat.com
All classes should be using the same code to actually set the size. --- blivet/devices/lvm.py | 12 +++++------- blivet/devices/partition.py | 6 ++---- 2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/blivet/devices/lvm.py b/blivet/devices/lvm.py index 2e0aa80..605165e 100644 --- a/blivet/devices/lvm.py +++ b/blivet/devices/lvm.py @@ -606,13 +606,12 @@ def _setSize(self, size):
size = self.vg.align(size) log.debug("trying to set lv %s size to %s", self.name, size) - if size <= self.vg.freeSpace + self.vgSpaceUsed: - self._size = size - self.targetSize = size - else: - log.debug("failed to set size: %s short", size - (self.vg.freeSpace + self.vgSpaceUsed)) + if size > self.vg.freeSpace + self.vgSpaceUsed: + log.error("failed to set size: %s short", size - (self.vg.freeSpace + self.vgSpaceUsed)) raise ValueError("not enough free space in volume group")
+ super(LVMLogicalVolumeDevice, self)._setSize(size) + size = property(StorageDevice._getSize, _setSize)
@property @@ -1500,8 +1499,7 @@ def _setSize(self, size):
size = self.vg.align(size) size = self.vg.align(util.numeric_type(size)) - self._size = size - self.targetSize = size + super(LVMThinLogicalVolumeDevice, self)._setSize(size)
size = property(StorageDevice._getSize, _setSize)
diff --git a/blivet/devices/partition.py b/blivet/devices/partition.py index cd81363..0742643 100644 --- a/blivet/devices/partition.py +++ b/blivet/devices/partition.py @@ -714,13 +714,11 @@ def _setSize(self, newsize): if not isinstance(newsize, Size): raise ValueError("new size must of type Size")
+ super(PartitionDevice, self)._setSize(newsize) if not self.exists: - # device does not exist (a partition request), just set basic value - self._size = newsize + # also update size fields used for partition allocation self.req_size = newsize self.req_base_size = newsize - else: - super(PartitionDevice, self)._setSize(newsize)
def _getDisk(self): """ The disk that contains this partition."""
From: David Lehman dlehman@redhat.com
We need to check new sizes for non-existent devices against the limits of the device's format.
Calls to size setter for existing devices should only be to reflect the current reality. Setting up a resize should be done by setting the target size. --- blivet/devices/lvm.py | 5 ++++- blivet/devices/storage.py | 22 +++++++++++++--------- 2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/blivet/devices/lvm.py b/blivet/devices/lvm.py index 605165e..ab1108f 100644 --- a/blivet/devices/lvm.py +++ b/blivet/devices/lvm.py @@ -606,7 +606,10 @@ def _setSize(self, size):
size = self.vg.align(size) log.debug("trying to set lv %s size to %s", self.name, size) - if size > self.vg.freeSpace + self.vgSpaceUsed: + # Don't refuse to set size if we think there's not enough space in the + # VG for an existing LV, since it's existence proves there is enough + # space for it. + if not self.exists and size > self.vg.freeSpace + self.vgSpaceUsed: log.error("failed to set size: %s short", size - (self.vg.freeSpace + self.vgSpaceUsed)) raise ValueError("not enough free space in volume group")
diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py index 54d79fc..98b0f1e 100644 --- a/blivet/devices/storage.py +++ b/blivet/devices/storage.py @@ -552,15 +552,19 @@ def _setSize(self, newsize): if not isinstance(newsize, Size): raise ValueError("new size must of type Size")
- # only calculate these once - max_size = self.maxSize - min_size = self.minSize - if max_size and newsize > max_size: - raise errors.DeviceError("device cannot be larger than %s" % - max_size, self.name) - elif min_size and newsize < min_size: - raise errors.DeviceError("device cannot be smaller than %s" % - min_size, self.name) + # There's no point in checking limits here for existing devices since + # the only way to change their size is by setting target size. Any call + # to this setter for an existing device should be to reflect existing + # state. + if not self.exists: + max_size = self.format.maxSize + min_size = self.format.minSize + if max_size and newsize > max_size: + raise errors.DeviceError("device cannot be larger than %s" % + max_size, self.name) + elif min_size and newsize < min_size: + raise errors.DeviceError("device cannot be smaller than %s" % + min_size, self.name)
self._size = newsize
From: David Lehman dlehman@redhat.com
This prevents, eg: assigning PReP boot format to a device much larger than the maximum size allowed for that format. --- blivet/devices/storage.py | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py index 98b0f1e..d90b3f5 100644 --- a/blivet/devices/storage.py +++ b/blivet/devices/storage.py @@ -686,6 +686,12 @@ def _setFormat(self, fmt): # FIXME: self.format.status doesn't mean much raise errors.DeviceError("cannot replace active format", self.name)
+ # check device size against format limits + if fmt.maxSize and fmt.maxSize < self.size: + raise errors.DeviceError("device is too large for new format") + elif fmt.minSize and fmt.minSize > self.size: + raise errors.DeviceError("device is too small for new format") + self._format = fmt self._format.device = self.path self._updateNetDevMountOption()
This is okay, but our PReP boot format size limit is wrong -- it can be arbitrarily big, the FW will use a small part of it, but that's not a hard limit. We need to do this check only for unexisting devices and probably fix some checks so that they do not raise exceptions if there's a bigger format than what we consider maximum size for such format.
I will change it to only check on non-existent formats. As far as bogus max sizes, there's nothing to do there except fix the limits. We already enforce these checks everywhere else -- this is just a back door that was left open.
From: David Lehman dlehman@redhat.com
This allows setting targetSize to zero to cancel a resize, which is nicer than having to set it to the current size. --- blivet/devices/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py index d90b3f5..62a8479 100644 --- a/blivet/devices/storage.py +++ b/blivet/devices/storage.py @@ -538,7 +538,7 @@ def addHook(self, new=True): def _getSize(self): """ Get the device's size, accounting for pending changes. """ size = self._size - if self.exists and self.resizable: + if self.exists and self.resizable and self.targetSize != Size(0): size = self.targetSize
return size
From: David Lehman dlehman@redhat.com
--- tests/devices_test/size_test.py | 99 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 tests/devices_test/size_test.py
diff --git a/tests/devices_test/size_test.py b/tests/devices_test/size_test.py new file mode 100644 index 0000000..bb664bb --- /dev/null +++ b/tests/devices_test/size_test.py @@ -0,0 +1,99 @@ + +import unittest + +from blivet.devices import StorageDevice +from blivet import errors +from blivet.formats import getFormat +from blivet.size import Size + +class StorageDeviceSizeTest(unittest.TestCase): + def _getDevice(self, *args, **kwargs): + return StorageDevice(*args, **kwargs) + + def testSizeSetter(self): + initial_size = Size('10 GiB') + new_size = Size('2 GiB') + + ## + ## setter sets the size + ## + dev = self._getDevice('sizetest', size=initial_size) + self.assertEqual(dev.size, initial_size) + + dev.size = new_size + self.assertEqual(dev.size, new_size) + + ## + ## setter raises exn if size outside of format limits + ## + dev.format._maxSize = Size("5 GiB") + with self.assertRaises(errors.DeviceError): + dev.size = Size("6 GiB") + + ## + ## new formats' min size is checked against device size + ## + fmt = getFormat(None) + fmt._minSize = Size("10 GiB") + with self.assertRaises(errors.DeviceError): + dev.format = fmt + + # the format assignment should succeed without the min size conflict + fmt._minSize = Size(0) + dev.format = fmt + + ## + ## new formats' max size is checked against device size + ## + fmt = getFormat(None) + fmt._maxSize = Size("10 MiB") + with self.assertRaises(errors.DeviceError): + dev.format = fmt + + # the format assignment should succeed without the min size conflict + fmt._maxSize = Size(0) + dev.format = fmt + + def testSizeGetter(self): + initial_size = Size("10 GiB") + new_size = Size("5 GiB") + dev = self._getDevice('sizetest', size=initial_size) + + ## + ## getter returns the size in the basic case for non-existing devices + ## + self.assertEqual(dev.size, initial_size) + + # create a new device that exists + dev = self._getDevice('sizetest', size=initial_size, exists=True) + + ## + ## getter returns the size in the basic case for existing devices + ## + self.assertEqual(dev.size, initial_size) + + ## + ## size does not reflect target size for non-resizable devices + ## + # bypass the setter since the min/max will be the current size for a + # non-resizable device + dev._targetSize = new_size + self.assertEqual(dev.size, initial_size) + + ## + ## getter returns target size when device is resizable and target size + ## is non-zero + ## + dev._resizable = True + dev.targetSize = new_size # verify that the target size setter works + self.assertEqual(dev.size, new_size) + self.assertEqual(dev.size, dev.targetSize) + self.assertNotEqual(dev._size, dev.targetSize) + + ## + ## getter returns current size when device is resizable and target size + ## is zero + ## + dev.targetSize = Size(0) + self.assertEqual(dev.size, initial_size) + self.assertEqual(dev.size, dev.currentSize)
Apart from the comment above this looks good to me. It's definitely an improvement.
Added label: ACK.
anaconda-patches@lists.fedorahosted.org
-
dashea