Merge pull request #510 from mwhudson/warn-silly-fs-reuse

Warn on suspicious filesystem reuse

requirements.txt

@@ -1,4 +1,4 @@
-urwid==1.2.1
+urwid==2.0.1
 nose-cov
 nose
 flake8

subiquity/ui/mount.py

@@ -44,8 +44,18 @@ def keypress(self, size, key):
 OTHER = object()
 LEAVE_UNMOUNTED = object()
 
+suitable_mountpoints_for_existing_fs = [
+    '/home',
+    '/srv',
+    OTHER,
+    LEAVE_UNMOUNTED,
+    ]
+
 
 class MountSelector(WidgetWrap):
+
+    signals = ['change']
+
     def __init__(self, mountpoints):
         opts = []
         first_opt = None
@@ -70,6 +80,16 @@ def __init__(self, mountpoints):
             # This can happen if all the common_mountpoints are in use.
             self._showhide_other(True)
 
+    def disable_unsuitable_mountpoints_for_existing_fs(self):
+        for opt in self._selector._options:
+            if opt.value is not None:
+                opt.enabled = opt.value in suitable_mountpoints_for_existing_fs
+
+    def enable_common_mountpoints(self):
+        for opt in self._selector._options:
+            if isinstance(opt.value, str):
+                opt.enabled = opt.value in common_mountpoints
+
     def _showhide_other(self, show):
         if show and not self._other_showing:
             self._w.contents.append(
@@ -84,6 +104,8 @@ def _select_mount(self, sender, value):
         self._showhide_other(value == OTHER)
         if value == OTHER:
             self._w.focus_position = 1
+            value = "/" + self._other.value
+        self._emit('change', value)
 
     @property
     def value(self):
@@ -96,10 +118,11 @@ def value(self):
 
     @value.setter
     def value(self, val):
+        opt = self._selector.option_by_value(val)
         if val is None:
             self._selector.value = LEAVE_UNMOUNTED
             self._showhide_other(False)
-        elif val in common_mountpoints:
+        elif opt is not None and opt.enabled:
             self._selector.value = val
             self._showhide_other(False)
         else:

subiquity/ui/views/filesystem/partition.py

@@ -44,7 +44,11 @@
     humanize_size,
     LVM_VolGroup,
 )
-from subiquity.ui.mount import MountField
+from subiquity.ui.mount import (
+    common_mountpoints,
+    MountField,
+    suitable_mountpoints_for_existing_fs,
+    )
 
 
 log = logging.getLogger('subiquity.ui.filesystem.add_partition')
@@ -99,16 +103,16 @@ def lost_focus(self):
         except ValueError:
             return
         if sz > self.form.max_size:
+            self.value = self.form.size_str
             self.form.size.show_extra(
                 ('info_minor',
                  _("Capped partition size at {}").format(self.form.size_str)))
-            self.value = self.form.size_str
         elif (align_up(sz) != sz and
               humanize_size(align_up(sz)) != self.form.size.value):
             sz_str = humanize_size(align_up(sz))
+            self.value = sz_str
             self.form.size.show_extra(
                 ('info_minor', _("Rounded size up to {}").format(sz_str)))
-            self.value = sz_str
 
 
 class SizeField(FormField):
@@ -164,18 +168,30 @@ def __init__(self, model, max_size, initial, lvm_names, device):
 
     def select_fstype(self, sender, fstype):
         show_use = False
+        if fstype is None and self.existing_fs_type is not None:
+            self.mount.widget.disable_unsuitable_mountpoints_for_existing_fs()
+            self.mount.value = self.mount.value
+        else:
+            self.mount.widget.enable_common_mountpoints()
+            self.mount.value = self.mount.value
         if fstype is None:
             if self.existing_fs_type == "swap":
                 show_use = True
-            fstype = self.existing_fs_type
         if self.form_pile is not None:
             for i, (w, o) in enumerate(self.form_pile.contents):
                 if w is self.mount._table and show_use:
                     self.form_pile.contents[i] = (self.use_swap._table, o)
                 elif w is self.use_swap._table and not show_use:
                     self.form_pile.contents[i] = (self.mount._table, o)
         if getattr(self.device, 'flag', None) != "boot":
-            self.mount.enabled = self.model.is_mounted_filesystem(fstype)
+            fstype_for_check = fstype
+            if fstype_for_check is None:
+                fstype_for_check = self.existing_fs_type
+            self.mount.enabled = self.model.is_mounted_filesystem(
+                fstype_for_check)
+        self.fstype.value = fstype
+        self.mount.showing_extra = False
+        self.mount.validate()
 
     name = LVNameField(_("Name: "))
     size = SizeField()
@@ -233,6 +249,15 @@ def validate_mount(self):
         if dev is not None:
             return _("{} is already mounted at {}.").format(
                 dev.label.title(), mount)
