Re: [PATCH v5 11/13] Support updates via systemd-sysupdate

Demi Marie Obenour
2025-11-28T20:27:57Z

[-- Attachment #1.1.1: Type: text/plain, Size: 11920 bytes --]

On 11/28/25 08:47, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
>
>> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
>> index a6d9f23e9f5277b7c79a53105eb2dfe1bab1451e..74ff64019560aae6387df0e1b3409bc174251bdb 100644
>> --- a/host/rootfs/Makefile
>> +++ b/host/rootfs/Makefile
>> @@ -10,6 +10,7 @@ include file-list.mk
>> ROOT_FS = build
>>
>> DIRS = \
>> + boot \
>> dev \
>> etc/s6-linux-init/env \
>> etc/s6-linux-init/run-image/configs \
>> @@ -33,13 +34,15 @@ DIRS = \
>> etc/s6-linux-init/run-image/vm/by-id \
>> etc/s6-linux-init/run-image/vm/by-name \
>> ext \
>> + home \
>> proc \
>> run \
>> - sys
>> + sys \
>> + tmp
>>
>> FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>>
>> -BUILD_FILES = build/etc/s6-rc
>> +BUILD_FILES = build/etc/s6-rc build/etc/os-release build/etc/update-url
>>
>> # This rule produces three files but Make only (portably)
>> # supports one output per rule. Instead of resorting to temporary
>> @@ -59,12 +62,22 @@ $(ROOT_FS_IMAGE): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_
>> mkdir -p $(ROOT_FS) && \
>> { \
>> cat $(PACKAGES_FILE) ;\
>> + printf '%s\n%s\n' "$$UPDATE_SIGNING_KEY" /etc/systemd/import-pubring.gpg; \
>
> Inconsistent use of shell variable instead of make macro.
>
>> 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 $@
>>
>> +build/etc/update-url:
>> + mkdir -p build/etc
>> + # might have metacharacters, so avoid interpolation
>> + printf %s\\n "$${UPDATE_URL:?'update URL empty or missing'}" > build/etc/update-url
>
> I'm learning so many shell parameter expansions I didn't know from you :)
>
>> diff --git a/host/rootfs/image/usr/bin/spectrum-update b/host/rootfs/image/usr/bin/spectrum-update
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..613b43570d0538fce20296ccb1de2a6364e0df55
>> --- /dev/null
>> +++ b/host/rootfs/image/usr/bin/spectrum-update
>> @@ -0,0 +1,92 @@
>> +#!/bin/execlineb -WS1
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +
>> +if { mkdir -p -m 0700 /run/updater }
>> +
>> +# Take a global lock to avoid races.
>> +s6-setlock /run/update-lock
>> +
>> +foreground { redirfd -w 2 /dev/null rmdir -- $1 }
>> +if { umask 0077 mkdir -p -- $1 }
>> +cd $1
>> +foreground {
>> + # If this exists already that is okay.
>> + foreground { redirfd -w 2 /dev/null btrfs subvolume create -- shared }
>> +
>> + # Delete any stale temporary files. Delete any existing signature
>> + # files. If the VM is still running (it should not be), the VM might
>> + # have write access to the directory. However, updates-dir-check is
>> + # safe against that.
>> + if { updates-dir-check cleanup shared }
>> +
>> + if {
>> + foreground {
>> + # TODO: suppress only "subvolume does not exist" errors.
>> + redirfd -w 2 /dev/null
>> + btrfs subvolume delete snapshot
>> + }
>> + rm -f snapshot
>> + }
>> +
>> + backtick -E update_vm_id {
>> + backtick -E id_path { readlink /run/vm/by-name/sys.appvm-systemd-sysupdate }
>> + basename -- $id_path
>> + }
>> +
>> + # $fsdir is read-only to the guest, but read-write to the host.
>> + # Directories bind-mounted into it are read-write to the guest.
>> + # See etc/s6-linux-init/run-image/service/vhost-user-fs/template/run
>> + # for details.
>> +
>
> This still refers to a non-existent variable.
>
>> + # Set up /etc with what the VM needs. The VM will overlay this
>> + # on its own /etc.
>> + #
>> + # In the future, this should use a bind mount instead of copying
>> + # into a tmpfs. However, this would significantly complicate the
>> + # cleanup code. Deleting fs/etc would require undoing the bind
>> + # mounts instead of rm -rf. Once this code is in a separate mount
>> + # namespace, the copies should be replaced by bind mounts.
>> + if {
>> + if { rm -rf -- /run/vm/by-id/${update_vm_id}/fs/etc }
>> + umask 022
>> + if { mkdir -p -- /run/vm/by-id/${update_vm_id}/fs/updates /run/vm/by-id/${update_vm_id}/fs/etc/systemd }
>> + if { cp -R -- /etc/vm-sysupdate.d /etc/update-url /run/vm/by-id/${update_vm_id}/fs/etc }
>> + cp -- /etc/systemd/import-pubring.gpg /run/vm/by-id/${update_vm_id}/fs/etc/systemd
>> + }
>> +
>> + # If the directory is already mounted, unmount it. This prevents a
>> + # confusing error from mount.
>> + foreground { redirfd -w 2 /dev/null umount -- /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> + # Share the update directory with the VM.
>> + if { mount --bind -- shared /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> + # Start the update VM.
>> + if { vm-start $update_vm_id }
>> +
>> + # Wait for the VM to exit.
>> + # TODO: This is racy. If the update finishes before this code runs,
>> + # the s6-svwait call will fail.
>> + if { s6-svwait -D /run/service/vmm/instance/${update_vm_id} }
>> +
>> + # Remove the bind mount.
>> + if { umount -- /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> + # Ensure that the VM cannot change the directory
>> + # while systemd-sysupdate is using it.
>> + if { btrfs subvolume snapshot -- shared snapshot }
>> +
>> + # Validate the update directory. Delete any stale temporary files.
>> + # Check that a signature file was downloaded.
>> + if { updates-dir-check check snapshot }
>> +
>> + unshare --mount
>> + if { mount --bind -o ro -- snapshot /run/updater }
>> +
>> + /usr/lib/systemd/systemd-sysupdate update
>
> Why not just make a readonly snapshot?
> (btrfs subvolume snapshot -r)

The checker will delete any temporary files it comes across, so it
needs write access. A snapshot is much heavier than a bind mount
and isn't automatically cleaned up.

>> diff --git a/vm/app/systemd-sysupdate/default.nix b/vm/app/systemd-sysupdate/default.nix
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..69be0bab500ea2ea6cb3b6d71edbf1a3e7bddbba
>> --- /dev/null
>> +++ b/vm/app/systemd-sysupdate/default.nix
>> @@ -0,0 +1,26 @@
>> +# SPDX-License-Identifier: MIT
>> +# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +
>> +import ../../../lib/call-package.nix (
>> +{ callSpectrumPackage, curl, lib, src
>> +, runCommand, systemd, writeScript
>> +}:
>> +
>> +let
>> + downloadUpdate = builtins.path {
>> + name = "download-update";
>> + path = ./download-update;
>> + };
>
> builtins.path is overkill here surely, as opposed to just writing
> ${./download-update} below?

${./download-update} includes the working directory in the Nix store
hash, which means that renaming your source tree forces an unnecessary
rebuild. builtins.path is the standard way to avoid this.

>> +in
>> +
>> +callSpectrumPackage ../../make-vm.nix {} {
>> + providers.net = [ "sys.netvm" ];
>> + type = "nix";
>> + run = writeScript "run-script" ''
>> +#!/usr/bin/env -S execlineb -WS0
>
> #!/bin/execlineb -WS0 would be fine — we know that'll exist in the VM.

Will fix.

>> diff --git a/vm/app/systemd-sysupdate/download-update b/vm/app/systemd-sysupdate/download-update
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..eada41c6c8ad5edcedd9f4d76b76492e0b8be826
>> --- /dev/null
>> +++ b/vm/app/systemd-sysupdate/download-update
>> @@ -0,0 +1,68 @@
>> +#!/usr/bin/env -S execlineb -WS0
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +export LC_ALL C
>> +export LANGUAGE C
>> +if { mount -toverlay -olowerdir=/run/virtiofs/virtiofs0/etc:/etc -- overlay /etc }
>> +backtick tmpdir { mktemp -d /tmp/sysupdate-XXXXXX }
>> +# Not a useless use of cat: if there are NUL bytes in the URL
>> +# busybox's awk might misbehave.
>> +backtick update_url { cat /etc/update-url }
>> +if {
>> + backtick sed_rhs {
>> + # Use awk to both validate the URL and to escape sed metacharacters.
>> + # Reject URLs with control characters, query parameters, or fragments.
>> + # They *cannot* work and so are rejected to produce better error messages.
>> + #
>> + # curl rejects control characters with "Malformed input to a URL function".
>> + # Fragment specifiers ("#") and query parameters ("?") break concatenating
>> + # /SHA256SUMS and /SHA256SUMS.sha256.asc onto the update URL. Also, it is
>> + # simpler to reject update URLs that contain whitespace than to try to
>> + # escape them.
>> + #
>> + # Backslash needs to be escaped once for systemd-sysupdate and again for sed.
>> + # Ampersand needs to be escaped once for sed.
>> + awk "BEGIN {
>> + update_url = ENVIRON[\"update_url\"];
>> + if (update_url ~ /^[^\\001-\\040?#\\x7F]+$/) {
>> + # Use & to avoid extra escaping (16 or 32 backslashes!)
>> + # and a divergence between POSIX and GNU awk.
>> + gsub(/\\\\/, \"&&&&\", update_url);
>> + gsub(/&/, \"\\\\\\\\&\", update_url);
>> + print update_url;
>> + exit 0;
>> + } else {
>> + print ARGV[2] > \"/dev/stderr\";
>> + exit 100;
>> + }
>> + }" -- $3
>> + "Bad update URL from host: control characters, whitespace, query parameters, and fragment specifiers not allowed"
>> + }
>> + elglob -w -0 transfer_file_ /etc/vm-sysupdate.d/*.transfer
>> + forx -E transfer_file { $transfer_file_ }
>> + backtick target_basename {
>> + basename -- $transfer_file
>> + }
>> + multisubstitute {
>> + importas -iuS sed_rhs
>> + importas -iuS target_basename
>> + importas -iuS tmpdir
>> + define sed_input $transfer_file
>> + }
>
> You could avoid some serial substitution here if you wanted, by not
> passing -E to forx:
>
> forx transfer_file { $transfer_file_ }
> backtick target_basename {
> importas -iuS transfer_file
> basename -- $transfer_file
> }
> multisubstitute {
> …
> }

I considered it and decided that the extra define in the
multisubstitute was cheaper than the extra importas process.

>> + redirfd -w 1 ${tmpdir}/${target_basename}
>> + sed -E -- "s#@UPDATE_URL@#${sed_rhs}#g" $sed_input
>
> Using awk to escape stuff for sed seems a bit Rube Goldberg. Would it
> make more sense to just do the replacement in the awk program? Actually
> a lot of this might be nicer in awk than execline? Feel free to tell me
> to leave it this way for now, though.

I'd prefer to leave it this way for now. Maybe add a TODO to clean
this up.

>> +}
>> +multisubstitute {
>> + importas -iuS update_url
>> + importas -iuS CURL_PATH
>> + importas -iuS SYSTEMD_SYSUPDATE_PATH
>> + importas -iuS tmpdir
>> +}
>> +if { $SYSTEMD_SYSUPDATE_PATH --definitions=${tmpdir} update }
>> +# [ and ] are allowed in update URLs so that IPv6 addresses work, but
>> +# they cause globbing in the curl command-line tool by default. Use --globoff
>> +# to disable this feature.
>> +if { $CURL_PATH -L --proto-redir =http,https --globoff
>> + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS -- ${update_url}/SHA256SUMS }
>> +$CURL_PATH -L --proto-redir =http,https --globoff
>> + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS.sha256.asc -- ${update_url}/SHA256SUMS.sha256.asc
>
> Much easier to understand now. Thanks!

You're welcome!
--
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]