From cda6c54b87b81edaf6b2623902eb16b6d86e5a75 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 9 Aug 2023 13:28:07 +0200 Subject: [PATCH] filesystem: store the actual size in bytes alongside the human readable size Currently, the partition form stores the size as a human readable value. (e.g., 123456K, 1.1G, 1.876G, 100G). When we exit the size field (i.e., upon losing focus), we convert the value to a number of bytes and then align it up to the nearest MiB (or whatever the alignment requirement is). Unfortunately, after computing the aligned value, we turn it back into a human-readable string and store it as is. It is not okay because the conversion does not ensure that the alignment requirement is still honored. For instance, if the user types in 1.1G, we do the following: * convert it to a number of bytes -> 1181116006.4 (surprise, it is not even an integer). * round it up to the nearest MiB -> 1181745152 (this is the correct value) * transform it into a human readable string and store it as is -> 1.1G - which actually corresponds to the original value. This leads to an exception later when creating the partition: File "subiquity/models/filesystem.py", line 1841, in add_partition raise Exception( Exception: ('size %s or offset %s not aligned to %s', 1181116006, 1048576, 1048576) Fixed by storing the actual size as a number of bytes - alongside the human readable size. Signed-off-by: Olivier Gayot --- subiquity/ui/views/filesystem/partition.py | 6 ++++ .../views/filesystem/tests/test_partition.py | 29 ++++++++++++++++++- subiquitycore/ui/form.py | 6 +++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/subiquity/ui/views/filesystem/partition.py b/subiquity/ui/views/filesystem/partition.py index 1e7cd7f0..eb0743b6 100644 --- a/subiquity/ui/views/filesystem/partition.py +++ b/subiquity/ui/views/filesystem/partition.py @@ -21,6 +21,7 @@ configuration. """ import logging import re +from typing import Optional from urwid import Text, connect_signal @@ -89,6 +90,7 @@ class FSTypeField(FormField): class SizeWidget(StringEditor): def __init__(self, form): self.form = form + self.accurate_value: Optional[int] = None super().__init__() def lost_focus(self): @@ -114,6 +116,9 @@ class SizeWidget(StringEditor): ), ) ) + # This will invoke self.form.clean_size() and it is expected that + # size_str (and therefore self.value) are propertly aligned. + self.accurate_value = self.form.size else: aligned_sz = align_up(sz, self.form.alignment) aligned_sz_str = humanize_size(aligned_sz) @@ -125,6 +130,7 @@ class SizeWidget(StringEditor): _("Rounded size up to {size}").format(size=aligned_sz_str), ) ) + self.accurate_value = aligned_sz class SizeField(FormField): diff --git a/subiquity/ui/views/filesystem/tests/test_partition.py b/subiquity/ui/views/filesystem/tests/test_partition.py index ab5363aa..8189b9a6 100644 --- a/subiquity/ui/views/filesystem/tests/test_partition.py +++ b/subiquity/ui/views/filesystem/tests/test_partition.py @@ -5,7 +5,7 @@ import urwid from subiquity.client.controllers.filesystem import FilesystemController from subiquity.common.filesystem import gaps -from subiquity.models.filesystem import dehumanize_size +from subiquity.models.filesystem import MiB, dehumanize_size from subiquity.models.tests.test_filesystem import make_model_and_disk from subiquity.ui.views.filesystem.partition import ( FormatEntireStretchy, @@ -58,6 +58,7 @@ class PartitionViewTests(unittest.TestCase): gap = gaps.Gap(device=disk, offset=1 << 20, size=99 << 30) view, stretchy = make_partition_view(model, disk, gap=gap) view_helpers.enter_data(stretchy.form, valid_data) + stretchy.form.size.widget.lost_focus() view_helpers.click(stretchy.form.done_btn.base_widget) valid_data["mount"] = "/" valid_data["size"] = dehumanize_size(valid_data["size"]) @@ -77,6 +78,7 @@ class PartitionViewTests(unittest.TestCase): view, stretchy = make_partition_view(model, disk, partition=partition) self.assertTrue(stretchy.form.done_btn.enabled) view_helpers.enter_data(stretchy.form, form_data) + stretchy.form.size.widget.lost_focus() view_helpers.click(stretchy.form.done_btn.base_widget) expected_data = { "size": dehumanize_size(form_data["size"]), @@ -111,6 +113,7 @@ class PartitionViewTests(unittest.TestCase): self.assertFalse(stretchy.form.size.enabled) self.assertTrue(stretchy.form.done_btn.enabled) view_helpers.enter_data(stretchy.form, form_data) + stretchy.form.size.widget.lost_focus() view_helpers.click(stretchy.form.done_btn.base_widget) expected_data = { "fstype": "xfs", @@ -177,6 +180,7 @@ class PartitionViewTests(unittest.TestCase): self.assertEqual(stretchy.form.mount.value, "/boot/efi") view_helpers.enter_data(stretchy.form, form_data) + stretchy.form.size.widget.lost_focus() view_helpers.click(stretchy.form.done_btn.base_widget) expected_data = { "size": dehumanize_size(form_data["size"]), @@ -246,3 +250,26 @@ class PartitionViewTests(unittest.TestCase): model._orig_config = model._render_actions() view, stretchy = make_format_entire_view(model, disk) self.assertEqual(stretchy.form.fstype.value, None) + + def test_create_partition_unaligned_size(self): + # In LP: #2013201, the user would type in 1.1G and the partition + # created would not be aligned to a MiB boundary. + unaligned_data = { + "size": "1.1G", # Corresponds to 1181116006.4 bytes (not an int) + "fstype": "ext4", + } + valid_data = { + "mount": "/", + "size": 1127 * MiB, # ~1.10058 GiB + "use_swap": False, + "fstype": "ext4", + } + model, disk = make_model_and_disk() + gap = gaps.Gap(device=disk, offset=1 << 20, size=99 << 30) + view, stretchy = make_partition_view(model, disk, gap=gap) + view_helpers.enter_data(stretchy.form, unaligned_data) + stretchy.form.size.widget.lost_focus() + view_helpers.click(stretchy.form.done_btn.base_widget) + view.controller.partition_disk_handler.assert_called_once_with( + stretchy.disk, valid_data, partition=None, gap=gap + ) diff --git a/subiquitycore/ui/form.py b/subiquitycore/ui/form.py index ea73c588..ad248dad 100644 --- a/subiquitycore/ui/form.py +++ b/subiquitycore/ui/form.py @@ -531,7 +531,11 @@ class Form(object, metaclass=MetaForm): data = {} for field in self._fields: if field.enabled: - data[field.field.name] = field.value + accurate_value = getattr(field.widget, "accurate_value", None) + if accurate_value is not None: + data[field.field.name] = accurate_value + else: + data[field.field.name] = field.value return data