diff mbox series

[1/6,v3] package/skeleton-systemd: move /var factory tmpfiles out of /etc

Message ID 18712_1666122192_634F01D0_18712_304_1_aadcab6f9e8f71b5581c8a15b2773a431bb29ca0.1666122184.git.yann.morin@orange.com
State Accepted
Headers show
Series [1/6,v3] package/skeleton-systemd: move /var factory tmpfiles out of /etc | expand

Commit Message

Yann E. MORIN Oct. 18, 2022, 7:43 p.m. UTC 
When the rootfs is not remounted read-write (thus assuming a read-only
rootfs like squashfs), we create a tmpfiles.d factory for /var.

However, we register those in /etc/tmpfiles.d/, but /etc could also be
a tmpfs (for full state-less systems, or easy factory-reset, see [0]).

So, we move our var factory to /usr/lib/tmpfiles.d/, which is also the
location where systemd itself places its own tmpfiles, and where we
already put all our other tmpfiles (see audit, avahi, cryptsetup, dhcp,
lighttpd, nfs-utils, quagga, samba4, swupdate) and our handling of
systemd's catalog files too. We also rename the file to a better name,
so that it is obvious it is generated by us (systemd already installs a
var.conf of its own, so we want to avoid name clashing).

Last little detail: there is no need or reason to create .../tmpfiles.d/
at install time; it is only needed in the rootfs-pre-cmd hook, so we
only create it just before we need it.

[0] http://0pointer.de/blog/projects/stateless.html

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Cc: Norbert Lange <nolange79@gmail.com>
Cc: Romain Naour <romain.naour@smile.fr>
Cc: Jérémy Rosen <jeremy.rosen@smile.fr>
[yann.morin.1998@free.fr:
  - split original patch in two
  - this one only moves out of /etc and into /usr/lib
  - adapt commit log accordingly
]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
 package/skeleton-init-systemd/skeleton-init-systemd.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Norbert Lange Nov. 6, 2022, 3:40 p.m. UTC | #1 
