[4/7] system: add systemd generator for /var

This commit adds an alternative that has the following characteristics:

-   Dont depend on anything being available, except the
    API File Systems [1].

-   Be a clean drop-in, that can be trivially added / removed.

-   Runs before systemd loads its configuration.

    Could be extended to handle more tricky paths like /etc.

For more explaination, see the commit introducing the overlay method.

The implementation doesnt focus on being expandable but simple,
in the future there could be an attempt to source shell scripts
for configuration.
So that multiple paths can be covered (/etc),
or the mounts and handling specified
(do-nothing, copy-over or overlay original contents).

The intent is to have a direct replacement for the var factory method.

[1] - https://www.freedesktop.org/wiki/Software/systemd/APIFileSystems/

Cc: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Romain Naour <romain.naour@smile.fr>
Cc: Jérémy Rosen <jeremy.rosen@smile.fr>
Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 .../generator/br-mount-generator              | 19 +++++++++++++++++++
 .../skeleton-init-systemd.mk                  | 10 +++++++++-
 system/Config.in                              | 13 +++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100755 package/skeleton-init-systemd/generator/br-mount-generator

Hi Norbert,

  I'm not entirely sure about this one, but not completely against it either. 
Since I do have a few comments below, I've marked as Changes Requested.

On 15/01/2023 13:52, Norbert Lange wrote:
> This commit adds an alternative that has the following characteristics:

  This commit adds an alternative /var management method that has the following 
characteristics:

> 
> -   Dont depend on anything being available, except the
>      API File Systems [1].
> 
> -   Be a clean drop-in, that can be trivially added / removed.
> 
> -   Runs before systemd loads its configuration.
> 
>      Could be extended to handle more tricky paths like /etc.

  Well, for /etc there is no real reason for it to be writeable (but not 
persistent), is there? There are some debugging situations where it could be 
useful, but then I think you could just as well use the overlayfs approach.


  What is missing here still is why you'd prefer this over the overlay approach. 
That's explained in the help text though. Not that I really believe there's a 
good reason though - see below.


> For more explaination, see the commit introducing the overlay method.

  That's commit 10c637ab06d935bbce4c833c4ab9695cdb32b6c8 now. I put most of your 
explanation in the commit message so this reference should still apply.

  explaination -> explanation

> 
> The implementation doesnt focus on being expandable but simple,

  doesn't

> in the future there could be an attempt to source shell scripts
> for configuration.
> So that multiple paths can be covered (/etc),
> or the mounts and handling specified
> (do-nothing, copy-over or overlay original contents).

  I don't think it makes sense to refer to an unproven theoretical extension 
like trying for some kind of half-baked writeable overlay without using 
overlayfs. Well, I shouldn't be talking, my current project is in fact doing 
exactly that - but let me tell you, it's half baked :-)


> The intent is to have a direct replacement for the var factory method.
> 
> [1] - https://www.freedesktop.org/wiki/Software/systemd/APIFileSystems/
> 
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Romain Naour <romain.naour@smile.fr>
> Cc: Jérémy Rosen <jeremy.rosen@smile.fr>
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>   .../generator/br-mount-generator              | 19 +++++++++++++++++++
>   .../skeleton-init-systemd.mk                  | 10 +++++++++-
>   system/Config.in                              | 13 +++++++++++++
>   3 files changed, 41 insertions(+), 1 deletion(-)
>   create mode 100755 package/skeleton-init-systemd/generator/br-mount-generator
> 
> diff --git a/package/skeleton-init-systemd/generator/br-mount-generator b/package/skeleton-init-systemd/generator/br-mount-generator
> new file mode 100755
> index 0000000000..fd10659c1c
> --- /dev/null
> +++ b/package/skeleton-init-systemd/generator/br-mount-generator
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +set -e
> +# SPDX-License-Identifier: MIT

  The MIT license suggest that you copied this from somewhere else. If so, you 
must refer to the original - at least to the original author, as required by the 
MIT license.

> +# Note: doing heavy stuff here is not recommended, but this way
> +# the ordering is guaranteed. Be quick about everything.
> +# Know that the generator can be run again later.
> +
> +[ "${SYSTEMD_IN_INITRD-0}" = 1 ] && exit

  I don't like this redundant default of 0 much. But that's just minor.

> +grep '^tmpfs_br_var /var tmpfs' /proc/self/mounts >/dev/null && exit

  Why >/dev/null instead of grep -q?

