[systemd-devel] [PATCH 3/3] core: add EnvironmentFiles to settable transient unit properties

[Lennart Poettering]

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

Sounds useful. 

> ---
>  src/core/dbus-execute.c          | 16 ++++++++++++++++
>  src/libsystemd/sd-bus/bus-util.c | 18 +++++++++++++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
> index 13b3d0d..1f1f602 100644
> --- a/src/core/dbus-execute.c
> +++ b/src/core/dbus-execute.c
> @@ -851,6 +851,22 @@ int bus_exec_context_set_transient_property(
>  
>                  return 1;
>  
> +        } else if (streq(name, "EnvironmentFiles")) {
> +
> +                _cleanup_strv_free_ char **l = NULL;
> +
> +                r = sd_bus_message_read_strv(message, &l);
> +                if (r < 0)
> +                        return r;
> +
> +                if (mode != UNIT_CHECK) {
> +                        r = strv_extend_strv(&c->environment_files, l);
> +                        if (r < 0)
> +                                return r;
> +                }
> +
> +                return 1;
> +

This should probably check whether the list only contains absolute file
names (possily prefixed with "-"), and refuse otherwise. It should
follow the rough behaviour of config_parse_unit_env_file().

This also needs to use unit_write_drop_in_private_format() or so, so
that the stuff is written to /run, so that it isn't lost on reload.

> +        else if (streq(field, "EnvironmentFiles")) {
> +                _cleanup_strv_free_ char **l = strv_split_quoted(eq);
> +                if(!l) return -ENOMEM;

Please avoid calling functions and declaring variables in the same line,
and also please write the if check in two lines...

> +
> +                r = sd_bus_message_open_container(m, 'v', "as");
> +                if (r < 0) {
> +                    return bus_log_create_error(r);
> +                }

No {} please if the if block only contains a single line.

Also 8ch indenting, please.

> +
> +                r = sd_bus_message_append_strv(m, l);
> +                if (r < 0) {
> +                    return bus_log_create_error(r);
> +                }

Same here.

> +
> +                r = sd_bus_message_close_container(m);
> +
> +        } else if (streq(field, "DeviceAllow")) {
>  
>                  if (isempty(eq))
>                          r = sd_bus_message_append(m, "v", "a(ss)", 0);

Looks good otherwise!

Thanks!

Lennart

-- 
Lennart Poettering, Red Hat