Skip to content

storage: Detect partition tables in unsupported places #22039

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

Merged
merged 2 commits into from
Jun 3, 2025
Merged

storage: Detect partition tables in unsupported places #22039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4405e8d
storage: Collect "available spaces" during page creation
mvollmer Jun 2, 2025
6e10240
storage: Detect partition tables in unsupported places
mvollmer May 22, 2025
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
9 changes: 6 additions & 3 deletions pkg/storaged/block/create-pages.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ import { make_btrfs_filesystem_card } from "../btrfs/filesystem.jsx";
import { make_btrfs_subvolume_pages } from "../btrfs/subvolume.jsx";

import { new_page } from "../pages.jsx";
import { register_available_block_space } from "../utils";

/* CARD must have page_name, page_location, and page_size set.
*/

export function make_block_page(parent, block, card) {
export function make_block_page(parent, block, card, options) {
let is_crypto = block.IdUsage == 'crypto';
let content_block = is_crypto ? client.blocks_cleartext[block.path] : block;
const fstab_config = get_fstab_config(content_block || block, true);
Expand All @@ -62,7 +63,7 @@ export function make_block_page(parent, block, card) {
const single_device_volume = block_btrfs_blockdev && block_btrfs_blockdev.data.num_devices === 1;

if (client.blocks_ptable[block.path]) {
make_partition_table_page(parent, block, card);
make_partition_table_page(parent, block, card, options && options.partitionable);
return;
}

Expand Down Expand Up @@ -113,7 +114,9 @@ export function make_block_page(parent, block, card) {
} else if (block_swap ||
(content_block.IdUsage == "other" && content_block.IdType == "swap")) {
card = make_swap_card(card, block, content_block);
} else if (client.blocks_available[content_block.path]) {
} else if (!content_block.IdUsage) {
if (!block.HintIgnore)
register_available_block_space(client, content_block);
Comment on lines -116 to +119
Copy link
Member

Choose a reason for hiding this comment

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

(So this is coming in as someone who doesn't know much at all about storaged)

Is the comparison here then changing from multiple checks to just checking if IdUsage is undefined? Do we not need to care about stuff like swap, mpath member, fs label, ic container, etc.?

Also from what you said and what I see this is all going from creating the available blocks on load to page creation then right? Will this make it easier or more difficult if we decide to implement another filesystem?

I see this from storaged docs that say:

If blank, no known signature was detected. This doesn't necessarily mean the device contains no structured data; it only means that no signature known to the probing code was detected.

Applications should not rely on the value in this or the "IdType" property - instead, applications should check for whether the object in question implements interfaces such as e.g. org.freedesktop.UDisks2.Filesystem, org.freedesktop.UDisks2.Swapspace or org.freedesktop.UDisks2.Encrypted.

Wouldn't that mean that we could technically show this as an available space even if it isn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the comparison here then changing from multiple checks to just checking if IdUsage is undefined? Do we not need to care about stuff like swap, mpath member, fs label, ic container, etc.?

The rest of the big create_pages function cares about swap, mpath, partitions, etc. When it comes to this place in its if-else-if chain, it only needs to distinguish between "empty" and "unrecognized".

The old is_available_block function would have to recreate the same logic as create_pages and arrive at the same conclusions. This was tedious to keep synchronized and the improvement here is that now there is only one place with the complex logic.

Also from what you said and what I see this is all going from creating the available blocks on load to page creation then right? Will this make it easier or more difficult if we decide to implement another filesystem?

It should make it easier.

I see this from storaged docs that say:

If blank, no known signature was detected. This doesn't necessarily mean the device contains no structured data; it only means that no signature known to the probing code was detected.

Applications should not rely on the value in this or the "IdType" property - instead, applications should check for whether the object in question implements interfaces such as e.g. org.freedesktop.UDisks2.Filesystem, org.freedesktop.UDisks2.Swapspace or org.freedesktop.UDisks2.Encrypted.

Oh, interesting! We do in fact check IdUsage == "filesystem" instead of checking whether there is a Filesystem interface... We could change that, and now it's much easier since there is only one place. :-)

Wouldn't that mean that we could technically show this as an available space even if it isn't?

Yes, true, if UDisks2 somehow has a better idea of what's on a block device than udev. That would be an unwelcome inconsistency, I'd say, but as you found out, it's clearly documented.

I'll go over the code and try to improve it.

(We are really digging into the oldest pieces of the Cockpit Storage code here... :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go over the code and try to improve it.

In a separate PR. We would in any case still check IdUsage to distinguish between "unrecognized" and "empty". The improvements would come from the rest of the code recognizing more things so that we don't reach this point in the code as often.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

card = make_unformatted_data_card(card, block, content_block);
} else {
card = make_unrecognized_data_card(card, block, content_block);
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/block/other.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function make_other_page(parent, block) {
actions: partitionable_block_actions(block),
});

make_block_page(parent, block, other_card);
make_block_page(parent, block, other_card, { partitionable: true });
}

const OtherCard = ({ card, block }) => {
Expand Down
4 changes: 2 additions & 2 deletions pkg/storaged/block/resize.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function lvol_or_part_and_fsys_resize(client, lvol_or_part, size, offline, passp
return Promise.reject(_("Stratis blockdevs can not be made smaller")); // not-covered: safety check
else
return Promise.resolve();
} else if (client.blocks_available[block.path]) {
} else if (!block.IdUsage) {
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.

// Growing or shrinking unformatted data, nothing to do
return Promise.resolve();
} else if (size < orig_size) {
Expand Down Expand Up @@ -293,7 +293,7 @@ export function get_resize_info(client, block, to_fit) {
can_grow: false,
};
shrink_excuse = grow_excuse = _("Swap can not be resized here");
} else if (client.blocks_available[block.path]) {
} else if (!block.IdUsage) {
info = {
can_shrink: true,
can_grow: true,
Expand Down
7 changes: 0 additions & 7 deletions pkg/storaged/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,13 +825,6 @@ function update_indices() {
}
}

client.blocks_available = { };
for (path in client.blocks) {
block = client.blocks[path];
if (utils.is_available_block(client, block))
client.blocks_available[path] = true;
}

client.path_jobs = { };
function enter_job(job) {
if (!job.Objects || !job.Objects.length)
Expand Down
5 changes: 2 additions & 3 deletions pkg/storaged/dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ import { FormHelper } from "cockpit-components-form-helper";

import {
decode_filename, fmt_size, block_name, format_size_and_text, format_delay, for_each_async, get_byte_units,
is_available_block, BTRFS_TOOL_MOUNT_PATH
BTRFS_TOOL_MOUNT_PATH
} from "./utils.js";
import { fmt_to_fragments } from "utils.jsx";
import client from "./client.js";
Expand Down Expand Up @@ -1391,8 +1391,7 @@ export function init_teardown_usage(client, usage, expect_single_unmount) {
} 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.block.IdUsage && u.block.IdUsage != "raid") {
u.data_warning = _("Device contains unrecognized data");
}
if (u.data_warning)
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/drive/drive.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export function make_drive_page(parent, drive) {
}

if (block.Size > 0) {
make_block_page(parent, block, card);
make_block_page(parent, block, card, { partitionable: true });
} else {
new_page(parent, card);
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/lvm2/create-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function create_vgroup() {
if (disks.length === 0)
return _("At least one disk is needed.");
},
spaces: get_available_spaces(client)
spaces: get_available_spaces()
})
],
Action: {
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/lvm2/volume-group.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function add_disk(vgroup) {
if (disks.length === 0)
return _("At least one disk is needed.");
},
spaces: get_available_spaces(client).filter(filter_inside_vgroup)
spaces: get_available_spaces().filter(filter_inside_vgroup)
})
],
Action: {
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/mdraid/create-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export function create_mdraid() {
return cockpit.format(cockpit.ngettext("At least $0 disk is needed.", "At least $0 disks are needed.", disks_needed),
disks_needed);
},
spaces: get_available_spaces(client)
spaces: get_available_spaces()
})
],
Action: {
Expand Down
4 changes: 2 additions & 2 deletions pkg/storaged/mdraid/mdraid.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ function add_disk(mdraid) {
if (disks.length === 0)
return _("At least one disk is needed.");
},
spaces: get_available_spaces(client).filter(filter_inside_mdraid)
spaces: get_available_spaces().filter(filter_inside_mdraid)
})
],
Action: {
Expand Down Expand Up @@ -247,7 +247,7 @@ export function make_mdraid_page(parent, mdraid) {
if (!block) {
new_page(parent, mdraid_card);
} else
make_block_page(parent, block, mdraid_card);
make_block_page(parent, block, mdraid_card, { partitionable: true });
}

const MDRaidCard = ({ card, mdraid, block }) => {
Expand Down
6 changes: 5 additions & 1 deletion pkg/storaged/pages.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ import { Spinner } from "@patternfly/react-core/dist/esm/components/Spinner/inde
import { DescriptionListDescription, DescriptionListGroup, DescriptionListTerm } from "@patternfly/react-core/dist/esm/components/DescriptionList/index.js";
import { Flex, FlexItem } from "@patternfly/react-core/dist/esm/layouts/Flex/index.js";

import { decode_filename, block_short_name, fmt_size } from "./utils.js";
import {
decode_filename, block_short_name, fmt_size,
reset_available_spaces
} from "./utils.js";
import { StorageButton, StorageBarMenu, StorageMenuItem, StorageSize } from "./storage-controls.jsx";
import { MultipathAlert } from "./multipath.jsx";
import { JobsPanel } from "./jobs-panel.jsx";
Expand Down Expand Up @@ -142,6 +145,7 @@ export const PAGE_CATEGORY_NETWORK = 3;
export function reset_pages() {
pages = new Map();
crossrefs = new Map();
reset_available_spaces();
}

function name_from_card(card) {
Expand Down
29 changes: 23 additions & 6 deletions pkg/storaged/partitions/partition-table.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import cockpit from "cockpit";
import React from "react";
import client from "../client";

import { get_partitions } from "../utils.js";
import { Alert } from "@patternfly/react-core/dist/esm/components/Alert/index.js";
import { CardBody } from "@patternfly/react-core/dist/esm/components/Card/index.js";

import { get_partitions, register_available_free_space } from "../utils.js";
import { StorageCard, ChildrenTable, new_page, new_card } from "../pages.jsx";
import { format_dialog } from "../block/format-dialog.jsx";
import { make_block_page } from "../block/create-pages.jsx";
Expand Down Expand Up @@ -68,9 +71,10 @@ function make_partition_pages(parent, block) {
let p;
for (i = 0; i < partitions.length; i++) {
p = partitions[i];
if (p.type == 'free')
if (p.type == 'free') {
register_available_free_space(client, block, p);
make_free_space_page(parent, p.start, p.size, enable_dos_extended);
else if (p.type == 'container')
} else if (p.type == 'container')
make_extended_partition_page(parent, p);
else {
const card = make_partition_card(null, p.block);
Expand All @@ -83,20 +87,21 @@ function make_partition_pages(parent, block) {
block_ptable.Type == 'dos');
}

export function make_partition_table_page(parent, block, next_card) {
export function make_partition_table_page(parent, block, next_card, partitionable) {
const block_ptable = client.blocks_ptable[block.path];

const parts_card = new_card({
title: (block_ptable.Type
? cockpit.format(_("$0 partitions"), block_ptable.Type.toLocaleUpperCase())
: _("Partitions")),
next: next_card,
component: PartitionsCard,
component: partitionable ? PartitionsCard : UnexpectedPartitionsCard,
props: { },
});

const p = new_page(parent, parts_card, { sorted: false });
make_partition_pages(p, block);
if (partitionable)
make_partition_pages(p, block);
}

const PartitionsCard = ({ card }) => {
Expand All @@ -109,3 +114,15 @@ const PartitionsCard = ({ card }) => {
</StorageCard>
);
};

const UnexpectedPartitionsCard = ({ card }) => {
return (
<StorageCard card={card}>
<CardBody>
<Alert isInline isPlain variant="info" title={_("Unexpected partitions")}>
<p>{_("Partitions are not supported on this block device. If it is used as a disk for a virtual machine, the partitions must be managed by the operating system inside the virtual machine.")}</p>
</Alert>
</CardBody>
</StorageCard>
);
};
2 changes: 1 addition & 1 deletion pkg/storaged/stratis/create-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function create_stratis_pool() {
if (disks.length === 0)
return _("At least one block device is needed.");
},
spaces: get_available_spaces(client)
spaces: get_available_spaces()
}),
CheckBoxes("encrypt_pass", _("Options"),
{
Expand Down
2 changes: 1 addition & 1 deletion pkg/storaged/stratis/pool.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ function add_disks(pool) {
if (disks.length === 0)
return _("At least one disk is needed.");
},
spaces: get_available_spaces(client)
spaces: get_available_spaces()
})
],
Action: {
Expand Down
103 changes: 21 additions & 82 deletions pkg/storaged/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,93 +462,32 @@ export function get_partitions(client, block) {
return process_level(0, 0, block.Size);
}

export function is_available_block(client, block, honor_ignore_hint) {
const block_ptable = client.blocks_ptable[block.path];
const block_part = client.blocks_part[block.path];
const block_pvol = client.blocks_pvol[block.path];
let available_spaces = [];

function has_fs_label() {
if (!block.IdUsage)
return false;
// Devices with a LVM2_member label need to actually be
// associated with a volume group.
if (block.IdType == 'LVM2_member' && (!block_pvol || !client.vgroups[block_pvol.VolumeGroup]))
return false;
return true;
}

function is_mpath_member() {
if (!client.drives[block.Drive])
return false;
if (!client.drives_block[block.Drive]) {
// Broken multipath drive
return true;
}
const members = client.drives_multipath_blocks[block.Drive];
for (let i = 0; i < members.length; i++) {
if (members[i] == block)
return true;
}
return false;
}

function is_vdo_backing_dev() {
return !!client.legacy_vdo_overlay.find_by_backing_block(block);
}

function is_swap() {
return !!block && client.blocks_swap[block.path];
}

return (!(block.HintIgnore && honor_ignore_hint) &&
block.Size > 0 &&
!has_fs_label() &&
!is_mpath_member() &&
!is_vdo_backing_dev() &&
!is_swap() &&
!block_ptable &&
!(block_part && block_part.IsContainer) &&
!should_ignore(client, block.path));
export function get_available_spaces() {
return available_spaces.sort((a, b) => block_cmp(a.block, b.block));
}

export function get_available_spaces(client) {
function make(path) {
const block = client.blocks[path];
const parts = get_block_link_parts(client, path);
const text = cockpit.format(parts.format, parts.link);
return { type: 'block', block, size: block.Size, desc: text };
}

const spaces = Object.keys(client.blocks).filter(p => is_available_block(client, client.blocks[p], true))
.sort(make_block_path_cmp(client))
.map(make);

function add_free_spaces(block) {
const parts = get_partitions(client, block);
let i;
let p;
let link_parts;
let text;
for (i in parts) {
p = parts[i];
if (p.type == 'free') {
link_parts = get_block_link_parts(client, block.path);
text = cockpit.format(link_parts.format, link_parts.link);
spaces.push({
type: 'free',
block,
start: p.start,
size: p.size,
desc: cockpit.format(_("unpartitioned space on $0"), text)
});
}
}
}
export function reset_available_spaces() {
available_spaces = [];
}

for (const p in client.blocks_ptable)
add_free_spaces(client.blocks[p]);
export function register_available_block_space(client, block) {
const parts = get_block_link_parts(client, block.path);
const text = cockpit.format(parts.format, parts.link);
available_spaces.push({ type: 'block', block, size: block.Size, desc: text });
}

return spaces;
export function register_available_free_space(client, block, partition) {
const link_parts = get_block_link_parts(client, block.path);
const text = cockpit.format(link_parts.format, link_parts.link);
available_spaces.push({
type: 'free',
block,
start: partition.start,
size: partition.size,
desc: cockpit.format(_("unpartitioned space on $0"), text)
});
}

export function prepare_available_spaces(client, spcs) {
Expand Down
Loading