[systemd-devel] [PATCH] core: don't allow enabling if unit is masked

[Lennart Poettering]

On Wed, 08.10.14 15:29, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:

> On Wed, Oct 08, 2014 at 11:54:19AM +0200, Lennart Poettering wrote:
> > On Tue, 07.10.14 13:35, Jan Synacek (jsynacek at redhat.com) wrote:
> > 
> > > ---
> > >  src/shared/install.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/src/shared/install.c b/src/shared/install.c
> > > index fa064c2..945bb27 100644
> > > --- a/src/shared/install.c
> > > +++ b/src/shared/install.c
> > > @@ -1516,6 +1516,19 @@ int unit_file_enable(
> > >                  return r;
> > >  
> > >          STRV_FOREACH(i, files) {
> > > +                UnitFileState state;
> > > +
> > > +                state = unit_file_get_state(scope, root_dir, *i);
> > > +                if (state < 0) {
> > > +                        log_error("Failed to get unit file state for %s: %s", *i, strerror(-state));
> > > +                        return state;
> > > +                }
> > > +
> > > +                if (state == UNIT_FILE_MASKED || state == UNIT_FILE_MASKED_RUNTIME) {
> > > +                        log_error("Failed to enable unit: Unit %s is masked", *i);
> > > +                        return -ENOTSUP;
> > > +                }
> > > +
> > 
> > Looks mostly OK. However, we should probably use a more useful error
> > here. Maybe EADDRNOTAVAIL or so. Even better though would be to
> > actually change the call to take an sd_bus_error argument and then
> > return a proper error message that can be passed back to the bus
> > clients with a real explanation.
> Yes, EADDRNOTAVAIL is used in another place where the unit is masked.
> I'm not sure which one is more "useful" :). But hopefully this will be
> an internal-only thing once meaningful dbus error messages are provided.
> 
> The problem with passing dbus *error into the lower levels is that
> the functions live in src/share/, and were so far not linked with the
> bus libraries. 

Hmm, yeah, and if we did it would be cyclic and stuff...

> I think that the best way to handle this would be to
> use a temporary structure like
>    { char *unit_name; char *error_message; int code}
> and use this to pass the information about the error from the lower
> to the upper levels. But maybe I'm overcomplicating things.

Hmm, maybe a simply solution would be to convert EADDRNOTAVAIL into a
proper sd_bus_error on the calling side, that shouldn't be too
difficult.

> A related thing: there's a mapping bus-error <-> errno implemented,
> but it only works for the errors defined in the library itself. It
> would be nice to extend this mapping to the "user" defined errors,
> e.g. in core/.  Would you be amenable to adding a mechianism to
> register additional mappings like bus-error-mapping.gperf so that they
> are used by the library?

Maybe have internal versions of the conversion calls that allow
passing in an additional table?

Lennart

-- 
Lennart Poettering, Red Hat