Re: [PATCH v2 4/8] Adjust partition layout to support updates

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 18628 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> systemd-sysupdate has strict requirements on the partition layout:
>
> - The label of the active partition must match the template in the
>   .transfer file.  For instance, the root filesystem of Spectrum 0.0.0
>   will be in a partition with label Spectrum_0.0.0.
> - The label of the inactive partition must either be that of the old
>   version of Spectrum or "_empty".  The former indicates an incomplete
>   update.

Do you mean "the latter"?

> - The partition type UUID must conform to the Discoverable Partition
>   Specification.
>
> After installing an image to a partition, systemd-sysupdate updates the
> label of the partition to match the image's version.  However, it does
> not update the partition UUID.  Therefore, use the partition label, not
> the partition UUID, to find the root filesystem and its verity metadata.

Seems a bit odd, considering I got this trick from
systemd-veritysetup-generator.  Is that not compatible with
systemd-sysupdate?

> systemd-sysupdate will fail if the OS image does not fit in the
> partitions that the installer created.  Therefor, make the partitions
> very large so that there is plenty of room for the OS to grow.  This
> requires rewriting the code that calculates the partition sizes.
>
> Since the partition label includes the OS version, add an OS version
> number.  Use 0.0.0 to indicate that Spectrum OS is still in very early
> development and should not be used.  The version number can be
> overridden in the build configuration file.
>
> mkfs.ext4 is not able to produce images with files large enough to hold
> both the primary and backup copy of the root partition [1].  Reducing
> the sizes of partitions to be little greater than the size of the root
> filesystem image does not help.  The produced file is still too large.
> Therefore, compress the image, which causes it to be small enough that
> mkfs.ext4 can handle it.  This breaks the live image, so remove it.
> The live image will return once Spectrum switches to the GNOME OS
> installer [2].
>
> [1]: https://github.com/tytso/e2fsprogs/issues/254
> [2]: https://spectrum-os.org/lists/archives/spectrum-devel/87wm4dlkhz.fsf@alyssa.is/
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  Documentation/development/build-configuration.adoc | 11 ++++
>  host/efi.nix                                       |  5 +-
>  host/initramfs/Makefile                            | 12 ++--
>  host/initramfs/default.nix                         |  1 +
>  host/initramfs/etc/init                            | 17 ++---
>  host/initramfs/etc/probe                           | 20 +++---
>  host/initramfs/shell.nix                           |  2 +
>  host/rootfs/Makefile                               | 21 ++++---
>  host/rootfs/default.nix                            |  1 +
>  host/rootfs/shell.nix                              |  2 +
>  img/app/Makefile                                   |  2 +-
>  img/app/default.nix                                |  1 +
>  lib/config.default.nix                             |  1 +
>  lib/config.nix                                     |  3 +-
>  lib/kcmdline-utils.mk                              |  5 ++
>  release/checks/integration/try.c                   |  4 ++
>  release/checks/no-roothash.nix                     |  2 +-
>  release/combined/eosimages.nix                     | 14 +++--
>  release/combined/grub.cfg.in                       |  5 --
>  release/live/Makefile                              |  9 +--
>  release/live/default.nix                           |  8 ++-
>  release/live/shell.nix                             |  3 +-
>  scripts/format-uuid.awk                            | 35 +++++++++++
>  scripts/make-gpt.bash                              | 72 ++++++++++++++++++++++
>  scripts/make-gpt.sh                                | 67 +-------------------
>  scripts/make-live-image.sh                         | 43 +++++++++++++
>  scripts/sfdisk-field.awk                           |  3 +-
>  vm/sys/net/Makefile                                |  2 +-
>  vm/sys/net/default.nix                             |  1 +
>  29 files changed, 248 insertions(+), 124 deletions(-)

Would you mind splitting up this patch in your next submission?  This is
a lot to review all at once, and I doubt finding the rootfs from label
is very difficult to separate from changing the size of the partitions,
for example.  I don't think I'm going to be able to do a full, thorough
review with there being this many different things going on at once.

