Re: [PATCH v4 2/2] host/rootfs: Switch to systemd-udevd

[Alyssa Ross <hi@alyssa.is>]

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

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

> This replaces the mdevd uevent daemon with systemd-udevd, which supports
> much more hardware and is therefore to be preferred for a full desktop
> environment.  Various Wayland compositors integrate with systemd-udevd,
> allowing them to discover devices as they appear rather than having to
> only use devices plugged in when the compositor is started.
> Additionally, systemd-udevd has quirks for various devices that are
> needed to support the wide variety of hardware end-users have.
>
> The dependency on /dev/dri/card0 being present is eliminated, and
> whatever devices the user has are now picked up by the compositor.  New
> dependencies are added to ensure that udev coldplug has finished before
> any non-trivial services are started.  systemd-udev-trigger.service runs
> 'udevadm trigger' and has Before=sysinit.target, so anything that is not
> an early boot service can assume 'udevadm trigger' has run.
>
> systemd-udevd unsets PATH before it runs programs.  Therefore, manually
> set it to /usr/bin before invoking any scripts.
>
> systemd-udevd provides the 'udevadm wait' command to wait for a device
> to appear and be processed.  This replaces all uses of /etc/mdev/wait,
> so this script is removed.  /etc/mdev/listen and /run/wait only existed
> to wake up /etc/mdev/wait, so they are also removed.
>
> The command-line options to 'udevadm trigger' are taken from
> systemd-udev-trigger.service on my Arch machine.  Using the wrong
> arguments leads to problems that take a very long time to debug.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  LICENSES/LGPL-2.1-or-later.txt                     | 502 +++++++++++++++++++++
>  host/rootfs/Makefile                               |   2 +-
>  host/rootfs/default.nix                            | 118 ++---
>  host/rootfs/file-list.mk                           |  33 +-
>  host/rootfs/image/etc/mdev.conf                    |   7 -
>  host/rootfs/image/etc/mdev/listen                  |  11 -
>  host/rootfs/image/etc/mdev/wait                    |  14 -
>  host/rootfs/image/etc/s6-rc/card0/type.license     |   2 -
>  host/rootfs/image/etc/s6-rc/card0/up               |   4 -
>  host/rootfs/image/etc/s6-rc/kvm/timeout-up         |   1 -
>  host/rootfs/image/etc/s6-rc/kvm/timeout-up.license |   2 -
>  host/rootfs/image/etc/s6-rc/kvm/type               |   1 -
>  host/rootfs/image/etc/s6-rc/kvm/type.license       |   2 -
>  host/rootfs/image/etc/s6-rc/kvm/up                 |   4 -
>  host/rootfs/image/etc/s6-rc/mdevd-coldplug/type    |   1 -
>  host/rootfs/image/etc/s6-rc/mdevd-coldplug/up      |   4 -
>  .../image/etc/s6-rc/mdevd/notification-fd.license  |   2 -
>  host/rootfs/image/etc/s6-rc/mdevd/run              |   5 -
>  .../contents.d/systemd-udevd-coldplug}             |   0
>  .../dependencies.d/systemd-udevd}                  |   0
>  .../s6-rc/{card0 => systemd-udevd-coldplug}/type   |   0
>  .../type.license                                   |   0
>  .../image/etc/s6-rc/systemd-udevd-coldplug/up      |   6 +
>  .../s6-rc/{mdevd => systemd-udevd}/notification-fd |   0
>  .../s6-rc/systemd-udevd/notification-fd.license    |   2 +
>  host/rootfs/image/etc/s6-rc/systemd-udevd/run      |  15 +
>  .../image/etc/s6-rc/{mdevd => systemd-udevd}/type  |   0
>  .../s6-rc/{mdevd => systemd-udevd}/type.license    |   0
>  .../contents.d/systemd-udevd-coldplug}             |   0
>  .../contents.d/systemd-udevd-coldplug}             |   0
>  .../weston/dependencies.d/systemd-udevd-coldplug   |   0
>  host/rootfs/image/etc/s6-rc/weston/run             |   7 +-
>  .../image/etc/udev/rules.d/99-spectrum.rules       |  15 +
>  host/rootfs/image/usr/bin/run-vmm                  |   1 +
>  host/rootfs/image/usr/bin/systemd-udevd            |   1 +
>  .../{etc/mdev/net/add => usr/libexec/net-add}      |   0
>  36 files changed, 604 insertions(+), 158 deletions(-)

Also looking good, and again if you'd prefer I just make my suggested
changes myself when I apply it that's fine by me.

