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

[Zbigniew Jędrzejewski-Szmek]

On Wed, Oct 08, 2014 at 05:02:31PM +0200, Lennart Poettering wrote:
> 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.

You can convert to an error, sure, but it is really nice to deliver
a specific message like "Unit boo.service is masked", instead of
"A unit is masked".
 
> > 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?
That is not as convenient. E.g. sd_bus_error_set
internally calls bus_error_name_to_errno. Currently, this always
returns EIO for errors unknown to the library, and then the caller
does it own lookup (e.g. looking at transaction_add_job_and_dependencies()).

Zbyszek