> diff --git a/Documentation/development/build-configuration.adoc b/Documentation/development/build-configuration.adoc
> index 545aa8c05ac40a101b5ee280015cde7ec4f3a66f..0659d104efeeb8f483c24d8ea8d38a5d928d9358 100644
> --- a/Documentation/development/build-configuration.adoc
> +++ b/Documentation/development/build-configuration.adoc
> @@ -40,3 +40,14 @@ for supported configuration attributes and their default values.
>    };
>  }
>  ----
> +
> +.config.nix to adjust the version of the OS
> +[example]
> +[source,nix]
> +----
> +{ default, ... }:
> +
> +{
> +  version = "0.0.1";
> +}
> +----

Not sure this is necessary.  The paragraph above already says to refer
to lib/config.default.nix to see supported configuration options, so
readers can learn about version there.  It's not like pkgsArgs where
it's quite complex to use so warrants additional handholding.

> diff --git a/host/initramfs/etc/probe b/host/initramfs/etc/probe
> index 4cbd00db52c1a7128b5c619a43d415675feaee0b..11a81c9be8f1adaef3cee17efdba1eb80e9fe3c7 100755
> --- a/host/initramfs/etc/probe
> +++ b/host/initramfs/etc/probe
> @@ -14,9 +14,13 @@ if -n {
>      forx -pE module { ext4 loop }
>      modprobe $module
>    }
> -  backtick -E uuid { lsblk -lnpo PARTUUID $mdev }
> +  backtick uuid { lsblk -lnpo PARTUUID $mdev }
> +  multisubstiute {
> +    define mdev_ $mdev
> +    importas -Si uuid
> +  }
>    if { mkdir -p /mnt/${uuid} }
> -  if { mount $mdev /mnt/${uuid} }
> +  if { mount $mdev_ /mnt/${uuid} }
>    find /mnt/${uuid} -name *.img -exec
>    losetup -Pf {}
>    ;

I don't understand what this change does.

> @@ -24,11 +28,13 @@ if -n {
>  
>  # Check whether we now have all the partitions we need to boot.
>  
> -importas -i rootfs_uuid ROOTFS_UUID
> -importas -i verity_uuid VERITY_UUID
> -
> -backtick -E rootfs_dev { findfs PARTUUID=${rootfs_uuid} }
> -backtick -E verity_dev { findfs PARTUUID=${verity_uuid} }
> +importas -i version x-spectrum-version
> +backtick rootfs_dev { findfs PARTLABEL=Spectrum_${version} }
> +backtick verity_dev { findfs PARTLABEL=Spectrum_${version}.verity }
> +multisubstitute {
> +  importas -iS rootfs_dev
> +  importas -iS verity_dev
> +}
>  
>  if { ln -s $rootfs_dev /dev/rootfs }
>  if { ln -s $verity_dev /dev/verity }

Using multisubstitute is a good change but should be a separate patch
since it's equally applicable to the current code as it is to yours.

> diff --git a/lib/config.nix b/lib/config.nix
> index e437cdbe9aa22dd0f9c8d7052ac331c8fccf6ce6..01bcfa2bb2d5c412e212f5a60d9032e89c8a7442 100644
> --- a/lib/config.nix
> +++ b/lib/config.nix
> @@ -18,5 +18,4 @@ let
>      inherit default;
>    } else config;
>  in
> -
> -default // callConfig config
> +  default // callConfig config;

Looks like an accidental inclusion.

> diff --git a/lib/kcmdline-utils.mk b/lib/kcmdline-utils.mk
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ed97c1a4b0c93d427fbb67f58736eee7fe09259
> --- /dev/null
> +++ b/lib/kcmdline-utils.mk
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +LIVE_IMAGE_DEPS = ../../scripts/format-uuid.awk ../../scripts/make-gpt.sh ../../scripts/make-gpt.bash ../../scripts/sfdisk-field.awk ../../scripts/make-live-image.sh ../../lib/kcmdline-utils.mk

Why is this called kcmdline-utils.mk?  It doesn't seem to have anything
to do with a kernel command line.

> diff --git a/release/checks/integration/try.c b/release/checks/integration/try.c
> index 4b874c0a7e9b48324497450fb5488e04576fd43b..c34b582230f75ff3374446468d2461a78c0099a6 100644
> --- a/release/checks/integration/try.c
> +++ b/release/checks/integration/try.c
> @@ -10,6 +10,10 @@ void test(struct config c)
>  {
>  	struct vm *vm;
>  
> +	// Spectrum's live image doesn't work right now.
> +	// Mark the test as skipped.
> +	exit(77);
> +
>  	c.drives.img = getenv_or_die("COMBINED_PATH");
>  
>  	vm = start_qemu(c);

We can just delete the test.

> diff --git a/release/combined/eosimages.nix b/release/combined/eosimages.nix
> index 0ac4c48374e7098a2b91f61fc07cebb2042ffbdc..ba44d9cd82d55d491293ed36cc0402db8ebd3ffe 100644
> --- a/release/combined/eosimages.nix
> +++ b/release/combined/eosimages.nix
> @@ -12,11 +12,15 @@ runCommand "eosimages.img" {
>    unsafeDiscardReferences = { out = true; };
>    dontFixup = true;
>  } ''
> +  set -o pipefail
>    mkdir dir
>    cd dir
> -  ln -s $image $imageName
> -  sha256sum $imageName > $imageName.sha256
> -  tar -chf $NIX_BUILD_TOP/eosimages.tar *
> -  tar2ext4 -i $NIX_BUILD_TOP/eosimages.tar -o $out
> -  e2label $out eosimages
> +  ln -s -- "$image" "$imageName"
> +  sha256sum -- "$imageName" > "$imageName.sha256" &
> +  pid=$!
> +  gzip -9 < "$image" > "$imageName.gz"
> +  sha256sum -- "$imageName.gz" > "$imageName.gz.sha256"
> +  wait "$pid"
> +  tar -ch -- "$imageName.gz" "$imageName.gz.sha256" "$imageName.sha256" | tar2ext4 -o "$out"
> +  e2label "$out" eosimages
>  '') (_: {})

My comments on this from last time still apply[1].

[1]: https://spectrum-os.org/lists/archives/spectrum-devel/87v7jyj5a3.fsf@alyssa.is

> diff --git a/scripts/format-uuid.awk b/scripts/format-uuid.awk
> new file mode 100644
> index 0000000000000000000000000000000000000000..a5349d68a4d29be5f750650236420c9b5a7257eb
> --- /dev/null
> +++ b/scripts/format-uuid.awk
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +function format_uuid(arg) {
> +	if (arg in found_so_far) {
> +		fail("Duplicate UUID, try changing the image (by even 1 bit)");
> +	}
> +	found_so_far[arg] = 1;
> +	print (substr(arg, 1, 8) "-" \
> +	       substr(arg, 9, 4) "-" \
> +	       substr(arg, 13, 4) "-" \
> +	       substr(arg, 17, 4) "-" \
> +	       substr(arg, 21, 12));
> +}
> +
> +function fail(msg) {
> +	print msg > "/dev/stderr";
> +	exit 1;
> +}
> +
> +BEGIN {
> +	FS = "";
> +	RS = "\n";
> +	if ((getline) != 1)
> +		fail("Empty input file");
> +	roothash = $0;
> +	if (roothash !~ /^[a-f0-9]{64}$/)
> +		fail("Invalid root hash");
> +	if (getline)
> +		fail("Junk after root hash");
> +	found_so_far[""] = "";
> +	for (i = 1; i != 49; i += 16) {
> +		format_uuid(substr($0, i, 32));
> +	}
> +	format_uuid(substr($0, 49, 16) substr($0, 1, 16));
> +}

So now we have two format-uuid scripts, one in sh and one in awk?  Why?
What was wrong with the sh one?

