Re: [PATCH v3 1/2] Build verity images in rootfs Nix derivation

Alyssa Ross
2025-11-13T11:46:23Z

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

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

> Avoid redundant rebuilds of the rootfs verity superblock and roothash.
> Remove duplicate code. Clean up Makefile to avoid temporary files.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
> host/initramfs/Makefile | 26 +++++---------------------
> host/initramfs/shell.nix | 4 +++-
> host/rootfs/Makefile | 45 +++++++++++++++++++++------------------------
> host/rootfs/default.nix | 2 +-
> host/rootfs/shell.nix | 2 +-
> release/live/Makefile | 26 +++++---------------------
> release/live/default.nix | 4 +++-
> 7 files changed, 39 insertions(+), 70 deletions(-)

Looking good!

> diff --git a/host/initramfs/Makefile b/host/initramfs/Makefile
> index cb13fbb35f065b67d291d4a35591d6f12720060c..102870ecba4456303414e2531ea592473ddfc1cf 100644
> --- a/host/initramfs/Makefile
> +++ b/host/initramfs/Makefile
> @@ -35,26 +35,10 @@ build/mountpoints:
> cd build/mountpoints && mkdir -p $(MOUNTPOINTS)
> find build/mountpoints -mindepth 1 -exec touch -d @0 {} ';'
>
> -# veritysetup format produces two files, but Make only (portably)
> -# supports one output per rule, so we combine the two outputs then
> -# define two more rules to separate them again.
> -build/rootfs.verity: $(ROOT_FS)
> - mkdir -p build
> - $(VERITYSETUP) format $(ROOT_FS) build/rootfs.verity.superblock.tmp \
> - | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
> - > build/rootfs.verity.roothash.tmp
> - cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
> - > $@
> - rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
> -build/rootfs.verity.roothash: build/rootfs.verity
> - head -n 1 build/rootfs.verity > $@
> -build/rootfs.verity.superblock: build/rootfs.verity
> - tail -n +2 build/rootfs.verity > $@
> -
> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(ROOT_FS)
> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_VERITY) $(ROOT_FS_VERITY_ROOTHASH) $(ROOT_FS)
> ../../scripts/make-gpt.sh $@.tmp \
> - build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
> - $(ROOT_FS):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
> + "$$ROOT_FS_VERITY:verity:$$(../../scripts/format-uuid.sh "$$(dd "if=$$ROOT_FS_VERITY_ROOTHASH" bs=32 skip=1 count=1 status=none)")" \

Indentation got messed up here.

Given rootfs has a well-defined output structure, maybe we could just
write $(ROOT_FS)/rootfs.verity.roothash, so we don't need to define lots
of different environment variables in each component that uses the
verity data.

I think we should consistently use Make variable expansion rather than
shell variable expansion when we're using the variable in a Make
dependency line too, to avoid the possibility of them being different.

> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
> index f107ca44fcf1ad85af5788d87f6c772910d40072..d7764d9b796f1773b4bebd0d50eec52b9be29e42 100644
> --- a/host/rootfs/Makefile
> +++ b/host/rootfs/Makefile
> @@ -6,7 +6,7 @@
> include ../../lib/common.mk
> include file-list.mk
>
> -dest = build/rootfs.erofs
> +dest = build
>
> DIRS = \
> dev \
> @@ -46,15 +46,27 @@ FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>
> BUILD_FILES = build/etc/s6-rc
>
> -$(dest): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
> - set -euo pipefail; \
> +# This rule produces three files but Make only (portably)
> +# supports one output per rule. Instead of resorting to temporary
> +# files, a timestamp file is created as the last step. The actual
> +# outputs are produced as side-effects.
> +$(dest)/timestamp: ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk $(dest)

Semes a bit odd to install the timestamp in $(dest).

> { \
> cat $(PACKAGES_FILE) ;\
> for file in $(FILES) $(LINKS); do printf '%s\n%s\n' $$file "$${file#image/}"; done ;\
> for file in $(BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
> printf 'build/empty\n%s\n' $(DIRS) ;\
> printf 'build/fifo\n%s\n' $(FIFOS) ;\
> - } | ../../scripts/make-erofs.sh $@
> + } | ../../scripts/make-erofs.sh $(dest)/rootfs
> + $(VERITYSETUP) format \
> + --root-hash-file $(dest)/rootfs.verity.roothash \
> + -- $(dest)/rootfs $(dest)/rootfs.verity.superblock
> + # Add trailing newline
> + echo >> $(dest)/rootfs.verity.roothash
> + touch -- $(dest)/timestamp

I prefer smaller independent Make rules where possible. Can't the
verity data stay in a separate rule from make-erofs.sh?

Either way, change deserves a copyright header IMO. :)

> +
> +$(dest):
> + mkdir -p $(dest)

Would rather this was inlined into every target in $(dest), because
directories can be a bit confusing to me as Make targets. "Does the
target just create the directory, or does it create the directory and
everything in it?" sort of thing.

>
> build/fifo:
> mkdir -p build
> @@ -83,25 +95,10 @@ clean:
> rm -rf build
> .PHONY: clean
>
> -# veritysetup format produces two files, but Make only (portably)
> -# supports one output per rule, so we combine the two outputs then
> -# define two more rules to separate them again.
> -build/rootfs.verity: $(dest)
> - $(VERITYSETUP) format $(dest) build/rootfs.verity.superblock.tmp \
> - | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
> - > build/rootfs.verity.roothash.tmp
> - cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
> - > $@
> - rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
> -build/rootfs.verity.roothash: build/rootfs.verity
> - head -n 1 build/rootfs.verity > $@
> -build/rootfs.verity.superblock: build/rootfs.verity
> - tail -n +2 build/rootfs.verity > $@
> -
> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(dest)
> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(dest)/timestamp
> ../../scripts/make-gpt.sh $@.tmp \
> - build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
> - $(dest):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
> + $(dest)/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(dest)/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
> + $(dest)/rootfs:root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(dest)/rootfs.verity.roothash)")
> mv $@.tmp $@
>
> debug:
> @@ -111,7 +108,7 @@ debug:
> $(VMLINUX)
> .PHONY: debug
>
> -run: build/live.img $(EXT_FS) build/rootfs.verity.roothash
> +run: build/live.img

What happened to $(EXT_FS)?

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