> +
> +TMPMOUNT=/run/.br/.tmpvar
> +trap "umount -l $TMPMOUNT || :; rmdir $TMPMOUNT || :" 0
> +
> +mkdir -m755 -p $TMPMOUNT
> +mount -n -t tmpfs -o nosuid,nodev,size=50% tmpfs_br_var $TMPMOUNT > +
> +cp -r -p /var/. $TMPMOUNT
> +# this is a shared mount so --move wont work

  "this" seems to refer to /run/.br/.tmpvar, which is _not_ a shared mount (and 
it's anyway not relevant if it is). /run is a shared mount and that is the 
reason you can't --move it.

  And perhaps add "so instead, bind-mount and unmount $TMPMOUNT afterwards (in 
the trap handler)."

> +mount -n --bind $TMPMOUNT /var
> diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> index f431ec4612..b9b8492f58 100644
> --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> @@ -69,11 +69,19 @@ define SKELETON_INIT_SYSTEMD_POST_INSTALL_VAR_OVERLAYFS
>   		$(TARGET_DIR)/usr/lib/systemd/system/br-bindmount-run@.service
>   	$(INSTALL) -D -m 0644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/overlayfs/br-overlay-prepare@.service \
>   		$(TARGET_DIR)/usr/lib/systemd/system/br-overlay-prepare@.service
> -	# /var mount gets pulled in automatically by basic.target
> +	# var.mount gets pulled in automatically by basic.target

  If you're anyway going to correct this comment, correct it correctly :-) It's 
pulled in by local-fs.target, not basic.target.

>   endef
>   SKELETON_INIT_SYSTEMD_POST_INSTALL_TARGET_HOOKS += SKELETON_INIT_SYSTEMD_POST_INSTALL_VAR_OVERLAYFS
>   endif  # BR2_INIT_SYSTEMD_VAR_OVERLAYFS
>   
> +ifeq ($(BR2_INIT_SYSTEMD_VAR_GENERATOR),y)
> +define SKELETON_INIT_SYSTEMD_POST_INSTALL_VAR_GENERATOR
> +	$(INSTALL) -D -m 0755 $(SKELETON_INIT_SYSTEMD_PKGDIR)/generator/br-mount-generator \
> +		$(TARGET_DIR)/usr/lib/systemd/system-generators/br-var-mount-generator

  Please keep the same name in PKGDIR. I know the intention is to keep it 
general so it can be used for other things (e.g. /etc), but it _isn't_ general 
so don't try pretending it is.

> +endef
> +SKELETON_INIT_SYSTEMD_POST_INSTALL_TARGET_HOOKS += SKELETON_INIT_SYSTEMD_POST_INSTALL_VAR_GENERATOR
> +endif  # BR2_INIT_SYSTEMD_VAR_GENERATOR
> +
>   endif  # BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW
>   
>   ifeq ($(BR2_INIT_SYSTEMD_POPULATE_TMPFILES),y)
> diff --git a/system/Config.in b/system/Config.in
> index cdf383d0d4..14fefbf283 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -200,6 +200,19 @@ config BR2_INIT_SYSTEMD_VAR_OVERLAYFS
>   	  ExecStart=
>   	  ExecStart=/bin/mount --make-private -n /dev/sdc1 ${OVERLAY_DIR}
>   
> +config BR2_INIT_SYSTEMD_VAR_GENERATOR
> +	bool "use a systemd generator to mount and populate a tmpfs"
> +	help
> +	  Mount an tmpfs on /var, with the the original contents copied.
> +
> +	  The logic is contained in a systemd generator, to ensure this
> +	  is done early and ordered before anything can access /var.
> +
> +	  Unlike the var factory, nothing needs to be prepared and
> +	  the populating happens before any unit accesses /var.

  Well, nothing is _allowed_ to access /var before local-fs-pre.target, exactly 
because /var may be mounted (and mounting something depends on local-fs.target). 
If you do need to access /var, you need to do special magic like journald does 
and wait with using it until it gets mounted.

  Do you have an actual proven use case of something that requires /var before 
local-fs-pre.target (and that isn't an unacceptable hack)?

> +	  Unlike the var factory, it does not need an overlayfs.

  If this is the only reason, then I don't think it's worth the bother to 
replace the factory.


  Regards,
  Arnout

> +
>   config BR2_INIT_SYSTEMD_VAR_NONE
>   	bool "do nothing"
>   	help