[18/20] fs: add pre- and post-command hooks

In some cases, the directory structure we want in the filesystem is not
exactly what we have in target/

For example, when systemd is used on a read-only rootfs, /var must be a
tmpfs. However, we may have packages that install stuff in there, and
set important rights (via the permission-table). So, at build time, we
need /var to be a symlink to the remanent location (/usr/share/factory)
while at runtime we need /var to be a directory.

One option would have seen to have /var as a real directory even during
build time, and in a target-finalize hook, move everything out of there
and into the "factory" location. However, that's not possible because
it's too early: some packages may want to set ownership and/or acces
rights on directories or files in /var, and this is only done in the
fakeroot script, which is called only later during the assembling of the
filesystem images.

Also, there would have been no way to undo the tweak (i.e. we need to
restore the /var symlink so that subsequent builds continue to work) if
it were done as a target-finalize hook.

The only solution is to allow packages to register pre- and post-hooks
that are called right before and right after the rootfs commands are
executed, and inside in the fakeroot script.

We can however not re-use the BR2_ROOTFS_POST_FAKEROOT_SCRIPT feature
either because it is done before the filesystem command, but there is
nothing that is done after. Also, we don't want to add to, and modify a
user-supplied variable.

So, we introduce two new variables that packages can set to add the
commands they need to run to tweak the filesystem right at the last
moment.

Those hooks are not documented on-purpose; they are probably going to
only ever be used by systemd.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: Romain Naour <romain.naour@gmail.com>

---
Changes v2 -> v3:
  - add space between >> and $$   (Romain)
---
 fs/common.mk           | 4 ++++
 package/pkg-generic.mk | 4 ++++
 2 files changed, 8 insertions(+)

Hi Yann,

On 18-07-17 19:25, Yann E. MORIN wrote:
> In some cases, the directory structure we want in the filesystem is not
> exactly what we have in target/
> 
> For example, when systemd is used on a read-only rootfs, /var must be a
> tmpfs. However, we may have packages that install stuff in there, and
> set important rights (via the permission-table). So, at build time, we
> need /var to be a symlink to the remanent location (/usr/share/factory)
> while at runtime we need /var to be a directory.
> 
> One option would have seen to have /var as a real directory even during
> build time, and in a target-finalize hook, move everything out of there
> and into the "factory" location. However, that's not possible because
> it's too early: some packages may want to set ownership and/or acces
> rights on directories or files in /var, and this is only done in the
> fakeroot script, which is called only later during the assembling of the
> filesystem images.
> 
> Also, there would have been no way to undo the tweak (i.e. we need to
> restore the /var symlink so that subsequent builds continue to work) if
> it were done as a target-finalize hook.
> 
> The only solution is to allow packages to register pre- and post-hooks
> that are called right before and right after the rootfs commands are
> executed, and inside in the fakeroot script.
> 
> We can however not re-use the BR2_ROOTFS_POST_FAKEROOT_SCRIPT feature
> either because it is done before the filesystem command, but there is
> nothing that is done after. Also, we don't want to add to, and modify a
> user-supplied variable.

 I completely agree with this approach.

> 
> So, we introduce two new variables that packages can set to add the
> commands they need to run to tweak the filesystem right at the last
> moment.

 But not with this.


> Those hooks are not documented on-purpose; they are probably going to
> only ever be used by systemd.

 If systemd is the only one ever to use it, then the approach with a list of
hooks is just too much over the top. Why not hardcode it? Like so:


> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Reviewed-by: Romain Naour <romain.naour@gmail.com>
> 
> ---
> Changes v2 -> v3:
>   - add space between >> and $$   (Romain)
> ---
>  fs/common.mk           | 4 ++++
>  package/pkg-generic.mk | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 14e0a6b868..9a7758ff49 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -95,10 +95,14 @@ endif
>  	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
>  		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
>  		echo $$(s) $$(TARGET_DIR) $$(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $$(FAKEROOT_SCRIPT)$$(sep))
> +	$$(foreach hook,$$(ROOTFS_PRE_CMD_HOOKS),\
> +		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT))

	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)

 You may want to change the variable name, obviously.

 Regards,
 Arnout


[snip]

Arnout, All,

