Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storage: Confirmation before erasing non-empty filesystems #20272

Merged
merged 1 commit into from
May 30, 2024
Merged

storage: Confirmation before erasing non-empty filesystems #20272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/storaged/block/format-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
dialog_open,
TextInput, PassInput, CheckBoxes, SelectOne, SizeSlider,
BlockingMessage, TeardownMessage,
init_active_usage_processes
init_teardown_usage
} from "../dialog.jsx";

import { get_fstab_config, is_valid_mount_point } from "../filesystem/utils.jsx";
Expand Down Expand Up @@ -630,7 +630,7 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended,
}
},
Inits: [
init_active_usage_processes(client, usage),
init_teardown_usage(client, usage),
unlock_before_format
? init_existing_passphrase(block, true, type => { existing_passphrase_type = type })
: null
Expand Down
6 changes: 3 additions & 3 deletions pkg/storaged/block/resize.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
} from "../crypto/keyslots.jsx";
import {
dialog_open, SizeSlider, BlockingMessage, TeardownMessage, SelectSpaces,
init_active_usage_processes
init_teardown_usage
} from "../dialog.jsx";
import { std_reply } from "../stratis/utils.jsx";
import { pvs_to_spaces } from "../lvm2/utils.jsx";
Expand Down Expand Up @@ -534,7 +534,7 @@ export function grow_dialog(client, lvol_or_part, info, to_fit) {
}
},
Inits: [
init_active_usage_processes(client, usage),
init_teardown_usage(client, usage),
passphrase_fields.length
? init_existing_passphrase(block, false, pp => { recovered_passphrase = pp })
: null
Expand Down Expand Up @@ -647,7 +647,7 @@ export function shrink_dialog(client, lvol_or_part, info, to_fit) {
}
},
Inits: [
init_active_usage_processes(client, usage),
init_teardown_usage(client, usage),
passphrase_fields.length
? init_existing_passphrase(block, false, pp => { recovered_passphrase = pp })
: null
Expand Down
5 changes: 3 additions & 2 deletions pkg/storaged/btrfs/subvolume.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { btrfs_usage, validate_subvolume_name, parse_subvol_from_options } from
import { at_boot_input, update_at_boot_input, mounting_dialog, mount_options } from "../filesystem/mounting-dialog.jsx";
import {
dialog_open, TextInput,
TeardownMessage, init_active_usage_processes,
TeardownMessage, init_teardown_usage,
} from "../dialog.jsx";
import { check_mismounted_fsys, MismountAlert } from "../filesystem/mismounting.jsx";
import {
Expand Down Expand Up @@ -204,6 +204,7 @@ function subvolume_delete(volume, subvol, mount_point_in_parent, card) {
const paths_to_delete = [];
const usage = [];

usage.Teardown = true;
for (const sv of all_subvols) {
const [config, mount_point] = get_fstab_config_with_client(client, block, false, sv);
const fs_is_mounted = is_mounted(client, block, sv);
Expand Down Expand Up @@ -241,7 +242,7 @@ function subvolume_delete(volume, subvol, mount_point_in_parent, card) {
}
},
Inits: [
init_active_usage_processes(client, usage)
init_teardown_usage(client, usage)
]
});
}
Expand Down
150 changes: 125 additions & 25 deletions pkg/storaged/dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,21 @@ import { HelperText, HelperTextItem } from "@patternfly/react-core/dist/esm/comp
import { List, ListItem } from "@patternfly/react-core/dist/esm/components/List/index.js";
import { ExclamationTriangleIcon, InfoIcon, HelpIcon, EyeIcon, EyeSlashIcon } from "@patternfly/react-icons";
import { InputGroup } from "@patternfly/react-core/dist/esm/components/InputGroup/index.js";
import { Table, Tbody, Tr, Td } from '@patternfly/react-table';

import { show_modal_dialog, apply_modal_dialog } from "cockpit-components-dialog.jsx";
import { ListingTable } from "cockpit-components-table.jsx";
import { FormHelper } from "cockpit-components-form-helper";

import { fmt_size, block_name, format_size_and_text, format_delay, for_each_async, get_byte_units } from "./utils.js";
import {
decode_filename, fmt_size, block_name, format_size_and_text, format_delay, for_each_async, get_byte_units,
is_available_block
} from "./utils.js";
import { fmt_to_fragments } from "utils.jsx";
import client from "./client.js";

import fsys_is_empty_sh from "./fsys-is-empty.sh";

const _ = cockpit.gettext;

function make_rows(fields, values, errors, onChange) {
Expand Down Expand Up @@ -328,6 +334,19 @@ const Body = ({ body, teardown, fields, values, errors, isFormHorizontal, onChan
);
};

const ExtraConfirmation = ({ text, onChange }) => {
const [confirmed, setConfirmed] = useState(false);

return (
<Checkbox isChecked={confirmed}
id="dialog-confirm"
label={text}
onChange={(_, val) => {
setConfirmed(val);
onChange(val);
}} />);
};

function flatten_fields(fields) {
return fields.reduce(
(acc, val) => acc.concat([val]).concat(val.options && val.options.nested_fields
Expand All @@ -340,6 +359,8 @@ export const dialog_open = (def) => {
const nested_fields = def.Fields || [];
const fields = flatten_fields(nested_fields);
const values = { };
let confirmation = null;
let confirmed = false;
let errors = null;

fields.forEach(f => { values[f.tag] = f.initial_value });
Expand Down Expand Up @@ -415,8 +436,10 @@ export const dialog_open = (def) => {
caption: variant.Title,
style: actions.length == 0 ? "primary" : "secondary",
danger: def.Action.Danger || def.Action.DangerButton,
disabled: running_promise != null || (def.Action.disable_on_error &&
errors && errors.toString() != "[object Object]"),
disabled: (running_promise != null ||
(def.Action.disable_on_error &&
errors && errors.toString() != "[object Object]") ||
(confirmation && !confirmed)),
clicked: progress_callback => run_action(progress_callback, variant.tag),
});
}
Expand All @@ -436,13 +459,19 @@ export const dialog_open = (def) => {
}
}

const extra = (
<div>
{ def.Action && def.Action.Danger
? <HelperText><HelperTextItem variant="error">{def.Action.Danger} </HelperTextItem></HelperText>
: null
}
</div>);
let extra = null;
if (confirmation) {
extra = <ExtraConfirmation text={confirmation}
onChange={val => {
confirmed = val;
update_footer();
}} />;
} else if (def.Action && def.Action.Danger) {
extra = (
<div>
<HelperText><HelperTextItem variant="error">{def.Action.Danger} </HelperTextItem></HelperText>
</div>);
}

return {
idle_message: (running_promise
Expand Down Expand Up @@ -537,6 +566,14 @@ export const dialog_open = (def) => {
update();
},

need_confirmation: (conf) => {
confirmation = conf;
confirmed = false;
def.Action.Danger = null;
def.Action.DangerButton = true;
update_footer();
},

close: () => {
dlg.footerProps.dialog_done();
}
Expand Down Expand Up @@ -1204,13 +1241,16 @@ const teardown_block_name = use => {
name = block_name(client.blocks[use.block.CryptoBackingDevice] || use.block);
}

return name;
return name.replace(/^\/dev\//, "");
};

export const TeardownMessage = (usage, expect_single_unmount) => {
if (usage.length == 0)
if (!usage.Teardown)
return null;

if (client.in_anaconda_mode() && !expect_single_unmount)
return <AnacondaTeardownMessage usage={usage} />;

if (is_expected_unmount(usage, expect_single_unmount))
return <StopProcessesMessage mount_point={expect_single_unmount} users={usage[0].users} />;

Expand Down Expand Up @@ -1251,6 +1291,36 @@ export const TeardownMessage = (usage, expect_single_unmount) => {
</div>);
};

const AnacondaTeardownMessage = ({ usage }) => {
const rows = [];

usage.forEach((use, index) => {
if (use.data_warning) {
const name = teardown_block_name(use);
const location = client.strip_mount_point_prefix(use.location) || use.block.IdLabel || "-";

rows.push(
<Tr key={index}>
<Td className="pf-v5-u-font-weight-bold">{name}</Td>
<Td>{location}</Td>
<Td>{use.data_warning}</Td>
</Tr>);
}
});

if (rows.length > 0) {
return (
<div className="modal-footer-teardown">
<HelperText>
<HelperTextItem variant="error">
{_("Important data might be deleted:")}
</HelperTextItem>
</HelperText>
<Table variant="compact" borders={false}><Tbody>{rows}</Tbody></Table>
</div>);
}
};

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);
Expand All @@ -1269,24 +1339,54 @@ export function teardown_danger_message(usage, expect_single_unmount) {
}
}

export function init_active_usage_processes(client, usage, expect_single_unmount) {
export function init_teardown_usage(client, usage, expect_single_unmount) {
return {
title: _("Checking related processes"),
func: dlg => {
return for_each_async(usage, u => {
title: _("Checking filesystem usage"),
func: async function (dlg) {
let have_data = false;
for (const u of usage) {
if (u.usage == "mounted") {
return client.find_mount_users(u.location)
.then(users => {
u.users = users;
});
} else
return Promise.resolve();
}).then(() => {
dlg.set_attribute("Teardown", TeardownMessage(usage, expect_single_unmount));
u.users = await client.find_mount_users(u.location);
}
if (client.in_anaconda_mode() && !expect_single_unmount && u.block) {
if (u.block.IdUsage == "filesystem" &&
["xfs", "ext2", "ext3", "ext4", "btrfs", "vfat", "ntfs"].indexOf(u.block.IdType) >= 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these all the filesystems udisks supports? If so Cockpit should already have a list of them somewhere right? Or can't we get this from udisks? Like http://storaged.org/doc/udisks2-api/latest/gdbus-org.freedesktop.UDisks2.Manager.html#gdbus-property-org-freedesktop-UDisks2-Manager.SupportedFilesystems

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these all the filesystems udisks supports?

No, the idea is to only mount a small number of well-known filesystems, instead of just throwing everything that we find at the kernel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! That sounds a bit weird:

[jelle@t14s][~/projects/cockpit-project.github.io]%busctl get-property org.freedesktop.UDisks2 /org/freedesktop/UDisks2/Manager org.freedesktop.UDisks2.Manager SupportedFilesystems

as 12 "ext2" "ext3" "ext4" "xfs" "vfat" "ntfs" "f2fs" "nilfs2" "exfat" "btrfs" "udf" "swap"

This seems almost what we want imo.

const empty = await cockpit.script(fsys_is_empty_sh,
[decode_filename(u.block.PreferredDevice)],
{ superuser: "require", err: "message" });
if (empty.trim() != "yes") {
try {
const info = JSON.parse(empty);
u.data_warning = cockpit.format(_("$0 used, $1 total"),
fmt_size((info.total - info.free) * info.unit),
fmt_size(info.total * info.unit));
} catch {
u.data_warning = _("Device contains unrecognized data");
Comment on lines +1363 to +1364
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test.

}
}
} else if (u.block.IdUsage == "crypto" && !client.blocks_cleartext[u.block.path]) {
u.data_warning = _("Locked encrypted device might contain data");
} else if (!client.blocks_ptable[u.block.path] &&
u.block.IdUsage != "raid" &&
!is_available_block(client, u.block)) {
u.data_warning = _("Device contains unrecognized data");
}
if (u.data_warning)
have_data = true;
}
}

if (have_data) {
usage.Teardown = true;
dlg.need_confirmation(_("I confirm I want to lose this data forever"));
mvollmer marked this conversation as resolved.
Show resolved Hide resolved
} else if (client.in_anaconda_mode() && !expect_single_unmount) {
dlg.need_confirmation(null);
} else {
const msg = teardown_danger_message(usage, expect_single_unmount);
if (msg)
dlg.add_danger(msg);
});
}
dlg.set_attribute("Teardown", TeardownMessage(usage, expect_single_unmount));
}
};
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/storaged/filesystem/mounting-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
dialog_open,
TextInput, PassInput, CheckBoxes, SelectOne,
TeardownMessage,
init_active_usage_processes
init_teardown_usage
} from "../dialog.jsx";
import { init_existing_passphrase, unlock_with_type } from "../crypto/keyslots.jsx";
import { initial_tab_options } from "../block/format-dialog.jsx";
Expand Down Expand Up @@ -404,7 +404,7 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) {
const dlg = dialog_open({
Title: cockpit.format(mode_title[mode], old_dir_for_display),
Fields: fields,
Teardown: TeardownMessage(usage, old_dir),
Teardown: TeardownMessage(usage, old_dir || true),
update: function (dlg, vals, trigger) {
update_at_boot_input(dlg, vals, trigger);
if (trigger == "mount_options")
Expand Down Expand Up @@ -447,7 +447,7 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) {
}
},
Inits: [
init_active_usage_processes(client, usage, old_dir),
init_teardown_usage(client, usage, old_dir || true),
init_existing_passphrase(block, true, type => {
passphrase_type = type;
update_explicit_passphrase(dlg.get_value("mount_options")?.ro ?? opt_ro);
Expand Down
34 changes: 34 additions & 0 deletions pkg/storaged/fsys-is-empty.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#! /bin/bash

set -eux

dev=$1

need_unmount=""

maybe_unmount () {
if [ -n "$need_unmount" ]; then
umount "$need_unmount"
rmdir "$need_unmount"
fi
}

trap maybe_unmount EXIT INT QUIT

mp=$(findmnt -no TARGET "$dev" | cat)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That cat seems unnecessary:

[jelle@t14s][~/projects/cockpit/main]%x=$(findmnt -no TARGET /dev/nvme0n1p1)
[jelle@t14s][~/projects/cockpit/main]%echo $x
/boot

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It protects against the exit code of findmnt. When $dev is not mounted, findmnt exits with code 1, which would stop the script because of set -e. In a pipeline, the last sub-command determines the exit code of the while pipeline, and cat always exits with 0.

if [ -z "$mp" ]; then
mp=$(mktemp -d)
need_unmount=$mp
mount "$dev" "$mp" -o ro
fi

# A filesystem is empty if it only has directories in it.

first=$(find "$mp" -not -type d | head -1)
info=$(stat -f -c '{ "unit": %S, "free": %f, "total": %b }' "$mp")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of these commands is allowed to fail as then we will exit and never unmount if needed. So maybe we should use a trap for that.

(I can only get find to fail when not running as root for example but maybe it can fail on broken symlinks or pipes as root?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe we should use a trap for that.

Good point!


if [ -z "$first" ]; then
echo yes
else
echo "$info"
fi
6 changes: 3 additions & 3 deletions pkg/storaged/legacy-vdo/legacy-vdo.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { DescriptionList, DescriptionListDescription, DescriptionListGroup, Desc

import { block_short_name, get_active_usage, teardown_active_usage, fmt_size, decode_filename, reload_systemd } from "../utils.js";
import {
dialog_open, SizeSlider, BlockingMessage, TeardownMessage, init_active_usage_processes
dialog_open, SizeSlider, BlockingMessage, TeardownMessage, init_teardown_usage
} from "../dialog.jsx";
import { StorageButton, StorageOnOff } from "../storage-controls.jsx";

Expand Down Expand Up @@ -68,7 +68,7 @@ export function make_legacy_vdo_page(parent, vdo, backing_block, next_card) {
}
},
Inits: [
init_active_usage_processes(client, usage)
init_teardown_usage(client, usage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

]
});
} else {
Expand Down Expand Up @@ -130,7 +130,7 @@ export function make_legacy_vdo_page(parent, vdo, backing_block, next_card) {
}
},
Inits: [
init_active_usage_processes(client, usage)
init_teardown_usage(client, usage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

]
});
}
Expand Down
Loading
Loading