[systemd-devel] [PATCH 2/3] core: Put transient user units in XDG_RUNTIME_DIR instead of XDG_CONFIG_HOME.

[Lennart Poettering]

On Fri, 28.03.14 19:38, Hristo Venev (hristo at venev.name) wrote:

Also looks like the absolute right thing to do.

> They are temporary and should not clutter the configuration directory.
> ---
>  src/core/unit.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/src/core/unit.c b/src/core/unit.c
> index 153b79b..69971ba 100644
> --- a/src/core/unit.c
> +++ b/src/core/unit.c
> @@ -2955,11 +2955,18 @@ static int drop_in_file(Unit *u, UnitSetPropertiesMode mode, const char *name, c
>          if (u->manager->running_as == SYSTEMD_USER) {
>                  _cleanup_free_ char *c = NULL;
>  
> -                r = user_config_home(&c);
> -                if (r < 0)
> -                        return r;
> -                if (r == 0)
> -                        return -ENOENT;
> +                if (mode & UNIT_PERSISTENT && !u->transient)

Can you enclose the binary & check with (), to clarify, what's going on here?

> +                        r = user_config_home(&c);
> +                        if (r < 0)
> +                                return r;
> +                        if (r == 0)
> +                                return -ENOENT;
> +
> +                else {
> +                        c = strappend(getenv("XDG_RUNTIME_DIR"),
> -                "/systemd/user");

This needs error checkiing, getenv() might return NULL....

> +
> +                        if (!c) return -ENOMEM;

Please break this into two lines...

> +                }
>  
>                  p = strjoin(c, "/", u->id, ".d", NULL);
>          } else if (mode == UNIT_PERSISTENT && !u->transient)
> @@ -3086,8 +3093,6 @@ int unit_remove_drop_in(Unit *u, UnitSetPropertiesMode mode, const char *name) {
>  }
>  
>  int unit_make_transient(Unit *u) {
> -        int r;
> -
>          assert(u);
>  
>          u->load_state = UNIT_STUB;
> @@ -3098,19 +3103,12 @@ int unit_make_transient(Unit *u) {
>          u->fragment_path = NULL;
>  
>          if (u->manager->running_as == SYSTEMD_USER) {
> -                _cleanup_free_ char *c = NULL;
> -
> -                r = user_config_home(&c);
> -                if (r < 0)
> -                        return r;
> -                if (r == 0)
> -                        return -ENOENT;
> -
> -                u->fragment_path = strjoin(c, "/", u->id, NULL);
> +                _cleanup_free_ char *dir = strappend(getenv("XDG_RUNTIME_DIR"), "/systemd/user/");
> +                u->fragment_path = strappend(dir, u->id);
>                  if (!u->fragment_path)
>                          return -ENOMEM;

We try to avoid declaring variables and invoking functions in the same
line. Could you split this up into two?

It might be nicer to use strjoin() here, and just concatenate the full
thing in one step, and then use mkdir_parents() instead of mkdir_p() to
create the parent directory...

>  
> -                mkdir_p(c, 0755);
> +                mkdir_p(dir, 0755);
>          } else {
>                  u->fragment_path = strappend("/run/systemd/system/", u->id);
>                  if (!u->fragment_path)


Lennart

-- 
Lennart Poettering, Red Hat