On 2017-07-23 15:42 +0200, Arnout Vandecappelle spake thusly:
> On 18-07-17 19:25, Yann E. MORIN wrote:
> > In some cases, the directory structure we want in the filesystem is not
> > exactly what we have in target/
> > 
> > For example, when systemd is used on a read-only rootfs, /var must be a
> > tmpfs. However, we may have packages that install stuff in there, and
> > set important rights (via the permission-table). So, at build time, we
> > need /var to be a symlink to the remanent location (/usr/share/factory)
> > while at runtime we need /var to be a directory.
> > 
> > One option would have seen to have /var as a real directory even during
> > build time, and in a target-finalize hook, move everything out of there
> > and into the "factory" location. However, that's not possible because
> > it's too early: some packages may want to set ownership and/or acces
> > rights on directories or files in /var, and this is only done in the
> > fakeroot script, which is called only later during the assembling of the
> > filesystem images.
> > 
> > Also, there would have been no way to undo the tweak (i.e. we need to
> > restore the /var symlink so that subsequent builds continue to work) if
> > it were done as a target-finalize hook.
> > 
> > The only solution is to allow packages to register pre- and post-hooks
> > that are called right before and right after the rootfs commands are
> > executed, and inside in the fakeroot script.
> > 
> > We can however not re-use the BR2_ROOTFS_POST_FAKEROOT_SCRIPT feature
> > either because it is done before the filesystem command, but there is
> > nothing that is done after. Also, we don't want to add to, and modify a
> > user-supplied variable.
> 
>  I completely agree with this approach.
> 
> > 
> > So, we introduce two new variables that packages can set to add the
> > commands they need to run to tweak the filesystem right at the last
> > moment.
> 
>  But not with this.
> 
> > Those hooks are not documented on-purpose; they are probably going to
> > only ever be used by systemd.
> 
>  If systemd is the only one ever to use it, then the approach with a list of
> hooks is just too much over the top. Why not hardcode it? Like so:
> 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Reviewed-by: Romain Naour <romain.naour@gmail.com>
> > 
> > ---
> > Changes v2 -> v3:
> >   - add space between >> and $$   (Romain)
> > ---
> >  fs/common.mk           | 4 ++++
> >  package/pkg-generic.mk | 4 ++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/fs/common.mk b/fs/common.mk
> > index 14e0a6b868..9a7758ff49 100644
> > --- a/fs/common.mk
> > +++ b/fs/common.mk
> > @@ -95,10 +95,14 @@ endif
> >  	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
> >  		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
> >  		echo $$(s) $$(TARGET_DIR) $$(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $$(FAKEROOT_SCRIPT)$$(sep))
> > +	$$(foreach hook,$$(ROOTFS_PRE_CMD_HOOKS),\
> > +		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT))
> 
> 	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)
> 
>  You may want to change the variable name, obviously.

And it also means that the declaration of ROOTFS_SKELETON_PRE_CMD_HOOK
be conditional in the skeleton-systemd package, like:

    ifeq ($(BR2_PACKAGE_SKELETON_SYSTEMD),y)
    define ROOTFS_SKELETON_PRE_CMD_HOOK
        blabla
    endef
    endif

But I do not like these kind of constructs in the packages themselves,
because they are too easy to miss...

Alternatively, I would rather move that logic into the filesystem code
itself, because after all that is its responsibility, and guard its use
with something like (with a proper variable name, of course):

    ifeq ($(ROOTFS_VAR_FACTORY_BLABLA),YES)
        blabla...
    endif

And then have the packages that need it just set:
    ROOTFS_VAR_FACTORY_BLABLA = YES

All that, because Thomas already suggested (last year in Toulouse) that
we could re-use the same factory mechanism for the other init systems as
well (on read-only rootfs). In this case, the code would then be guarded
by an test against an empty $(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),
because then there would be no relation to an init system at all.

But the tmpfiles format is very specific to systemd; we'd have to write
our own parser for that (not too difficult, I think, but it'd run on
the target so it'd have to be really dead-simple).

So in the end, I'd remove the hooks and move the code to the fs
handling, protected by a conditional that so far only systemd would set.

OK with this new approach?

Still, there is yet ine thing that I dislike in this solution, whether
it be with hooks or the variable, and that noone has seen so far: it is
not atomic and the target/ directory will be broken if the filesystem
command fails right in the middle, before the var symlink was restored.

I have seen no solution to this issue, except doing a copy of target
first, right before assembling the image(s), becasue in that case, we
wouldn't care about restoring the previous status, so we would no longer
need atomicity.

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> 
> [snip]
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

On 23-07-17 16:17, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2017-07-23 15:42 +0200, Arnout Vandecappelle spake thusly:
[snip]
>> 	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)
>>
>>  You may want to change the variable name, obviously.
> 
> And it also means that the declaration of ROOTFS_SKELETON_PRE_CMD_HOOK
> be conditional in the skeleton-systemd package, like:
> 
>     ifeq ($(BR2_PACKAGE_SKELETON_SYSTEMD),y)
>     define ROOTFS_SKELETON_PRE_CMD_HOOK
>         blabla
>     endef
>     endif
> 
> But I do not like these kind of constructs in the packages themselves,
> because they are too easy to miss...

 I'm not sure if it's that hard to miss. You just grep for
ROOTFS_SKELETON_PRE_CMD_HOOK to get the context.


> Alternatively, I would rather move that logic into the filesystem code
> itself, because after all that is its responsibility, and guard its use
> with something like (with a proper variable name, of course):
> 
>     ifeq ($(ROOTFS_VAR_FACTORY_BLABLA),YES)
>         blabla...
>     endif
> 
> And then have the packages that need it just set:
>     ROOTFS_VAR_FACTORY_BLABLA = YES

 I'm not so sure I like that better...


> All that, because Thomas already suggested (last year in Toulouse) that
> we could re-use the same factory mechanism for the other init systems as
> well (on read-only rootfs). In this case, the code would then be guarded
> by an test against an empty $(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),
> because then there would be no relation to an init system at all.

 You completely lost me here.

> But the tmpfiles format is very specific to systemd; we'd have to write
> our own parser for that (not too difficult, I think, but it'd run on
> the target so it'd have to be really dead-simple).
> 
> So in the end, I'd remove the hooks and move the code to the fs
> handling, protected by a conditional that so far only systemd would set.
> 
> OK with this new approach?

 I'm not so convinced. I mean, if you're putting systemd-specific code into
ROOTFS_TARGET_INTERNAL, you might just as well make that bit depend on
BR2_INIT_SYSTEMD directly. But then it becomes really too intertwined for my
liking. I think my ROOTFS_SKELETON_PRE_CMD_HOOK proposal is a better middle
ground between complexity and abstraction. If you really don't like that, I
prefer the original ROOTFS_PRE_CMD_HOOKS approach. I just think that giving the
possibility of defining more than one hook is useless. Particularly since the
systemd hook hinges on being the very last thing before rootfs creation.


> Still, there is yet ine thing that I dislike in this solution, whether
> it be with hooks or the variable, and that noone has seen so far: it is
> not atomic and the target/ directory will be broken if the filesystem
> command fails right in the middle, before the var symlink was restored.

 Can we create the symlink early in the build process, every time? I.e. run
SKELETON_SYSTEMD_POST_ROOTFS_VAR somehwere at the start of the build as well?
Only, I'm not sure where we could put it.


 Regards,
 Arnout


> I have seen no solution to this issue, except doing a copy of target
> first, right before assembling the image(s), becasue in that case, we
> wouldn't care about restoring the previous status, so we would no longer
> need atomicity.

On 2017-07-23 23:51 +0200, Arnout Vandecappelle spake thusly:
> 
> 
> On 23-07-17 16:17, Yann E. MORIN wrote:
> > Arnout, All,
> > 
> > On 2017-07-23 15:42 +0200, Arnout Vandecappelle spake thusly:
> [snip]
> >> 	$$(call PRINTF,$$(ROOTFS_SKELETON_PRE_CMD_HOOK)) >> $$(FAKEROOT_SCRIPT)
> >>
> >>  You may want to change the variable name, obviously.
> > 
> > And it also means that the declaration of ROOTFS_SKELETON_PRE_CMD_HOOK
> > be conditional in the skeleton-systemd package, like:
> > 
> >     ifeq ($(BR2_PACKAGE_SKELETON_SYSTEMD),y)
> >     define ROOTFS_SKELETON_PRE_CMD_HOOK
> >         blabla
> >     endef
> >     endif
> > 
> > But I do not like these kind of constructs in the packages themselves,
> > because they are too easy to miss...
> 
>  I'm not sure if it's that hard to miss. You just grep for
> ROOTFS_SKELETON_PRE_CMD_HOOK to get the context.
> 
> 
> > Alternatively, I would rather move that logic into the filesystem code
> > itself, because after all that is its responsibility, and guard its use
> > with something like (with a proper variable name, of course):
> > 
> >     ifeq ($(ROOTFS_VAR_FACTORY_BLABLA),YES)
> >         blabla...
> >     endif
> > 
> > And then have the packages that need it just set:
> >     ROOTFS_VAR_FACTORY_BLABLA = YES
> 
>  I'm not so sure I like that better...

I'm not sure either, but I'll further explain below, so you get a better
picture than the blurred one I posted yesterday...

> > All that, because Thomas already suggested (last year in Toulouse) that
> > we could re-use the same factory mechanism for the other init systems as
> > well (on read-only rootfs). In this case, the code would then be guarded
> > by an test against an empty $(BR2_TARGET_GENERIC_REMOUNT_ROOTFS_RW),
> > because then there would be no relation to an init system at all.
> 
>  You completely lost me here.

Ah...

So, the factory is something that is indeed specific to systemd.

However, it allows for a systemd-based rootfs to start with a known
first-boot state, that is put in place early, before
applications/services need them.

IWith this patch, it means that every boot if a first-boot, but at least
the content of /var is what packages isntalled, and they know it is
writable.

But with the other init systems (busybox, syysvinit) we do not have a
factory. Instead /var contains only symlinks to /tmp, which is clean on
boot. So packages won't find what they need in /var, either:
  - it is redirected to /tmp, in which case it is empty, or
  - it is not redirected to /tmp, and it is read-only.

So Thomas suggested that we apply the factory feature to all init
systems. With systemd, it would be built-in (by mean of tmpfiles),
while for the others we'd need some extra startup scripts to mount a
tmpfs on /var and populate it.

> > But the tmpfiles format is very specific to systemd; we'd have to write
> > our own parser for that (not too difficult, I think, but it'd run on
> > the target so it'd have to be really dead-simple).
> > 
> > So in the end, I'd remove the hooks and move the code to the fs
> > handling, protected by a conditional that so far only systemd would set.
> > 
> > OK with this new approach?
> 
>  I'm not so convinced. I mean, if you're putting systemd-specific code into
> ROOTFS_TARGET_INTERNAL, you might just as well make that bit depend on
> BR2_INIT_SYSTEMD directly.

But then it is no longer systemd-specific, see above.

> But then it becomes really too intertwined for my
> liking. I think my ROOTFS_SKELETON_PRE_CMD_HOOK proposal is a better middle
> ground between complexity and abstraction. If you really don't like that, I
> prefer the original ROOTFS_PRE_CMD_HOOKS approach. I just think that giving the
> possibility of defining more than one hook is useless. Particularly since the
> systemd hook hinges on being the very last thing before rootfs creation.
> 
> 
> > Still, there is yet ine thing that I dislike in this solution, whether
> > it be with hooks or the variable, and that noone has seen so far: it is
> > not atomic and the target/ directory will be broken if the filesystem
> > command fails right in the middle, before the var symlink was restored.
> 
>  Can we create the symlink early in the build process, every time? I.e. run
> SKELETON_SYSTEMD_POST_ROOTFS_VAR somehwere at the start of the build as well?
> Only, I'm not sure where we could put it.

Hmm... Good point. I'll try to find a place. But I'm not sure that would
be possible.

Maybe with the step-hooks, that are called very early for all steps, and
then we'd use that to create the symlink if missing. But I do not like
that too much...

I'll investigate.

In the meantime, I'll drop the read-only patches from the series, to
avoid too much noise for now, and so we concentrate on the skeleton
stuff for this release.

Thanks!

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> 
> > I have seen no solution to this issue, except doing a copy of target
> > first, right before assembling the image(s), becasue in that case, we
> > wouldn't care about restoring the previous status, so we would no longer
> > need atomicity.
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

On 24-07-17 18:01, Yann E. MORIN wrote:
> On 2017-07-23 23:51 +0200, Arnout Vandecappelle spake thusly:

[snip]
> So Thomas suggested that we apply the factory feature to all init
> systems. With systemd, it would be built-in (by mean of tmpfiles),
> while for the others we'd need some extra startup scripts to mount a
> tmpfs on /var and populate it.

 Hum-hum, I thought that the Buildroot rule was: we don't add features. If
someone needs the factory feature, they should use systemd (or s6 I believe also
has some kind of factory feature).

[snip]
>>  Can we create the symlink early in the build process, every time? I.e. run
>> SKELETON_SYSTEMD_POST_ROOTFS_VAR somehwere at the start of the build as well?
>> Only, I'm not sure where we could put it.
> 
> Hmm... Good point. I'll try to find a place. But I'm not sure that would
> be possible.

 "dependencies" is an option, but that's *really* dirty.

 Perhaps we can add a stamp file to TARGET_DIR/usr/share/factory, and then add
TARGET_DIR/var/stampfile as a dependency of dirs. If TARGET_DIR/var is an
(empty) directory, then TARGET_DIR/var/stampfile will not exist so the rule will
trigger.

 But there are a few problems with that:

- dirs is also "called" for legal-info, but legal-info doesn't otherwise create
a target directory;

- it would be natural for this stampfile to depend on skeleton-systemd (which
can then create the stampfile), but that would give a circular dependency.


> Maybe with the step-hooks, that are called very early for all steps, and
> then we'd use that to create the symlink if missing. But I do not like
> that too much...

 Meh, no, that would repeat the check for *every* package...


> I'll investigate.
> 
> In the meantime, I'll drop the read-only patches from the series, to
> avoid too much noise for now, and so we concentrate on the skeleton
> stuff for this release.

 I think it's better if you still include them:

- it doesn't hurt if they are part of the series but don't get applied;
- it gives some context about the direction in which we are going;
- it gives an opportunity to continue discussion on these patches.

 Even if you re-post with some changes NOT applied, just say so in the commit
message. Perhaps remove your Sob to make sure it doesn't get applied by
accident. Or remove it from patchwork.


 Regards,
 Arnout