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

[Zbigniew Jędrzejewski-Szmek]

On Wed, Apr 23, 2014 at 09:12:15PM +0200, Lennart Poettering wrote:
> On Fri, 28.03.14 19:38, Hristo Venev (hristo at venev.name) wrote:
> 
> Sounds useful. 

The other two patches were redone and applied (1/3 by Kay in June,
2/3 by Steven Allen now). Do you intend to clean up and resubmit
this one?

Zbyszek

> 
> > ---
> >  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
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>