On Wed, Oct 08, 2014 at 05:02:31PM +0200, Lennart Poettering wrote: > On Wed, 08.10.14 15:29, Zbigniew Jędrzejewski-Szmek (zbys...@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 (jsyna...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel