On 22 April 2015 at 19:30, Lennart Poettering <lenn...@poettering.net> wrote: > On Sat, 21.02.15 02:38, Dimitri John Ledkov (dimitri.j.led...@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. _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel