Re: [PATCH v4 06/14] Support generating multiple partition UUIDs

[Alyssa Ross <hi@alyssa.is>]

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

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

> Generate 4 partition UUIDs instead of just 2.  Port
> scripts/format-uuid.sh to awk to make this much easier.

Would have been nice to see the awk port first with unchanged behaviour,
so it was then easier to see what behaviour was actually getting changed
here.

> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
> Changes since v2:
> - Split into separate commit.
> ---
>  host/initramfs/Makefile  |  8 +++++---
>  host/rootfs/Makefile     |  6 ++++--
>  release/live/Makefile    |  8 +++++---
>  release/live/default.nix |  2 +-
>  scripts/format-uuid.awk  | 35 +++++++++++++++++++++++++++++++++++
>  scripts/format-uuid.sh   | 19 -------------------
>  6 files changed, 50 insertions(+), 28 deletions(-)

No blockers in here, but there are improvements I'd like to see made at
some point, even if that's after this has been applied.

> diff --git a/host/initramfs/Makefile b/host/initramfs/Makefile
> index fd8cbb6c3e775ed27d0a524bf167cb4d3940d799..27a26b46a8110d35ee02a63b12931d6b9c2742e5 100644
> --- a/host/initramfs/Makefile
> +++ b/host/initramfs/Makefile
> @@ -35,10 +35,12 @@ build/mountpoints:
>  	cd build/mountpoints && mkdir -p $(MOUNTPOINTS)
>  	find build/mountpoints -mindepth 1 -exec touch -d @0 {} ';'
>  
> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_IMAGES)
> +build/live.img: ../../scripts/format-uuid.awk ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/boot.fat $(ROOT_FS_IMAGES)
> +	uuids=$$(awk -f ../../scripts/format-uuid.awk < $(ROOT_FS_VERITY_ROOTHASH)) && \
> +	set -euo pipefail -- $$uuids && \

Nice trick — I like it.  There's no need to set -eo pipefail on Make
commands that don't use ; or | though.  If we did that consistently our
Makefiles would be unreadable because it'd be hard to see past all the
sets to the actual functionality, and seeing this here when it's not set
on every command makes me wonder what's special about this one, which
turns out to be nothing.

>  	bash ../../scripts/make-gpt.sh $@.tmp \
> -	$(ROOT_FS_VERITY):verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(ROOT_FS_VERITY_ROOTHASH) bs=32 skip=1 count=1 status=none)") \
> -	    $(ROOT_FS):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(ROOT_FS_VERITY_ROOTHASH))")
> +	    $(ROOT_FS_VERITY):verity:$$3 \
> +	    $(ROOT_FS):root:$$1
>  	mv $@.tmp $@
>  
>  build/loop.tar: build/live.img
> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
> index fcfbc3e437fdb108252ba77d4d4e8f4f636ffd78..f02bb76371f000e3f65bb7c2a7f217d437845481 100644
> --- a/host/rootfs/Makefile
> +++ b/host/rootfs/Makefile
> @@ -90,9 +90,11 @@ clean:
>  .PHONY: clean
>  
>  build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_DIR)/verity-timestamp $(ROOT_FS)
> +	uuids=$$(awk -f ../../scripts/format-uuid.awk < $(ROOT_FS_VERITY_ROOTHASH)) && \
> +	set -euo pipefail -- $$uuids && \
>  	bash ../../scripts/make-gpt.sh $@.tmp \
> -	    $(ROOT_FS)/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(ROOT_FS_VERITY_ROOTHASH) bs=32 skip=1 count=1 status=none)") \
> -	    $(ROOT_FS)/rootfs:root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(ROOT_FS_VERITY_ROOTHASH)")
> +	    $(ROOT_FS_VERITY):verity:$$3 \
> +	    $(ROOT_FS):root:$$1
>  	mv $@.tmp $@
>  
>  debug:
> diff --git a/release/live/Makefile b/release/live/Makefile
> index a79947c57d562677760bc669c66320953a2b0d2d..78361a48512a37514ba0e57e0cc8b0ec3a71664b 100644
> --- a/release/live/Makefile
> +++ b/release/live/Makefile
> @@ -9,11 +9,13 @@ DTBS ?= build/empty
>  
>  dest = build/live.img
>  
> -$(dest): ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/boot.fat $(ROOT_FS_IMAGES)
> +$(dest): ../../scripts/format-uuid.awk ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/boot.fat $(ROOT_FS_IMAGES)
> +	uuids=$$(awk -f ../../scripts/format-uuid.awk < $(ROOT_FS_VERITY_ROOTHASH)) && \
> +	set -euo pipefail -- $$uuids && \
>  	bash ../../scripts/make-gpt.sh $@.tmp \
>  	    build/boot.fat:c12a7328-f81f-11d2-ba4b-00a0c93ec93b \
> -	    $(ROOT_FS_VERITY):verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(ROOT_FS_VERITY_ROOTHASH) bs=32 skip=1 count=1 status=none)") \
> -	    $(ROOT_FS):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(ROOT_FS_VERITY_ROOTHASH))")
> +	    $(ROOT_FS_VERITY):verity:$$3 \
> +	    $(ROOT_FS):root:$$1
>  	mv $@.tmp $@
>  
>  build/empty:
> diff --git a/release/live/default.nix b/release/live/default.nix
> index ac2d7a55fd4fe0c02108309ecea20e368000af0d..98cb4862e239e3ad9ddbd7b5ace5716f57df683b 100644
> --- a/release/live/default.nix
> +++ b/release/live/default.nix
> @@ -29,7 +29,7 @@ stdenv.mkDerivation {
>      fileset = lib.fileset.intersection src (lib.fileset.unions [
>        ./.
>        ../../lib/common.mk
> -      ../../scripts/format-uuid.sh
> +      ../../scripts/format-uuid.awk
>        ../../scripts/make-gpt.sh
>        ../../scripts/sfdisk-field.awk
>      ]);
> 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)");
> +	}

This doesn't really feel like the right place for this check?  If
duplicate UUIDs are broken that should be detected when assembling the
partition table.

> +	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";

RS is \n by default.

> +	if ((getline) != 1)

No need for the inner parens.

> +		fail("Empty input file");
> +	roothash = $0;
> +	if (roothash !~ /^[a-f0-9]{64}$/)
> +		fail("Invalid root hash");
> +	if (getline)
> +		fail("Junk after root hash");

This will only ever be used by our build system.  It'll be readily
apparent if it's providing junk even without these validations, and
without them the script would be shorter and easier to fit in my brain.

> +	found_so_far[""] = "";

What does this do?  Won't this just cause the script to falsely claim an
empty string is a duplicate UUID if it ever ends up being passed to
format_uuid()?

> +	for (i = 1; i != 49; i += 16) {
> +		format_uuid(substr($0, i, 32));
> +	}

Either unrolling the loop, or using a modulo inside the loop and getting
rid of the format_uuid() call afterwards, would have helped me figure
out what this does a lot quicker, but I see now that we're generating
the two UUIDs we had before, and then two additional UUIDs offset into
the string.

I guess this is something to do with having multiple copies of the OS
installed, but I wish I understood how the offset ones will be used
without having to go hunting in later patches and come back here
afterwards.

> +	format_uuid(substr($0, 49, 16) substr($0, 1, 16));
> +}

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