[systemd-devel] [PATCH 2/2] systemd: enabling/disabling template units

[Lennart Poettering]

On Tue, 03.07.12 12:04, Michal Sekletar (msekleta at redhat.com) wrote:

Heya,

After a bit of roth and back I think this is an OK approach. I am a bit
concerned that we overload install with two different meanings here, but
I must admit that a number of people have asked for this, under this
name, so I guess it would be kinda natural. Also, the other options
(place this in "systemctl link" or come up with an entirely new name
doesn't sounds so good either...).

Anyway, before I merge this: could you update the man page of systemctl
for this? i.e. say that this does two different things for unit files
and instances.

One question: what happens if somebody installs a unit file that is
already named as an instance? In theory that should be handled like any
other unit file (though I think it is actually currently broken), rather
than as a request to instantiate it... It's a corner case to think
about? (though nothing to fix with your patch if it is currently broken
anyway...)

Also, please fix:

>  
>                          if (r == 0)
> @@ -284,9 +285,16 @@ static int remove_marked_symlinks_fd(
>                                  continue;
>                          }
>  
> -                        found =
> -                                set_get(remove_symlinks_to, dest) ||
> -                                set_get(remove_symlinks_to, path_get_file_name(dest));
> +                        if (unit_name_is_instance(p)) {
> +                                char *template =
> -                                path_get_file_name(p);

Why is this called 'template'? I mean, it's an instance, right, not the
template, or am I wrong? Also, might make sense to just fold this call
into the strv_contains() invocation below, as we do that for the
set_get() too.

> +
> +                                found = (strv_contains(files, template)) &&
> +                                        (set_get(remove_symlinks_to, dest) ||
> +                                         set_get(remove_symlinks_to, path_get_file_name(dest)));
> +                        } else
> +                                found = set_get(remove_symlinks_to, dest) ||
> +                                        set_get(remove_symlinks_to, path_get_file_name(dest));
> +

Could you shorten this a bit and remove the duplication? i.e. just
assign "found" as originally, but then add afterwards: 

if (unit_name_is_instance(p)) 
        found = found || strv_contains(files, path_get_file_name(p));

or someting like that?

> @@ -1090,6 +1099,43 @@ static int unit_file_search(
>                  if (!path)
>                          return -ENOMEM;
>  
> +                r = access(path, R_OK);

Hmm, I am not happy with always doing an access() first before we open()
the file. That has this "racy" feeling to it... Can we find a different
way here? so that that open() result is used here for all decisions?


> +bool unit_name_is_instance(const char *n) {
> +        const char *p;
> +
> +        assert(n);
> +
> +        if (!(p = strchr(n, '@')))
> +                return false;

For new code we try to avoid this:

 if (!(foo = bar())) { ...

and try to write this instead:

 foo = bar();
 if (!foo) { ...

Please follow the same style in your patch!

Otherwise looks good. Thanks for you work!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.