[systemd-devel] [PATCH v2] Add support for transient presets, applied on every boot.

[Dimitri John Ledkov]

On 22 April 2015 at 19:30, Lennart Poettering <lennart at poettering.net> wrote:
> On Sat, 21.02.15 02:38, Dimitri John Ledkov (dimitri.j.ledkov at intel.com) wrote:
>
> Sorry for the late review!
>

Hello, blast from the past =)))))

> Can you please add a commit description to this, explaining the
> precise rationale for this?
>

Right, will work on that. But let me comment on below to close up
discussion here, before moving next edition of the patchset into
github.


>> ---
>>  src/core/main.c      | 27 +++++++++++++++++++++++++++
>>  src/core/unit.c      |  2 +-
>>  src/shared/install.c | 25 ++++++++++++++++++++-----
>>  src/shared/install.h |  2 +-
>>  4 files changed, 49 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/core/main.c b/src/core/main.c
>> index 08f46f5..2656779 100644
>> --- a/src/core/main.c
>> +++ b/src/core/main.c
>> @@ -1207,6 +1207,23 @@ static int write_container_id(void) {
>>          return write_string_file("/run/systemd/container", c);
>>  }
>>
>> +static int transient_presets(void) {
>> +        struct stat st;
>> +
>> +        if (lstat("/usr/lib/systemd/system-preset-transient", &st) == 0)
>> +                return !!S_ISDIR(st.st_mode);
>
> Please use is_dir() for this, it's slightly nicer to read.
>

ok.

>> +#ifdef HAVE_SPLIT_USR
>> +        if (lstat("/lib/systemd/system-preset-transient", &st) == 0)
>> +                return !!S_ISDIR(st.st_mode);
>> +#endif
>> +
>> +        if (lstat("/etc/systemd/system-preset-transient", &st) == 0)
>> +                return !!S_ISDIR(st.st_mode);
>> +
>> +        return 0;
>> +}
>
> Also, the function should probably return a proper "bool" instead of
> an int. We use C99 bool heavily.
>
> That said, maybe we shouldn't have this function at all, see below.
>

Well, I think we need this. Either here, or in unit_file_preset_all.

>> +
>>  int main(int argc, char *argv[]) {
>>          Manager *m = NULL;
>>          int r, retval = EXIT_FAILURE;
>> @@ -1619,6 +1636,16 @@ int main(int argc, char *argv[]) {
>>          if (arg_running_as == SYSTEMD_SYSTEM) {
>>                  bump_rlimit_nofile(&saved_rlimit_nofile);
>>
>> +                // NB! transient presets must be applied before
>> normal
>
> We try to stick to /* comments */ instead of // comments....
>

Ok. I grew up with // comments =)

>> +                if (transient_presets()) {
>> +                        r = unit_file_preset_all(UNIT_FILE_SYSTEM, true, NULL, UNIT_FILE_PRESET_ENABLE_ONLY, false, NULL, 0);
>> +                        if (r < 0)
>> +                                log_warning_errno(r, "Failed to populate transient preset unit settings, ignoring: %m");
>> +                        else
>> +                                log_info("Populated transient preset unit settings.");
>> +                }
>
> Hmm, do we actually need the explicit check with transient_presets()
> at all? I mean, it replicates the search path logic, and
> unit_file_preset_all() should notice on its own that there are no
> preset files in those dirs...
>

Well. it does notice there are no presets, and hence defaults to
enabling all units. At the moment the chain of calls is like so:

main.c decides to call unit_file_preset_all at an appropriate time:

1) list of all unit paths is constructed, and iterated
2) for each valid unit in a unit path presets are queried
3) a list of preset paths is constructed
4) for each valid preset file, it is iterated to check if it has
anything about the unit in question
5) ... and if nothing found in preset files default to enable.

Thus the currently logic does a lot of iterations and without any
.preset files or folders, defaults to "enable *".

Now, remember we discussed ways to optimise the preset application
logic. (parse and cache the policy first, and then for each unit do
lookups in the parsed policy). Reopening that discussion, would the
behaviour be acceptable to change slitghly:

1) if there are no .preset policy files defined at all. Bail out,
nothing is enabled.
2) if there are /usr/lib/systemd/preset/*.preset. Parse and create
policy cache, with the fall-through to "enable *" as before.
3) do similar for the transient-presets.

In general one would not use both types. And imho, it would be useful
to skip "enable *" if no policies exist at all. Mostly, because
without any policies installed (even the default one that systemd
ships) it is harmful to "enable *". E.g. clearlinux, debian, ubuntu do
not use persistent presets and end up plugging the preset application
on first boot with hacks (e.g. making sure machine-id is always
available pre-first boot, shipping "disable *" persistent policy, or
in clearlinux case even patching out persistent policy application
since it takes time to iterate pointlessly all preset dirs, for each
unit file).

The changes above would optimise policy application, and would be no
change to behaviour of existing users, who have at least one .preset
file on disk.

(if .preset file presence not enough, could do "journald" style
presence check of any /preset/ folders instead).



>> diff --git a/src/shared/install.c b/src/shared/install.c
>> index 65f1c24..7372e56 100644
>> --- a/src/shared/install.c
>> +++ b/src/shared/install.c
>> @@ -1873,7 +1873,7 @@ UnitFileState unit_file_get_state(
>>          return r < 0 ? r : state;
>>  }
>>
>> -int unit_file_query_preset(UnitFileScope scope, const char
>> -*root_dir, const char *name) {
>> +int unit_file_query_preset(UnitFileScope scope, bool runtime, const
>> -char *root_dir, const char *name) {
>
> Hmm, "runtime", or "transient"? Pick one!
>

transient, transient, missed a rename.

> Lennart
>
> --
> Lennart Poettering, Red Hat



-- 
Regards,

Dimitri.
Pura Vida!

https://clearlinux.org
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.