Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Thu, 09.10.14 07:21, Jan Synacek (jsyna...@redhat.com) wrote: 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()). What precisely are you proposing instead? What about extending usage of errno with systemd specific errors? Something like EUNITMASKED or Eanything systemd specific? Plus, implementing extended version of strerror(), which would use the standard stderror() for the standard errno? Well, using errno-style errors is certainly handy, but let's face it, it's not particularly descriptive. Before attempting to extending it we should probably look at other options, such as adopting sd_bus_error at more places, which allows more flexible identifiers as well as descriptive strings. Also, we return these errnos in many of our public APIs. We really should never return something there that cannot be made sense of externally, since people might (and should be able to) escalate the errors unmodified up the chain, where the immediate systemd context is lost or unknown. (Also, just think what would happen if multiple libraries would extend errno that way! how confusing would that get). So, my suggestion is to either accept the limited vocabulary of errno for this case (which I don't think is such a loss here), or to go for sd_bus_error, but some entirely new concept sounds less than ideal. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
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.
Logging about this with log_error() is probably not a good idea.
Also, the same logic should be added to unit_file_link() and
unit_file_preset() I figure.
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
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. 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.
--
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?
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
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.
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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
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
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Wed, 08.10.14 17:24, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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.
Well, true, but then again, it's not thaat much worse...
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()).
What precisely are you proposing instead?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Wed, Oct 08, 2014 at 09:28:39PM +0200, Lennart Poettering wrote:
On Wed, 08.10.14 17:24, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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.
Well, true, but then again, it's not thaat much worse...
Yeah, I guess now its time to do a proof-of-concept implementation to
see how it works in practice.
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()).
What precisely are you proposing instead?
typedef const name_error_mapping*
(*bus_error_mapping_lookup_t) (const char *str, size_t len);
int bus_error_add_mapping(bus_error_mapping_lookup_t mapping);
This could be used to register a custom function similar to our
bus_error_mapping_lookup(). It would get stored in a global table (of
fixed size?), and bus_error_mapping_lookup would be the first slot in
this table and the calls to bus_error_mapping_lookup would be
replace by a function which iterates over this table and
returns the first successful lookup.
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Wed, 08.10.14 23:07, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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()).
What precisely are you proposing instead?
typedef const name_error_mapping*
(*bus_error_mapping_lookup_t) (const char *str, size_t len);
int bus_error_add_mapping(bus_error_mapping_lookup_t mapping);
This could be used to register a custom function similar to our
bus_error_mapping_lookup(). It would get stored in a global table (of
fixed size?), and bus_error_mapping_lookup would be the first slot in
this table and the calls to bus_error_mapping_lookup would be
replace by a function which iterates over this table and
returns the first successful lookup.
I am really not a fan of global variables for things like this... If
it would be marked as TLS I'd feel a bit more comfortable though...
I wonder what it would take to make this work without any registration
code. i.e. we could use some executbale section tricks hidden in some
macros to allow any module to register mappings, and simply by being
compiled in we'd find them.
In such a scheme the registration race and scope issues should go
away, since everything is always registered in all threads.
To define a map array, in as many .c files as desired:
__attribute((__section__(errnomap))) __attribute((__used__)) const struct
struct my_errno_map[] = { };
And then, use this to find the section:
extern struct my_errno_map[] __start_errnomap;
extern struct my_errno_map[] __stop_errnomap;
gcc supposedly maps this automatically to the start and end of the
section we defined above (yes, __start_ and __stop_ is magic), and
then we can just enumerate through them. And any .c module that
defines more maps transparently is found.
Or something like this?
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Wed, Oct 08, 2014 at 11:23:51PM +0200, Lennart Poettering wrote:
On Wed, 08.10.14 23:07, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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()).
What precisely are you proposing instead?
typedef const name_error_mapping*
(*bus_error_mapping_lookup_t) (const char *str, size_t len);
int bus_error_add_mapping(bus_error_mapping_lookup_t mapping);
This could be used to register a custom function similar to our
bus_error_mapping_lookup(). It would get stored in a global table (of
fixed size?), and bus_error_mapping_lookup would be the first slot in
this table and the calls to bus_error_mapping_lookup would be
replace by a function which iterates over this table and
returns the first successful lookup.
I am really not a fan of global variables for things like this... If
it would be marked as TLS I'd feel a bit more comfortable though...
I wonder what it would take to make this work without any registration
code. i.e. we could use some executbale section tricks hidden in some
macros to allow any module to register mappings, and simply by being
compiled in we'd find them.
In such a scheme the registration race and scope issues should go
away, since everything is always registered in all threads.
To define a map array, in as many .c files as desired:
__attribute((__section__(errnomap))) __attribute((__used__)) const struct
struct my_errno_map[] = { };
And then, use this to find the section:
extern struct my_errno_map[] __start_errnomap;
extern struct my_errno_map[] __stop_errnomap;
gcc supposedly maps this automatically to the start and end of the
section we defined above (yes, __start_ and __stop_ is magic), and
then we can just enumerate through them. And any .c module that
defines more maps transparently is found.
That would be lovely. Last time I checked, a custom linker
script was also necessary, but this was a long time ago, so maybe
things have changed.
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
Lennart Poettering lenn...@poettering.net writes:
On Wed, 08.10.14 17:24, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
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.
Well, true, but then again, it's not thaat much worse...
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()).
What precisely are you proposing instead?
What about extending usage of errno with systemd specific errors?
Something like EUNITMASKED or Eanything systemd specific? Plus,
implementing extended version of strerror(), which would use the
standard stderror() for the standard errno?
--
Jan Synacek
Software Engineer, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core: don't allow enabling if unit is masked
---
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;
+}
+
r = install_info_add_auto(c, *i);
if (r 0)
return r;
--
1.9.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: don't allow enabling if unit is masked
On Tue, Oct 07, 2014 at 01:35:41PM +0200, Jan Synacek wrote: --- src/shared/install.c | 13 + 1 file changed, 13 insertions(+) Applied. Making the error messages better is another step that needs to be done. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
