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

Add configration option for partition UUIDs #951

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add configration option for partition UUIDs #951

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1ae5c1
Add configration option for partition UUIDs
Rua Jan 24, 2025
41ca2d7
Use `concatMapStrings`
Rua Jan 24, 2025
79442d0
Use regex for UUID, adjust documentation a bit
Rua Jan 24, 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
64 changes: 43 additions & 21 deletions lib/types/gpt.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,18 @@ in
device = lib.mkOption {
type = lib.types.str;
default =
if config._parent.type == "mdadm" then
# workaround because mdadm partlabel do not appear in /dev/disk/by-partlabel
if partition.config.uuid != null then
"/dev/disk/by-partuuid/${diskoLib.hexEscapeUdevSymlink partition.config.uuid}"
Rua marked this conversation as resolved.
Show resolved Hide resolved
else if config._parent.type == "mdadm" then
# workaround because mdadm partlabel do not appear in /dev/disk/by-partlabel
"/dev/disk/by-id/md-name-any:${config._parent.name}-part${toString partition.config._index}"
else
"/dev/disk/by-partlabel/${diskoLib.hexEscapeUdevSymlink partition.config.label}";
defaultText = ''
if the parent is an mdadm device:
if `uuid` is provided:
/dev/disk/by-partuuid/''${diskoLib.hexEscapeUdevSymlink partition.config.uuid}

otherwise, if the parent is an mdadm device:
/dev/disk/by-id/md-name-any:''${config._parent.name}-part''${toString partition.config._index}

otherwise:
Expand Down Expand Up @@ -74,6 +79,16 @@ in
description = "Name of the partition";
default = name;
};
uuid = lib.mkOption {
Copy link

Choose a reason for hiding this comment

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

Maybe call the option guid so it's harder to mistake for the filesystem's id?

Copy link
Author

Choose a reason for hiding this comment

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

I called it uuid because, within the Linux context, it's always referred as that. For example, the name of /dev/disk/by-partuuid, blkid calls it PARTUUID etc. I could have named it partuuid, but I figured that if there is ever a separate filesystem UUID option, that would be distinguished as partition.content.uuid vs partition.uuid.

Copy link

Choose a reason for hiding this comment

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

Yeah, that works.

type = lib.types.nullOr lib.types.str;
default = null;
example = "809b3a2b-828a-4730-95e1-75b6343e415a";
description = ''
The UUID (also known as GUID) of the partition. Note that this is distinct from the UUID of the filesystem.
Copy link

Choose a reason for hiding this comment

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

Document which types of partition table support the option. Maybe also add a corresponding assertion to the module, to give a clear error rather than unexpected behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

Does disko support anything other than GPT? At least, I included it in the gpt.nix module assuming that it would only be available for GPT.

Copy link
Collaborator

Choose a reason for hiding this comment

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

currently we still support DOS with the legacy table, but that is deprecated and it's fine to do it here for gpt only


If none is provided, a random UUID will be generated when creating the partition.
'';
};
label = lib.mkOption {
type = lib.types.str;
default =
Expand Down Expand Up @@ -195,24 +210,31 @@ in
if ! blkid "${config.device}" >&2; then
sgdisk --clear "${config.device}"
fi
${lib.concatStrings (map (partition: ''
# try to create the partition, if it fails, try to change the type and name
if ! sgdisk \
--align-end ${lib.optionalString (partition.alignment != 0) ''--set-alignment=${builtins.toString partition.alignment}''} \
--new=${toString partition._index}:${partition.start}:${partition.end} \
--change-name="${toString partition._index}:${partition.label}" \
--typecode=${toString partition._index}:${partition.type} \
"${config.device}"
then sgdisk \
--change-name="${toString partition._index}:${partition.label}" \
--typecode=${toString partition._index}:${partition.type} \
"${config.device}"
fi
# ensure /dev/disk/by-path/..-partN exists before continuing
partprobe "${config.device}" || : # sometimes partprobe fails, but the partitions are still up2date
udevadm trigger --subsystem-match=block
udevadm settle
'') sortedPartitions)}
${lib.concatMapStrings (partition:
let
args = ''
--partition-guid="${toString partition._index}:${if partition.uuid == null then "R" else partition.uuid}" \
--change-name="${toString partition._index}:${partition.label}" \
--typecode=${toString partition._index}:${partition.type} \
"${config.device}" \
'';
createArgs = ''
--align-end ${lib.optionalString (partition.alignment != 0) ''--set-alignment=${builtins.toString partition.alignment}''} \
--new=${toString partition._index}:${partition.start}:${partition.end} \
'';
Comment on lines +216 to +225
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why this was split, but isn' the device more of a createArg?

Copy link
Author

Choose a reason for hiding this comment

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

Just to avoid repeating itself, nothing more. It seemed nicer this way. I can revert it to the original style if you want.

in
''
# try to create the partition, if it fails, try to change the type and name
if ! sgdisk ${createArgs} ${args}
then
sgdisk ${args}
fi
# ensure /dev/disk/by-path/..-partN exists before continuing
partprobe "${config.device}" || : # sometimes partprobe fails, but the partitions are still up2date
udevadm trigger --subsystem-match=block
udevadm settle
''
) sortedPartitions}

${
lib.optionalString (sortedHybridPartitions != [])
Expand Down