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....
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....
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?