[systemd-devel] [feature request] allow instances in file.preset

[Zbigniew Jędrzejewski-Szmek]

On Thu, Sep 25, 2014 at 10:44:35PM +0000, Damien Robert wrote:
> Zbigniew Jędrzejewski-Szmek  wrote in message
> <20140925211702.GV29977 at in.waw.pl>:
> > This seems to be a mis-design. I'm pretty sure we should allow users
> > to set their own presets, so those directories underneath the home
> > dir should be added.
> 
> Ok great! I'll be happy to provide a patch but I have never hacked systemd
> before. Would something like that be ok? (not tested, just to see if I am
> in the right direction)
In general yes. Some specific notes below.

> ---------------------- >8 -------------------------
> From 7755e4afc3dc24f50c97c28fd7c00fd576d882cc Mon Sep 17 00:00:00 2001
> From: Damien Robert <damien.olivier.robert+git at gmail.com>
> Date: Fri, 26 Sep 2014 00:34:46 +0200
> Subject: [PATCH 1/1] preset: read files in $XDG_CONFIG_HOME/systemd/user-preset/*
> 
> This is the only way for a user to modify preset files as the other
> directory read
>        /run/systemd/user-preset/*.preset
>        /usr/lib/systemd/user-preset/*.preset
> are not user owned.
> 
> ---
>  man/systemd.preset.xml |  1 +
>  src/shared/install.c   | 24 ++++++++++++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/man/systemd.preset.xml b/man/systemd.preset.xml
> index 55cb4de..9d414f4 100644
> --- a/man/systemd.preset.xml
> +++ b/man/systemd.preset.xml
> @@ -49,6 +49,7 @@
>                  <para><filename>/etc/systemd/system-preset/*.preset</filename></para>
>                  <para><filename>/run/systemd/system-preset/*.preset</filename></para>
>                  <para><filename>/usr/lib/systemd/system-preset/*.preset</filename></para>
> +                <para><literallayout><filename>$XDG_CONFIG_HOME/systemd/user-preset/*</filename>
>                  <para><filename>/etc/systemd/user-preset/*.preset</filename></para>
>                  <para><filename>/run/systemd/user-preset/*.preset</filename></para>
>                  <para><filename>/usr/lib/systemd/user-preset/*.preset</filename></para>
You seem to open an xml element, without closing it.

> diff --git a/src/shared/install.c b/src/shared/install.c
> index 61e572b..7981556 100644
> --- a/src/shared/install.c
> +++ b/src/shared/install.c
> @@ -1769,6 +1769,7 @@ UnitFileState unit_file_get_state(
>  
>  int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char *name) {
>          _cleanup_strv_free_ char **files = NULL;
> +        _cleanup_free_ char *user_preset = NULL;
You can define this below, where it is used.

>          char **p;
>          int r;
>  
> @@ -1786,12 +1787,23 @@ int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char
>  #endif
>                                      NULL);
>          else if (scope == UNIT_FILE_GLOBAL)
> -                r = conf_files_list(&files, ".preset", root_dir,
> -                                    "/etc/systemd/user-preset",
> -                                    "/usr/local/lib/systemd/user-preset",
> -                                    "/usr/lib/systemd/user-preset",
> -                                    NULL);
> -        else
> +                if (user_config_home(&user_preset) >= 0) {
> +                        user_preset = strappend(user_preset, "-preset");
> +                        if (!user_preset)
> +                                return -ENOMEM;
> +                        r = conf_files_list(&files, ".preset", root_dir,
> +                                            user_preset,
> +                                            "/etc/systemd/user-preset",
> +                                            "/usr/local/lib/systemd/user-preset",
> +                                            "/usr/lib/systemd/user-preset",
> +                                            NULL);
> +                }
> +                else
> +                        r = conf_files_list(&files, ".preset", root_dir,
> +                                            "/etc/systemd/user-preset",
> +                                            "/usr/local/lib/systemd/user-preset",
> +                                            "/usr/lib/systemd/user-preset",
> +                                            NULL);
Please align all the if's at the same level, not nested.

Zbyszek