diff --git a/pkg/storaged/dialog.jsx b/pkg/storaged/dialog.jsx index 3bc707515d1c..b8fafa01d148 100644 --- a/pkg/storaged/dialog.jsx +++ b/pkg/storaged/dialog.jsx @@ -1140,10 +1140,18 @@ const UsersPopover = ({ users }) => { ); }; -export const TeardownMessage = (usage) => { +function is_expected_unmount(usage, expect_single_unmount) { + return (expect_single_unmount && usage.length == 1 && + usage[0].usage == "mounted" && usage[0].location == expect_single_unmount); +} + +export const TeardownMessage = (usage, expect_single_unmount) => { if (usage.length == 0) return null; + if (is_expected_unmount(usage, expect_single_unmount)) + return ; + const rows = []; usage.forEach((use, index) => { if (use.block) { @@ -1178,7 +1186,24 @@ export const TeardownMessage = (usage) => { ); }; -export function init_active_usage_processes(client, usage) { +export function teardown_danger_message(usage, expect_single_unmount) { + if (is_expected_unmount(usage, expect_single_unmount)) + return stop_processes_danger_message(usage[0].users); + + const usage_with_users = usage.filter(u => u.users); + const n_processes = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.pid).length, 0); + const n_services = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.unit).length, 0); + if (n_processes > 0 && n_services > 0) + return _("Related processes and services will be forcefully stopped."); + else if (n_processes > 0) + return _("Related processes will be forcefully stopped."); + else if (n_services > 0) + return _("Related services will be forcefully stopped."); + else + return null; +} + +export function init_active_usage_processes(client, usage, expect_single_unmount) { return { title: _("Checking related processes"), func: dlg => { @@ -1191,22 +1216,19 @@ export function init_active_usage_processes(client, usage) { } else return Promise.resolve(); }).then(() => { - dlg.set_attribute("Teardown", TeardownMessage(usage)); - const usage_with_users = usage.filter(u => u.users); - const n_processes = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.pid).length, 0); - const n_services = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.unit).length, 0); - if (n_processes > 0 && n_services > 0) - dlg.add_danger(_("Related processes and services will be forcefully stopped.")); - else if (n_processes > 0) - dlg.add_danger(_("Related processes will be forcefully stopped.")); - else if (n_services > 0) - dlg.add_danger(_("Related services will be forcefully stopped.")); + dlg.set_attribute("Teardown", TeardownMessage(usage, expect_single_unmount)); + const msg = teardown_danger_message(usage, expect_single_unmount); + if (msg) + dlg.add_danger(msg); }); } }; } export const StopProcessesMessage = ({ mount_point, users }) => { + if (!users || users.length == 0) + return null; + const process_rows = users.filter(u => u.pid).map(u => { return { columns: [ diff --git a/pkg/storaged/fsys-tab.jsx b/pkg/storaged/fsys-tab.jsx index 46c0c1e7d1ee..bca681d78c8e 100644 --- a/pkg/storaged/fsys-tab.jsx +++ b/pkg/storaged/fsys-tab.jsx @@ -28,7 +28,7 @@ import { parse_options, unparse_options, extract_option, set_crypto_options, set import { dialog_open, TextInput, PassInput, CheckBoxes, SelectOne, - StopProcessesMessage, stop_processes_danger_message + TeardownMessage, init_active_usage_processes } from "./dialog.jsx"; import { StorageButton, StorageLink } from "./storage-controls.jsx"; import { initial_tab_options, mount_explanation } from "./format-dialog.jsx"; @@ -96,7 +96,7 @@ function nice_block_name(block) { return utils.block_name(client.blocks[block.CryptoBackingDevice] || block); } -export function is_valid_mount_point(client, block, val) { +export async function is_valid_mount_point(client, block, val, ignore_overmounting) { if (val === "") return _("Mount point cannot be empty"); @@ -104,6 +104,16 @@ export function is_valid_mount_point(client, block, val) { if (other_blocks.length > 0) return cockpit.format(_("Mount point is already used for $0"), other_blocks.map(nice_block_name).join(", ")); + + if (!ignore_overmounting) { + const children = utils.find_children_for_mount_point(client, val, block); + if (Object.keys(children).length > 0) + return <> + {_("Filesystems are already mounted below this mountpoint.")} + {Object.keys(children).map(m =>
{cockpit.format("• $0 on $1", nice_block_name(children[m]), m)}
)} + {_("Please unmount them first.")} + ; + } } export function get_cryptobacking_noauto(client, block) { @@ -179,7 +189,6 @@ export function mounting_dialog(client, block, mode, forced_options) { const extra_options = unparse_options(split_options); const is_filesystem_mounted = is_mounted(client, block); - let mount_point_users = null; function maybe_update_config(new_dir, new_opts, passphrase, passphrase_type) { let new_config = null; @@ -217,13 +226,6 @@ export function mounting_dialog(client, block, mode, forced_options) { } } - function maybe_unmount() { - if (block_fsys && block_fsys.MountPoints.indexOf(utils.encode_filename(old_dir)) >= 0) - return client.unmount_at(old_dir, mount_point_users); - else - return Promise.resolve(); - } - function get_block_fsys() { if (block_fsys) return Promise.resolve(block_fsys); @@ -293,7 +295,7 @@ export function mounting_dialog(client, block, mode, forced_options) { // backs. return (utils.reload_systemd() - .then(maybe_unmount) + .then(() => utils.teardown_active_usage(client, usage)) .then(maybe_unlock) .then(() => { if (!old_config && new_config) @@ -327,7 +329,8 @@ export function mounting_dialog(client, block, mode, forced_options) { TextInput("mount_point", _("Mount point"), { value: old_dir, - validate: val => is_valid_mount_point(client, block, val) + validate: val => is_valid_mount_point(client, block, val, + mode == "update" && !is_filesystem_mounted) }), CheckBoxes("mount_options", _("Mount options"), { @@ -375,17 +378,6 @@ export function mounting_dialog(client, block, mode, forced_options) { ]); } - let teardown = null; - if (!is_filesystem_mounted && block_fsys && block_fsys.MountPoints.length > 0) - teardown = ( - <> - {teardown} -
-

{cockpit.format(_("The filesystem is already mounted at $0. Proceeding will unmount it."), - utils.decode_filename(block_fsys.MountPoints[0]))}

-
- ); - const mode_title = { mount: _("Mount filesystem"), unmount: _("Unmount filesystem $0"), @@ -428,10 +420,12 @@ export function mounting_dialog(client, block, mode, forced_options) { return Promise.resolve(); } + const usage = utils.get_active_usage(client, block.path); + const dlg = dialog_open({ Title: cockpit.format(mode_title[mode], old_dir), Fields: fields, - Teardown: teardown, + Teardown: TeardownMessage(usage, old_dir), update: function (dlg, vals, trigger) { if (trigger == "at_boot") dlg.set_options("at_boot", { explanation: mount_explanation[vals.at_boot] }); @@ -467,19 +461,7 @@ export function mounting_dialog(client, block, mode, forced_options) { } }, Inits: [ - { - title: _("Checking related processes"), - func: dlg => { - return client.find_mount_users(old_dir, is_filesystem_mounted) - .then(users => { - mount_point_users = users; - if (users.length > 0) { - dlg.set_attribute("Teardown", ); - dlg.add_danger(stop_processes_danger_message(users)); - } - }); - } - }, + init_active_usage_processes(client, usage, old_dir), (block.IdUsage == "crypto" && mode == "mount") ? init_existing_passphrase(block, true, type => { passphrase_type = type }) : null diff --git a/pkg/storaged/utils.js b/pkg/storaged/utils.js index 9434e4ff5f3c..20971698b330 100644 --- a/pkg/storaged/utils.js +++ b/pkg/storaged/utils.js @@ -741,8 +741,34 @@ export function get_children(client, path) { return children; } +export function find_children_for_mount_point(client, mount_point, self) { + const children = {}; + + function is_self(b) { + return self && (b == self || client.blocks[b.CryptoBackingDevice] == self); + } + + for (const p in client.blocks) { + const b = client.blocks[p]; + const fs = client.blocks_fsys[p]; + + if (is_self(b)) + continue; + + if (fs) { + for (const mp of fs.MountPoints) { + const mpd = decode_filename(mp); + if (mpd.length > mount_point.length && mpd.indexOf(mount_point) == 0 && mpd[mount_point.length] == "/") + children[mpd] = b; + } + } + } + + return children; +} + export function get_active_usage(client, path, top_action, child_action) { - function get_usage(path, level) { + function get_usage(usage, path, level) { const block = client.blocks[path]; const fsys = client.blocks_fsys[path]; const mdraid = block && client.mdraids[block.MDRaidMember]; @@ -752,7 +778,7 @@ export function get_active_usage(client, path, top_action, child_action) { const stratis_blockdev = block && client.blocks_stratis_blockdev[path]; const stratis_pool = stratis_blockdev && client.stratis_pools[stratis_blockdev.Pool]; - const usage = flatten(get_children_for_teardown(client, path).map(p => get_usage(p, level + 1))); + get_children_for_teardown(client, path).map(p => get_usage(usage, p, level + 1)); function get_actions(teardown_action) { const actions = []; @@ -764,16 +790,34 @@ export function get_active_usage(client, path, top_action, child_action) { return actions; } + function enter_unmount(block, location, is_top) { + for (const u of usage) { + if (u.usage == 'mounted' && u.location == location) { + if (is_top) { + u.actions = get_actions(_("unmount")); + u.set_noauto = false; + } + return; + } + } + usage.push({ + level, + block, + usage: 'mounted', + location, + set_noauto: !is_top, + actions: is_top ? get_actions(_("unmount")) : [_("unmount")], + blocking: false + }); + } + if (fsys && fsys.MountPoints.length > 0) { fsys.MountPoints.forEach(mp => { - usage.push({ - level, - usage: 'mounted', - block, - location: decode_filename(mp), - actions: get_actions(_("unmount")), - blocking: false, - }); + const mpd = decode_filename(mp); + const children = find_children_for_mount_point(client, mpd, null); + for (const c in children) + enter_unmount(children[c], c, false); + enter_unmount(block, mpd, true); }); } else if (mdraid) { const active_state = mdraid.ActiveDevices.find(as => as[0] == block.path); @@ -828,7 +872,8 @@ export function get_active_usage(client, path, top_action, child_action) { return usage; } - let usage = get_usage(path, 0); + let usage = []; + get_usage(usage, path, 0); if (usage.length == 1 && usage[0].level == 0 && usage[0].usage == "none") usage = []; @@ -839,6 +884,37 @@ export function get_active_usage(client, path, top_action, child_action) { return usage; } +async function set_fsys_noauto(client, block, mount_point) { + for (const conf of block.Configuration) { + if (conf[0] == "fstab" && + decode_filename(conf[1].dir.v) == mount_point) { + const options = parse_options(get_block_mntopts(conf[1])); + if (options.indexOf("noauto") >= 0) + continue; + options.push("noauto"); + const new_conf = [ + "fstab", + Object.assign({ }, conf[1], + { + opts: { + t: 'ay', + v: encode_filename(unparse_options(options)) + } + }) + ]; + await block.UpdateConfigurationItem(conf, new_conf, { }); + } + } + + const crypto_backing = client.blocks[block.CryptoBackingDevice]; + if (crypto_backing) { + const crypto_backing_crypto = client.blocks_crypto[crypto_backing.path]; + await set_crypto_auto_option(crypto_backing, false); + if (crypto_backing_crypto) + await crypto_backing_crypto.Lock({}); + } +} + export function teardown_active_usage(client, usage) { // The code below is complicated by the fact that the last // physical volume of a volume group can not be removed @@ -849,10 +925,12 @@ export function teardown_active_usage(client, usage) { // physical volumes here, and it is easiest to catch this // condition upfront by reshuffling the data structures. - function unmount(mounteds) { - return Promise.all(mounteds.map(m => { - return client.unmount_at(m.location, m.users); - })); + async function unmount(mounteds) { + for (const m of mounteds) { + await client.unmount_at(m.location, m.users); + if (m.set_noauto) + await set_fsys_noauto(client, m.block, m.location); + } } function mdraid_remove(members) { diff --git a/test/reference b/test/reference index 4a17d53eb46e..a760cc815d4d 160000 --- a/test/reference +++ b/test/reference @@ -1 +1 @@ -Subproject commit 4a17d53eb46e7aec8af85c2959e90d144c4c0efe +Subproject commit a760cc815d4d51eb9364963fef10162c6d49e6ef diff --git a/test/verify/check-storage-mounting b/test/verify/check-storage-mounting index 5cf374d032bc..6e32b221c572 100755 --- a/test/verify/check-storage-mounting +++ b/test/verify/check-storage-mounting @@ -524,6 +524,101 @@ class TestStorageMountingLUKS(storagelib.StorageCase): b.click(fsys_tab + " button:contains(Do not mount automatically)") b.wait_not_present(fsys_tab + " button:contains(Do not mount automatically)") + def testOverMounting(self): + m = self.machine + b = self.browser + + self.login_and_go("/storage") + + # Add a disk and make two partitions on it, one on /run/foo + # and one on /run/foo/bar + + disk = self.add_ram_disk(100) + b.click(f'#drives .sidepanel-row:contains("{disk}")') + b.wait_visible("#storage-detail") + + b.click('button:contains(Create partition table)') + self.dialog({"type": "gpt"}) + self.content_row_wait_in_col(1, 0, "Free space") + + self.content_row_action(1, "Create partition") + self.dialog({"type": "ext4", + "size": 50, + "crypto": self.default_crypto_type, + "passphrase": "vainu-reku-toma-rolle-kaja", + "passphrase2": "vainu-reku-toma-rolle-kaja", + "mount_point": "/run/foo"}, + secondary=True) + self.content_row_wait_in_col(1, 3, "/run/foo") + + self.content_row_action(2, "Create partition", isExpandable=False) + self.dialog({"type": "ext4", + "crypto": self.default_crypto_type, + "passphrase": "vainu-reku-toma-rolle-kaja", + "passphrase2": "vainu-reku-toma-rolle-kaja", + "mount_point": "/run/foo/bar"}, + secondary=True) + self.content_row_wait_in_col(2, 3, "/run/foo/bar") + + # Mount /run/foo/bar first and check that mounting /run/foo is + # rejected + + self.content_row_action(2, "Mount") + self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) + self.wait_mounted(2, 2) + + self.content_row_action(1, "Mount") + self.dialog_wait_open() + self.dialog_set_val("passphrase", "vainu-reku-toma-rolle-kaja") + self.dialog_apply() + self.dialog_wait_error("mount_point", "Filesystems are already mounted below this mountpoint.") + b.assert_pixels("#dialog", "overmounting-rejection") + self.dialog_cancel() + self.dialog_wait_close() + + # Unmount /run/foo/bar, mount /run/foo, mount /run/foo/bar + # again and then check that unmounting /run/foo will also + # unmount /run/foo/bar. + + self.content_dropdown_action(2, "Unmount") + self.confirm() + + self.content_row_action(1, "Mount") + self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) + self.wait_mounted(1, 2) + + self.content_row_action(2, "Mount") + self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) + self.wait_mounted(2, 2) + + self.content_dropdown_action(1, "Unmount") + self.dialog_wait_open() + b.wait_in_text("#dialog tr:contains('/run/foo/bar')", "unmount") + self.dialog_apply() + self.dialog_wait_close() + + # Now /run/foo/bar should be noauto. + self.assert_in_configuration(disk + "2", "crypttab", "options", "noauto") + self.assertIn("noauto", m.execute("findmnt --fstab -n -o OPTIONS /run/foo/bar")) + + # Mount them again and check that initializing the disk will + # unmount /run/foo/bar first. + + self.content_row_action(1, "Mount") + self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) + self.wait_mounted(1, 2) + + self.content_row_action(2, "Mount") + self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) + self.wait_mounted(2, 2) + + b.click('button:contains(Create partition table)') + self.dialog_wait_open() + b.wait_text("#dialog tr:nth-child(1) td[data-label=Location]", "/run/foo/bar") + b.wait_text("#dialog tr:nth-child(2) td[data-label=Location]", "/run/foo") + self.dialog_apply() + self.content_row_wait_in_col(1, 0, "Free space") + if __name__ == '__main__': testlib.test_main()