> diff --git a/scripts/make-gpt.bash b/scripts/make-gpt.bash
> new file mode 100644
> index 0000000000000000000000000000000000000000..f9d53817e3cc4342cac5d4c832cf4aa129880399
> --- /dev/null
> +++ b/scripts/make-gpt.bash
> @@ -0,0 +1,72 @@
> +#!/usr/bin/bash --
> +# SPDX-FileCopyrightText: 2021-2023 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2022 Unikie
> +# SPDX-License-Identifier: EUPL-1.2+
> +#
> +# usage: make-gpt.sh GPT_PATH PATH:PARTTYPE[:PARTUUID[:PARTLABEL]]...
> +
> +set -xeuo pipefail
> +ONE_MiB=1048576
> +
> +# Prints the number of 1MiB blocks required to store the file named
> +# $1.  We use 1MiB blocks because that's what sfdisk uses for
> +# alignment.  It would be possible to get a slightly smaller image
> +# using actual normal-sized 512-byte blocks, but it's probably not
> +# worth it to configure sfdisk to do that.
> +sizeMiB() {
> +	wc -c "$1" | awk -v ONE_MiB=$ONE_MiB \
> +		'{printf "%d\n", ($1 + ONE_MiB - 1) / ONE_MiB}'
> +}
> +
> +# Copies from path $3 into partition number $2 in partition table $1.
> +fillPartition() {
> +	start="$(sfdisk -J "$1" | jq -r --argjson index "$2" \
> +		'.partitiontable.partitions[$index].start * 512')"
> +
> +	# GNU cat will use copy_file_range(2) if possible, whereas dd
> +	# will always do a userspace copy, which is significantly slower.
> +	lseek -S 1 "$start" cat "$3" 1<>"$1"
> +}
> +
> +# Prints the partition path from a PATH:PARTTYPE[:PARTUUID[:PARTLABEL]] string.
> +partitionPath() {
> +	awk -F: '{print $1}' <<EOF
> +$1
> +EOF
> +}
> +
> +scriptsDir="$(dirname "$0")"
> +
> +out="$1"
> +shift
> +
> +table="label: gpt"
> +
> +# Keep 1MiB free at the start, and 1MiB free at the end.
> +gptBytes=$((ONE_MiB * 2))
> +for partition; do
> +	if [[ "$partition" =~ :([1-9][0-9]*)MiB$ ]]; then
> +		sizeMiB=${BASH_REMATCH[1]}
> +		partition=${partition%:*}
> +	else
> +		partitionPath=$(partitionPath "$partition")
> +		sizeMiB=$(sizeMiB "$partitionPath")
> +	fi

Would be a lot simpler to just multiply by 1024 * 1024 in whatever runs
this script, wouldn't it?

> diff --git a/scripts/make-gpt.sh b/scripts/make-gpt.sh
> index 96f0d2c8494c093558c0e32e7e920b569bb078ef..665057da8281d2b5282081e4999098fbaa29e6ca 100755
> --- a/scripts/make-gpt.sh
> +++ b/scripts/make-gpt.sh
> @@ -1,65 +1,4 @@
> -#!/bin/sh -eu
> -#
> -# SPDX-FileCopyrightText: 2021-2023 Alyssa Ross <hi@alyssa.is>
> -# SPDX-FileCopyrightText: 2022 Unikie
> +#!/bin/sh --
>  # SPDX-License-Identifier: EUPL-1.2+
> -#
> -# usage: make-gpt.sh GPT_PATH PATH:PARTTYPE[:PARTUUID[:PARTLABEL]]...
> -
> -ONE_MiB=1048576
> -
> -# Prints the number of 1MiB blocks required to store the file named
> -# $1.  We use 1MiB blocks because that's what sfdisk uses for
> -# alignment.  It would be possible to get a slightly smaller image
> -# using actual normal-sized 512-byte blocks, but it's probably not
> -# worth it to configure sfdisk to do that.
> -sizeMiB() {
> -	wc -c "$1" | awk -v ONE_MiB=$ONE_MiB \
> -		'{printf "%d\n", ($1 + ONE_MiB - 1) / ONE_MiB}'
> -}
> -
> -# Copies from path $3 into partition number $2 in partition table $1.
> -fillPartition() {
> -	start="$(sfdisk -J "$1" | jq -r --argjson index "$2" \
> -		'.partitiontable.partitions[$index].start * 512')"
> -
> -	# GNU cat will use copy_file_range(2) if possible, whereas dd
> -	# will always do a userspace copy, which is significantly slower.
> -	lseek -S 1 "$start" cat "$3" 1<>"$1"
> -}
> -
> -# Prints the partition path from a PATH:PARTTYPE[:PARTUUID[:PARTLABEL]] string.
> -partitionPath() {
> -	awk -F: '{print $1}' <<EOF
> -$1
> -EOF
> -}
> -
> -scriptsDir="$(dirname "$0")"
> -
> -out="$1"
> -shift
> -
> -nl='
> -'
> -table="label: gpt"
> -
> -# Keep 1MiB free at the start, and 1MiB free at the end.
> -gptBytes=$((ONE_MiB * 2))
> -for partition; do
> -	sizeMiB="$(sizeMiB "$(partitionPath "$partition")")"
> -	table="$table${nl}size=${sizeMiB}MiB,$(awk -f "$scriptsDir/sfdisk-field.awk" -v partition="$partition")"
> -	gptBytes="$((gptBytes + sizeMiB * ONE_MiB))"
> -done
> -
> -rm -f "$out"
> -truncate -s "$gptBytes" "$out"
> -sfdisk --no-reread --no-tell-kernel "$out" <<EOF
> -$table
> -EOF
> -
> -n=0
> -for partition; do
> -	fillPartition "$out" "$n" "$(partitionPath "$partition")"
> -	n="$((n + 1))"
> -done
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +exec bash -- "${0%.sh}.bash" "$@"

