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

> On 11/13/25 11:44, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>> +# 9. Call systemd-sysupdate to run the actual update.
>>> +
>>> +if { mkdir -p -m 0700 /run/updater }
>>> +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 }
>>> +
>> 
>> Wouldn't it break if there's already stuff in it?
>
> No, it works fine in this case.  I checked :).
>
>> I'd do
>> 
>> 	foreground { redirfd -w 2 /dev/null btrfs subvolume delete -- shared }
>> 	if { btrfs subvolume create -- shared }
>> 
>> and then you know you've got an empty subvolume.
>
> An empty subvolume isn't good: it means that systemd-sysupdate will
> redownload an update even when it isn't needed.

When would the update have already been downloaded but not applied?
Only if there's an error in actually installing it?  I'd feel happier
knowing we were always starting with a clean state.

>>> +  # Snapshot directory may have files or directories with untrusted names.
>>> +  # Redirect its output to /dev/null to avoid printing them to the console.
>>> +  ifelse -n { redirfd -w 2 /dev/null rm -rf -- snapshot } {
>>> +    foreground { redirfd -w 2 echo "Cannot remove snapshot directory" }
>>> +    exit 1
>>> +  }
>> 
>> Why not btrfs subvolume delete?  It's faster and won't print names.
>
> It doesn't distinguish "subvolume doesn't exist" from "problem
> deleting subvolume".  A better solution is to call `rm -f` if `btrfs
> subvolume delete` failed.  That ignores "does not exist" errors,
> but not other errors.

Right.  I had assumed btrfs subvolume create would cause an error if it
already existed, but you've said it doesn't.

>>> +
>>> +  backtick -E update_vm_id_ {
>>> +    backtick -E id_path { readlink /run/vm/by-name/sys.appvm-updates }
>>> +    basename -- $id_path
>>> +  }
>>> +
>>> +  multisubstitute {
>>> +    define fsdir /run/vm/by-id/${update_vm_id_}/fs
>>> +    define update_vm_id ${update_vm_id_}
>> 
>> Why?
>
> Avoiding serial substitution.

But this is serial substitution.  You're substituting update_vm_id_, and
then you're doing another substitution of update_vm_id without the
underscore.  Why?  Why not the following?

  backtick update_vm_id { ... }
  multisubstitute {
    define fsdir ...
    importas -Siu update_vm_id
  }

>>> +  }
>>> +
>>> +  # $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.
>>> +
>>> +  # Set up /etc with what the VM needs.  The VM will overlay this
>>> +  # on its own /etc.
>>> +  if { rm -rf -- ${fsdir}/etc }
>>> +  if { umask 022 mkdir -p -- ${fsdir}/updates ${fsdir}/etc/systemd }
>>> +  if { cp -R -- /etc/updatevm/sysupdate.d /etc/updatevm/url-env ${fsdir}/etc }
>>> +  if { cp -- /etc/systemd/import-pubring.gpg ${fsdir}/etc/systemd }
>> 
>> Why copy rather than bind mount?
>
> Target does not exist and I didn't want to bind-mount all of /etc/systemd.

You can touch a file and then bind mount, and still save a copy.

>>> +
>>> +  # If the directory is already mounted, unmount it.  This prevents a
>>> +  # confusing error from mount.
>>> +  foreground { redirfd -w 2 /dev/null umount -- ${fsdir}/updates }
>>> +
>>> +  # Share the update directory with the VM.
>>> +  if { mount --bind -- shared ${fsdir}/updates }
>>> +
>>> +  # Start the update VM.
>>> +  if { vm-start $update_vm_id }
>>> +
>>> +  # Wait for the VM to exit.
>>> +  if { s6-svwait -D ${svcdir} }
>>> +
>> 
>> It might be more robust to use a transient VM, like we use for
>> AppImages, so that nothing can restart it.  Transient VMs are still
>> developing though, so it's also fine to say we'll do it this way for now
>> and adapt it later.  This would also save all the filesystem resetting
>> you're needing to do here.
>
> The path to the update directory is user-provided.  It's not from
> the VM's persistent storage.

Ideally (at some point) it isn't user provided, and is the VM's
transient disk-backed storage, IMO.

>>> @@ -17,5 +18,11 @@ let
>>>    callConfig = config: if builtins.typeOf config == "lambda" then config {
>>>      inherit default;
>>>    } else config;
>>> +  finalConfig = default // callConfig config;
>>>  in
>>> -  default // callConfig config;
>>> +  finalConfig // {
>>> +    update-signing-key = builtins.path {
>>> +      name = "signing-key";
>>> +      path = finalConfig.update-signing-key;
>>> +    };
>>> +  }
>> 
>> What does this do?
>
> This ensures that the Nix store path doesn't depend on the name of
> the update signing key, only its contents.

Interesting.  Does that matter, though?  It ends up being called
/etc/systemd/import-pubring.gpg in the image regardless.

>>> diff --git a/vm/app/updates.nix b/vm/app/updates.nix
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..d2c1e5fcb35b37c7ed8a173f19b97894a36a7f0c
>>> --- /dev/null
>>> +++ b/vm/app/updates.nix
>>> @@ -0,0 +1,37 @@
>>> +# 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, config, curl, lib, src
>>> +, runCommand, systemd, writeScript
>>> +}:
>>> +
>>> +let
>>> +  update-url = config.update-url;
>>> +  mountpoint = "/run/virtiofs/virtiofs0";
>>> +  sysupdate-path = "${systemd}/lib/systemd/systemd-sysupdate";
>>> +  runner = writeScript "update-run-script"
>>> +    ''
>>> +    #!/usr/bin/execlineb -P
>>> +    if { mount -toverlay -olowerdir=${mountpoint}/etc:/etc -- overlay /etc }
>>> +    envfile ${mountpoint}/etc/url-env
>> 
>> Seems like overkill to use an envfile for a single URL?
>
> It is indeed overkill, but I'm not aware of a simpler option.
> There is backtick + cat but that's two programs rather than one.

I think the canonical way would be redirfd + withstdinas, but that's
also two programs, so if you want to avoid that, perhaps s6-envdir?
Reading it isn't any simpler but writing it at least doesn't require a
special tool.

>>> +    importas -i update_url UPDATE_URL
>>> +    if { ${sysupdate-path} update }
>>> +    if { ${curl}/bin/curl -L --proto =http,https
>>> +       -o ${mountpoint}/updates/SHA256SUMS.gpg ''${update_url}/SHA256SUMS.gpg }
>>> +    # systemd-sysupdate recently went from needing SHA256SUMS.gpg to SHA256SUMS.sha256.asc.
>>> +    # I (Demi) have no need if this is intentional or a bug.  I also have no idea if this
>>> +    # behavior will stay unchanged in the future.  Therefore, create both files and let
>>> +    # systemd-sysupdate ignore the one it isn't interested in.
>>> +    if { ln -f ${mountpoint}/updates/SHA256SUMS.gpg ${mountpoint}/updates/SHA256SUMS.sha256.asc }
>> 
>> Would be good to figure out why that happened.  If we add a comment like
>> this it's very unlikely to ever get cleaned up.
>
> https://github.com/systemd/systemd/issues/39273

"hwdb: drop trailing whitespace"?

>>> +    ${curl}/bin/curl -L --proto =http,https
>>> +       -o ${mountpoint}/updates/SHA256SUMS ''${update_url}/SHA256SUMS
>>> +    '';
>>> +in
>>> +
>>> +callSpectrumPackage ../make-vm.nix {} {
>>> +  providers.net = [ "sys.netvm" ];
>>> +  type = "nix";
>>> +  run = "${runner}";
>> 
>> Might as well inline this.
>
> I chose to keep it separate to improve readability.

Okay.  I'd find it more readable inlined but it's your code. :)