+        if self.existing_fs_type is not None:
+            if self.fstype.value is None:
+                if mount in common_mountpoints:
+                    if mount not in suitable_mountpoints_for_existing_fs:
+                        self.mount.show_extra(
+                            ('info_error',
+                             _("Mounting an existing filesystem at {} is "
+                               "usually a bad idea, proceed only with "
+                               "caution.").format(mount)))
 
     def as_rows(self):
         r = super().as_rows()

subiquity/ui/views/filesystem/tests/test_partition.py

@@ -89,6 +89,18 @@ def test_edit_partition(self):
         view.controller.partition_disk_handler.assert_called_once_with(
             stretchy.disk, stretchy.partition, expected_data)
 
+    def test_size_clamping(self):
+        model, disk = make_model_and_disk()
+        partition = model.add_partition(disk, 512*(2**20))
+        model.add_filesystem(partition, "ext4")
+        view, stretchy = make_partition_view(model, disk, partition)
+        self.assertTrue(stretchy.form.done_btn.enabled)
+        stretchy.form.size.value = "1000T"
+        stretchy.form.size.widget.lost_focus()
+        self.assertTrue(stretchy.form.size.showing_extra)
+        self.assertIn(
+            "Capped partition size", stretchy.form.size.under_text.text)
+
     def test_edit_existing_partition(self):
         form_data = {
             'fstype': "xfs",
@@ -110,6 +122,45 @@ def test_edit_existing_partition(self):
         view.controller.partition_disk_handler.assert_called_once_with(
             stretchy.disk, stretchy.partition, expected_data)
 
+    def test_edit_existing_partition_mountpoints(self):
+        # Set up a PartitionStretchy for editing a partition with an
+        # existing filesystem.
+        model, disk = make_model_and_disk()
+        partition = model.add_partition(disk, 512*(2**20))
+        partition.preserve = True
+        fs = model.add_filesystem(partition, "ext4")
+        fs.preserve = True
+        partition._original_fs = fs
+        view, stretchy = make_partition_view(model, disk, partition)
+        self.assertFalse(stretchy.form.size.enabled)
+        self.assertTrue(stretchy.form.done_btn.enabled)
+
+        # By default, the "leave formatted as xxx" option is selected.
+        self.assertIs(stretchy.form.fstype.value, None)
+        # As is "leave unmounted"
+        self.assertIs(stretchy.form.mount.value, None)
+
+        # The option for mounting at / is disabled. But /srv is still
+        # enabled.
+        selector = stretchy.form.mount.widget._selector
+        self.assertFalse(selector.option_by_value('/').enabled)
+        self.assertTrue(selector.option_by_value('/srv').enabled)
+
+        # Typing in an unsuitable mountpoint triggers a message.
+        stretchy.form.mount.value = "/boot"
+        stretchy.form.mount.validate()
+        self.assertTrue(stretchy.form.mount.showing_extra)
+        self.assertIn(
+            "bad idea", stretchy.form.mount.under_text.text)
+        self.assertIn(
+            "/boot", stretchy.form.mount.under_text.text)
+
+        # Selecting to reformat the partition clears the message and
+        # reenables the / option.
+        stretchy.form.select_fstype(None, "ext4")
+        self.assertFalse(stretchy.form.mount.showing_extra)
+        self.assertTrue(selector.option_by_value('/').enabled)
+
     def test_edit_boot_partition(self):
         form_data = {
             'size': "256M",

subiquitycore/ui/form.py

@@ -182,7 +182,7 @@ def clean(self, value):
         return value
 
     def _change(self, sender, new_val):
-        if self.in_error:
+        if self.in_error or self.showing_extra:
             self.showing_extra = False
             # the validator will likely inspect self.value to decide
             # if the new input is valid. So self.value had better

subiquitycore/ui/selector.py

@@ -79,11 +79,16 @@ class Option:
 
     def __init__(self, val):
         if not isinstance(val, tuple):
-            if not isinstance(val, str):
+            if isinstance(val, Option):
+                self.label = val.label
+                self.enabled = val.enabled
+                self.value = val.value
+            elif isinstance(val, str):
+                self.label = val
+                self.enabled = True
+                self.value = val
+            else:
                 raise SelectorError("invalid option %r", val)
-            self.label = val
-            self.enabled = True
-            self.value = val
         elif len(val) == 1:
             self.label = val[0]
             self.enabled = True
@@ -140,9 +145,7 @@ def __init__(self, opts, index=0):
 
         options = []
         for opt in opts:
-            if not isinstance(opt, Option):
-                opt = Option(opt)
-            options.append(opt)
+            options.append(Option(opt))
 
         self.options = options
         self._set_index(index)