> diff --git a/host/rootfs/image/etc/s6-rc/systemd-udevd-coldplug/up b/host/rootfs/image/etc/s6-rc/systemd-udevd-coldplug/up
> new file mode 100644
> index 0000000000000000000000000000000000000000..f4bfe58d1219218ba486050e4de0f949df8f2058
> --- /dev/null
> +++ b/host/rootfs/image/etc/s6-rc/systemd-udevd-coldplug/up
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +# SPDX-FileCopyrightText: systemd is free software; you can redistribute it and/or modify it
> +#                         under the terms of the GNU Lesser General Public License as published by
> +#                         the Free Software Foundation; either version 2.1 of the License, or
> +#                         (at your option) any later version.

This is a statement of the license and is covered by the
SPDX-License-Identifier metadata.  SPDX-FileCopyrightText is for
identifying copyright owners.  For a project that doesn't provide a
copyright notice, we'd typically attribute copyright to e.g. "The
systemd Authors".

In this case though, I don't think there's anything copyrightable here,
so CC0-1.0 and your name should be fine.

> +udevadm trigger --type=all --action=add --prioritized-subsystem=module,block,tpmrm,net,tty,input

> diff --git a/host/rootfs/image/etc/s6-rc/systemd-udevd/run b/host/rootfs/image/etc/s6-rc/systemd-udevd/run
> new file mode 100644
> index 0000000000000000000000000000000000000000..98d1aa83153c99f32afb4d27a97cf39fcadb2968
> --- /dev/null
> +++ b/host/rootfs/image/etc/s6-rc/systemd-udevd/run
> @@ -0,0 +1,15 @@
> +#!/bin/execlineb -P
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +s6-setlock /run/sd-notify-wrapper/systemd-udevd.lock
> +# Save stdin to restore it later.
> +fdmove -c 4 0

stdin will always be /dev/null, so instead of this dance you can just
redirfd -r 0 /dev/null after the background.

> +s6-ipcserver-socketbinder -b0 -m -a 0600 /run/sd-notify-wrapper/systemd-udevd.sock
> +background {
> +   fdmove 1 3
> +   sd-notify-adapter
> +}
> +fdclose 3
> +fdmove 0 4
> +export NOTIFY_SOCKET /run/sd-notify-wrapper/systemd-udevd.sock
> +systemd-udevd
> diff --git a/host/rootfs/image/etc/s6-rc/weston/run b/host/rootfs/image/etc/s6-rc/weston/run
> index 7dca0dab095569c9e7d49df9d245533a7265283e..8276e7eed45ab8155e21a3e11f86c57c03d66cc4 100644
> --- a/host/rootfs/image/etc/s6-rc/weston/run
> +++ b/host/rootfs/image/etc/s6-rc/weston/run
> @@ -4,9 +4,9 @@
>  
>  unexport WAYLAND_DISPLAY
>  
> -foreground {
> -  umask 077
> -  mkdir /run/user/0
> +if {
> +  umask 0022
> +  mkdir -p -m 0700 /run/user/0

What's the point in continuing to umask when you explicitly provide the mode?

>  }
>  unexport ?
>  
> @@ -20,4 +20,5 @@ redirfd -r 0 /dev/tty1
>  
>  importas -i home HOME
>  cd $home
> +if { udevadm wait /dev/dri/card0 }
>  weston
> diff --git a/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules b/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules
> new file mode 100644
> index 0000000000000000000000000000000000000000..c65829419de3516d6ec8e95e7928a69c308507aa
> --- /dev/null
> +++ b/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +# systemd-udevd unsets PATH, so fix that.

Stray copy of comment.

> +ACTION=="remove", GOTO="spectrum-end"
> +ENV{PCI_CLASS}!="2????", GOTO="spectrum-end"
> +
> +# net-add unbinds and rebinds the driver, so avoid calling
> +# it more than once.
> +IMPORT{db}="SPECTRUM_DRIVER_ASSIGNED"
> +ENV{SPECTRUM_DRIVER_ASSIGNED}=="yes", GOTO="spectrum-end"
> +# systemd-udevd unsets PATH, so fix that.
> +RUN+="/usr/bin/env PATH=/usr/bin /usr/libexec/net-add"
> +ENV{SPECTRUM_DRIVER_ASSIGNED}="yes"
> +LABEL="spectrum-end"

Again, I'd much rather this was taken care of in net-add.  I'll post a
patch to make it re-entrant, I guess.

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