Am Di., 18. Okt. 2022 um 21:43 Uhr schrieb <yann.morin@orange.com>:
>
> When the rootfs is not remounted read-write (thus assuming a read-only
> rootfs like squashfs), we create a tmpfiles.d factory for /var.
>
> However, we register those in /etc/tmpfiles.d/, but /etc could also be
> a tmpfs (for full state-less systems, or easy factory-reset, see [0]).
>
> So, we move our var factory to /usr/lib/tmpfiles.d/, which is also the
> location where systemd itself places its own tmpfiles, and where we
> already put all our other tmpfiles (see audit, avahi, cryptsetup, dhcp,
> lighttpd, nfs-utils, quagga, samba4, swupdate) and our handling of
> systemd's catalog files too. We also rename the file to a better name,
> so that it is obvious it is generated by us (systemd already installs a
> var.conf of its own, so we want to avoid name clashing).
>
> Last little detail: there is no need or reason to create .../tmpfiles.d/
> at install time; it is only needed in the rootfs-pre-cmd hook, so we
> only create it just before we need it.
>
> [0] http://0pointer.de/blog/projects/stateless.html
>
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Norbert Lange <nolange79@gmail.com>
> Cc: Romain Naour <romain.naour@smile.fr>
> Cc: Jérémy Rosen <jeremy.rosen@smile.fr>
> [yann.morin.1998@free.fr:
>   - split original patch in two
>   - this one only moves out of /etc and into /usr/lib
>   - adapt commit log accordingly
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> ---
>  package/skeleton-init-systemd/skeleton-init-systemd.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> index 795a171809..7b66732ef4 100644
> --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> @@ -29,7 +29,6 @@ else
>  # a real (but empty) directory, and the "factory files" will be copied
>  # back there by the tmpfiles.d mechanism.
>  define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW
> -       mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
>         echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab
>         echo "tmpfs /var tmpfs mode=1777 0 0" >>$(TARGET_DIR)/etc/fstab
>  endef
> @@ -38,6 +37,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>         rm -rf $(TARGET_DIR)/usr/share/factory/var
>         mv $(TARGET_DIR)/var $(TARGET_DIR)/usr/share/factory/var
>         mkdir -p $(TARGET_DIR)/var
> +       mkdir -p $(TARGET_DIR)/usr/lib/tmpfiles.d
>         for i in $(TARGET_DIR)/usr/share/factory/var/* \
>                  $(TARGET_DIR)/usr/share/factory/var/lib/* \
>                  $(TARGET_DIR)/usr/share/factory/var/lib/systemd/*; do \
> @@ -51,7 +51,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>                         printf "C! %s - - - -\n" "$${j}" \
>                         || exit 1; \
>                 fi; \

From tmpfiles.d docs: "C ... Recursively copy a file or directory, if
the destination files or directories do not
  exist yet or the destination directory is empty. Note that this
command will not descend into subdirectories
  if the destination directory already exists and is not empty.
Instead, the entire copy operation is skipped."

So all the drama could be just replaced with "C! /var - - - -\n" imho.

(non standard permissions/owner are probably gone either way)

> -       done >$(TARGET_DIR)/etc/tmpfiles.d/var-factory.conf
> +       done >$(TARGET_DIR)/usr/lib/tmpfiles.d/buildroot-factory.conf

How about naming this something like 00-buildroot-var.conf, so hopefully any
*.conf that might expect something in /var existing is executed later.

>  endef
>  SKELETON_INIT_SYSTEMD_ROOTFS_PRE_CMD_HOOKS += SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>
> --
> 2.25.1
>
>
> _________________________________________________________________________________________________________________________
>
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
>
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.
>

Regardless, the changes are an improvement, buildroot is the "distro"
and its special sauce
should reside inside /usr as much as possible.
Id highly recommend atleast changing the name to
00-buildroot-var.conf, other than that:

Acked-by: Norbert Lange <nolange79@gmail.com>
Yann E. MORIN Nov. 6, 2022, 3:58 p.m. UTC | #2 
Norbert, All,

On 2022-11-06 16:40 +0100, Norbert Lange spake thusly:
> Am Di., 18. Okt. 2022 um 21:43 Uhr schrieb <yann.morin@orange.com>:
[--SNIP--]
> > diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > index 795a171809..7b66732ef4 100644
> > --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > @@ -29,7 +29,6 @@ else
> >  # a real (but empty) directory, and the "factory files" will be copied
> >  # back there by the tmpfiles.d mechanism.
> >  define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW
> > -       mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
> >         echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab
> >         echo "tmpfs /var tmpfs mode=1777 0 0" >>$(TARGET_DIR)/etc/fstab
> >  endef
> > @@ -38,6 +37,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> >         rm -rf $(TARGET_DIR)/usr/share/factory/var
> >         mv $(TARGET_DIR)/var $(TARGET_DIR)/usr/share/factory/var
> >         mkdir -p $(TARGET_DIR)/var
> > +       mkdir -p $(TARGET_DIR)/usr/lib/tmpfiles.d
> >         for i in $(TARGET_DIR)/usr/share/factory/var/* \
> >                  $(TARGET_DIR)/usr/share/factory/var/lib/* \
> >                  $(TARGET_DIR)/usr/share/factory/var/lib/systemd/*; do \
> > @@ -51,7 +51,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> >                         printf "C! %s - - - -\n" "$${j}" \
> >                         || exit 1; \
> >                 fi; \
> 
> From tmpfiles.d docs: "C ... Recursively copy a file or directory, if
> the destination files or directories do not
>   exist yet or the destination directory is empty. Note that this
> command will not descend into subdirectories
>   if the destination directory already exists and is not empty.
> Instead, the entire copy operation is skipped."
> 
> So all the drama could be just replaced with "C! /var - - - -\n" imho.
> 
> (non standard permissions/owner are probably gone either way)

I do remember that I had to explicitly register individual entries
rather than the directory alone, as that did not work. That was 5+
years ago (commit 26085bbbd500), and I did not write it down in the
commit log...

However, it looks like we hit this issue, as hinted in commit
7e811708f31c (package/skeleton-init-systemd: work around for /var/lib
not populating), which states:

    It turns out /var/lib will exist, because some part of systemd creates
    /var/lib/systemd/catalog on boot before tmpfiles runs.

So, we do have a chicken-n-eggs issue, that registering /var as a whole
does not work.

I'll try to investigate further, and see if that's still the case.

> > -       done >$(TARGET_DIR)/etc/tmpfiles.d/var-factory.conf
> > +       done >$(TARGET_DIR)/usr/lib/tmpfiles.d/buildroot-factory.conf
> 
> How about naming this something like 00-buildroot-var.conf, so hopefully any
> *.conf that might expect something in /var existing is executed later.

I am totally OK for 00-buildroot-var.conf.

[--SNIP--]
> Regardless, the changes are an improvement, buildroot is the "distro"
> and its special sauce
> should reside inside /usr as much as possible.

Yes, that was my reasoning as well.

> Id highly recommend atleast changing the name to
> 00-buildroot-var.conf, other than that:

Agreed.

> Acked-by: Norbert Lange <nolange79@gmail.com>

Thanks!

Regards,
Yann E. MORIN.

> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Norbert Lange Nov. 7, 2022, 1:32 p.m. UTC | #3 
Am So., 6. Nov. 2022 um 16:58 Uhr schrieb Yann E. MORIN
<yann.morin.1998@free.fr>:
>
> Norbert, All,
>
> On 2022-11-06 16:40 +0100, Norbert Lange spake thusly:
> > Am Di., 18. Okt. 2022 um 21:43 Uhr schrieb <yann.morin@orange.com>:
> [--SNIP--]
> > > diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > > index 795a171809..7b66732ef4 100644
> > > --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > > +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > > @@ -29,7 +29,6 @@ else
> > >  # a real (but empty) directory, and the "factory files" will be copied
> > >  # back there by the tmpfiles.d mechanism.
> > >  define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW
> > > -       mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
> > >         echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab
> > >         echo "tmpfs /var tmpfs mode=1777 0 0" >>$(TARGET_DIR)/etc/fstab
> > >  endef
> > > @@ -38,6 +37,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> > >         rm -rf $(TARGET_DIR)/usr/share/factory/var
> > >         mv $(TARGET_DIR)/var $(TARGET_DIR)/usr/share/factory/var
> > >         mkdir -p $(TARGET_DIR)/var
> > > +       mkdir -p $(TARGET_DIR)/usr/lib/tmpfiles.d
> > >         for i in $(TARGET_DIR)/usr/share/factory/var/* \
> > >                  $(TARGET_DIR)/usr/share/factory/var/lib/* \
> > >                  $(TARGET_DIR)/usr/share/factory/var/lib/systemd/*; do \
> > > @@ -51,7 +51,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> > >                         printf "C! %s - - - -\n" "$${j}" \
> > >                         || exit 1; \
> > >                 fi; \
> >
> > From tmpfiles.d docs: "C ... Recursively copy a file or directory, if
> > the destination files or directories do not
> >   exist yet or the destination directory is empty. Note that this
> > command will not descend into subdirectories
> >   if the destination directory already exists and is not empty.
> > Instead, the entire copy operation is skipped."
> >
> > So all the drama could be just replaced with "C! /var - - - -\n" imho.
> >
> > (non standard permissions/owner are probably gone either way)
>
> I do remember that I had to explicitly register individual entries
> rather than the directory alone, as that did not work. That was 5+
> years ago (commit 26085bbbd500), and I did not write it down in the
> commit log...
>
> However, it looks like we hit this issue, as hinted in commit
> 7e811708f31c (package/skeleton-init-systemd: work around for /var/lib
> not populating), which states:
>
>     It turns out /var/lib will exist, because some part of systemd creates
>     /var/lib/systemd/catalog on boot before tmpfiles runs.
>
> So, we do have a chicken-n-eggs issue, that registering /var as a whole
> does not work.

I guess thats the logging service, systemd-tmpfiles-setup.service already
requires some stuff running and other stuff is able to run and freely access
/var aswell.

The clean way would be to be done with "migrating" /var before local-fs.target
is reached.

Regards, Norbert
Yann E. MORIN Dec. 21, 2022, 9:16 p.m. UTC | #4 
Yann, All,

On 2022-10-18 21:43 +0200, yann.morin@orange.com spake thusly:
> When the rootfs is not remounted read-write (thus assuming a read-only
> rootfs like squashfs), we create a tmpfiles.d factory for /var.
> 
> However, we register those in /etc/tmpfiles.d/, but /etc could also be
> a tmpfs (for full state-less systems, or easy factory-reset, see [0]).
> 
> So, we move our var factory to /usr/lib/tmpfiles.d/, which is also the
> location where systemd itself places its own tmpfiles, and where we
> already put all our other tmpfiles (see audit, avahi, cryptsetup, dhcp,
> lighttpd, nfs-utils, quagga, samba4, swupdate) and our handling of
> systemd's catalog files too. We also rename the file to a better name,
> so that it is obvious it is generated by us (systemd already installs a
> var.conf of its own, so we want to avoid name clashing).
> 
> Last little detail: there is no need or reason to create .../tmpfiles.d/
> at install time; it is only needed in the rootfs-pre-cmd hook, so we
> only create it just before we need it.
> 
> [0] http://0pointer.de/blog/projects/stateless.html
> 
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Norbert Lange <nolange79@gmail.com>
> Cc: Romain Naour <romain.naour@smile.fr>
> Cc: Jérémy Rosen <jeremy.rosen@smile.fr>
> [yann.morin.1998@free.fr:
>   - split original patch in two
>   - this one only moves out of /etc and into /usr/lib
>   - adapt commit log accordingly
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>

Applied to master with the renaming suggested by Norbert, thanks.

Regards,
Yann E. MORIN.

> ---
>  package/skeleton-init-systemd/skeleton-init-systemd.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> index 795a171809..7b66732ef4 100644
> --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> @@ -29,7 +29,6 @@ else
>  # a real (but empty) directory, and the "factory files" will be copied
>  # back there by the tmpfiles.d mechanism.
>  define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW
> -	mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
>  	echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab
>  	echo "tmpfs /var tmpfs mode=1777 0 0" >>$(TARGET_DIR)/etc/fstab
>  endef
> @@ -38,6 +37,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>  	rm -rf $(TARGET_DIR)/usr/share/factory/var
>  	mv $(TARGET_DIR)/var $(TARGET_DIR)/usr/share/factory/var
>  	mkdir -p $(TARGET_DIR)/var
> +	mkdir -p $(TARGET_DIR)/usr/lib/tmpfiles.d
>  	for i in $(TARGET_DIR)/usr/share/factory/var/* \
>  		 $(TARGET_DIR)/usr/share/factory/var/lib/* \
>  		 $(TARGET_DIR)/usr/share/factory/var/lib/systemd/*; do \
> @@ -51,7 +51,7 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>  			printf "C! %s - - - -\n" "$${j}" \
>  			|| exit 1; \
>  		fi; \
> -	done >$(TARGET_DIR)/etc/tmpfiles.d/var-factory.conf
> +	done >$(TARGET_DIR)/usr/lib/tmpfiles.d/buildroot-factory.conf
>  endef
>  SKELETON_INIT_SYSTEMD_ROOTFS_PRE_CMD_HOOKS += SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
>  
> -- 
> 2.25.1
> 
> 
> _________________________________________________________________________________________________________________________
> 
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
> 
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
diff mbox series

Patch

diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
index 795a171809..7b66732ef4 100644
--- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
+++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
@@ -29,7 +29,6 @@  else
 # a real (but empty) directory, and the "factory files" will be copied
 # back there by the tmpfiles.d mechanism.
 define SKELETON_INIT_SYSTEMD_ROOT_RO_OR_RW
-	mkdir -p $(TARGET_DIR)/etc/systemd/tmpfiles.d
 	echo "/dev/root / auto ro 0 1" >$(TARGET_DIR)/etc/fstab
 	echo "tmpfs /var tmpfs mode=1777 0 0" >>$(TARGET_DIR)/etc/fstab
 endef
@@ -38,6 +37,7 @@  define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
 	rm -rf $(TARGET_DIR)/usr/share/factory/var
 	mv $(TARGET_DIR)/var $(TARGET_DIR)/usr/share/factory/var
 	mkdir -p $(TARGET_DIR)/var
+	mkdir -p $(TARGET_DIR)/usr/lib/tmpfiles.d
 	for i in $(TARGET_DIR)/usr/share/factory/var/* \
 		 $(TARGET_DIR)/usr/share/factory/var/lib/* \
 		 $(TARGET_DIR)/usr/share/factory/var/lib/systemd/*; do \
@@ -51,7 +51,7 @@  define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
 			printf "C! %s - - - -\n" "$${j}" \
 			|| exit 1; \
 		fi; \
-	done >$(TARGET_DIR)/etc/tmpfiles.d/var-factory.conf
+	done >$(TARGET_DIR)/usr/lib/tmpfiles.d/buildroot-factory.conf
 endef
 SKELETON_INIT_SYSTEMD_ROOTFS_PRE_CMD_HOOKS += SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR