[systemd-devel] [PATCH 0/2] units: add and use ConditionInitrd= instead of checking for /etc/initrd-release.

[Lennart Poettering]

On Wed, 27.08.14 22:48, Thomas Bächler (thomas at archlinux.org) wrote:

> Am 27.08.2014 um 20:25 schrieb Ivan Shapovalov:
> > On Wednesday 27 August 2014 at 20:19:45, Lennart Poettering wrote:	
> >> On Wed, 27.08.14 20:26, Ivan Shapovalov (intelfx100 at gmail.com) wrote:
> >>
> >>> This is as proposed by Thomas in review of my hibernate-resume patchset.
> >>>
> >>> The objective benefit of this change is that in_initrd() function is used
> >>> for checking, which not only checks for /etc/initrd-release, but also verifies
> >>> that the rootfs is on a virtual device.
> >>
> >> If we add a new condition then I want to hear a strong case for it. I
> >> mean, what's wrong with checking for initrd-release? Why would that not
> >> suffice?
> 
> Whether or not we are in initrd is what we want to check. The existence
> of /etc/initrd-release is an implementation detail. Having the same file
> check as a condition in units duplicates the code that has been
> implemented in the in_initrd() function.

Well, in_initrd() previously did only the exacty same check, as the
condition... We added the tmpfs/ramfs check only as extra safety net,
because dracut once upon a time ended up copying the initrd-release file
into the host os and disaster ensued...

Note that the ConditionXYZ stuff in most cases is just supposed to be
optimization stuff, which allows us to skip execution of things if
there's no point to bother...

I think you do have a point though, and it would be good to use the full
in_initrd() check here, but maybe the lesson to take here is to simply
do that inside of the resume tool, rather than add a new condition for
it... That way, the condition is just an optimization, but the safety
net is inside the tool itself...

I have now commited a patch that adds this. (the generator actually
already had it...)

> In addition, someone who writes unit files should not be forced to know
> these implementation details, but should have the proper condition
> documented for the purpose.

No, /etc/initrd-release is really supposed to be API (we probably should
put together a man page for it, to make that clear). It's something an
initrd implementor has to write, and then systemd can make use of it,
but where systemd might just be one consumer of...

Lennart

-- 
Lennart Poettering, Red Hat