Not sure what the purpose of this is.  We don't need to keep old paths
working.

> diff --git a/scripts/make-live-image.sh b/scripts/make-live-image.sh
> new file mode 100755
> index 0000000000000000000000000000000000000000..6608cc35b7a15178adf5ff3d3917b5243c5da6cd
> --- /dev/null
> +++ b/scripts/make-live-image.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh --
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +set -euo pipefail
> +case $0 in
> +(/*) dir=${0%/*}/;;
> +(*/*) dir=./${0%/*};;
> +(*) dir=.;;
> +esac
> +usage () {
> +  echo 'Usage: make-live-image.sh [release|live] OUTPUT_FILE' >&2
> +  exit 1
> +}
> +if [ "$#" != 2 ]; then usage; fi
> +file_type=$1 output_file=$2
> +for i in "$ROOT_FS" "$ROOT_FS_VERITY" "$ROOT_FS_VERITY_ROOTHASH" "$VERSION"; do
> +  # Some characters not special to the shell can't be handled by this code.
> +  case $i in
> +  (-*|*[!A-Za-z0-9._/+@-]*) printf 'Forbidden characters in "%s"\n' "$i" >&2; exit 1;;
> +  esac
> +done
> +root_hashes=$(LC_ALL=C awk -f "${dir}/format-uuid.awk" < "$ROOT_FS_VERITY_ROOTHASH")
> +# The awk script produces output that is meant for field splitting
> +# and has no characters special for globbing.
> +# shellcheck disable=SC2086
> +set -- $root_hashes
> +case $file_type in
> +(release)
> +  "$dir/make-gpt.sh" "$output_file.tmp" \
> +    build/boot.fat:c12a7328-f81f-11d2-ba4b-00a0c93ec93b \
> +    "$ROOT_FS_VERITY:verity:$1:Spectrum_$VERSION.verity:1024MiB" \
> +    "$ROOT_FS:root:$2:Spectrum_$VERSION:20480MiB" \
> +    "/dev/null:verity:$3:_empty:1024MiB" \
> +    "/dev/null:root:$4:_empty:20480MiB"
> +  ;;
> +(live)
> +  "$dir/make-gpt.sh" "$output_file.tmp" \
> +    "$ROOT_FS_VERITY:verity:$1:Spectrum_$VERSION.verity" \
> +    "$ROOT_FS:root:$2:Spectrum_$VERSION";;
> +(*) usage;;
> +esac
> +mv -- "$output_file.tmp" "$output_file"

If we need separate modes for each caller anyway, I don't think there's
having them in a shared script is a win over having them local to the
place they're used.

> diff --git a/scripts/sfdisk-field.awk b/scripts/sfdisk-field.awk
> index e13c86d2fb11a066eebd043808e659b08dbd269c..72eec9a0a770563d32da14440fe2552eb2e39b68 100644
> --- a/scripts/sfdisk-field.awk
> +++ b/scripts/sfdisk-field.awk
> @@ -24,6 +24,7 @@ BEGIN {
>  			arch = _arch
>  	}
>  
> +	comma = ""
>  	for (n in fields) {
>  		if (n <= skip)
>  			continue
> @@ -33,6 +34,6 @@ BEGIN {
>  				fields[n] = uuid
>  		}
>  
> -		printf "%s=%s,", keys[n - skip], fields[n]
> +		printf ",%s%s=%s", comma, keys[n - skip], fields[n]
>  	}
>  }

I don't understand.  The comma variable is always empty?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]