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

[Lennart Poettering]

On Sat, 21.02.15 02:38, Dimitri John Ledkov (dimitri.j.ledkov at intel.com) wrote:

Sorry for the late review!

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

> ---
>  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.

> +#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.

> +
>  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....

> +                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...

> 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!

Lennart

-- 
Lennart Poettering, Red Hat