Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
Lennart Poettering píše v St 08. 08. 2012 v 19:26 +0200: On Mon, 06.08.12 17:16, Lukáš Nykrýn (lnyk...@redhat.com) wrote: Again thanks for review. Here is modified patch. If you think that it would be better to add signal stuff before accepting this patch I will not disagree. +r = set_put(*set, INT_TO_PTR(val)); Hmm, so I was about to merge this, but there is a problem here: exit code 0 is added to the set as NULL, and that's what we return if something is *not* in the set. So we'll always mishandle exit code 0 like this. We probably need some code here that just adds one to all exit codes, so that we can safely distuingish exit code 0 from not in this set. And that probably means the parsing function needs to be renamed a bit, to include plus_one or so... Lennart Sorry for delay. I have rewrote the patch and added posibility to set successful return statuses and now you can also specify signals. Lukas From 65bc722b758d2906be8e811979eee9a5e0640e28 Mon Sep 17 00:00:00 2001 From: Lukas Nykryn lnyk...@redhat.com Date: Mon, 13 Aug 2012 13:58:01 +0200 Subject: [PATCH] service: add options RestartPreventExitStatus and SuccessfulExitStatus In some cases, like wrong configuration, restarting after error does not help, so administrator can specify statuses by RestartPreventExitStatus which will not cause restart of a service. Sometimes you have non-standart exit status, so this can be specified by SuccessfulExitStatus. --- man/systemd.service.xml | 14 +++ src/core/load-fragment-gperf.gperf.m4 |2 + src/core/mount.c |2 +- src/core/service.c| 20 +- src/core/service.h|3 + src/core/socket.c |2 +- src/core/swap.c |2 +- src/remount-fs/remount-fs.c |2 +- src/shared/conf-parser.c | 69 + src/shared/conf-parser.h |1 + src/shared/exit-status.c | 16 +-- src/shared/exit-status.h | 11 - src/shared/hashmap.c | 15 +++ src/shared/hashmap.h |1 + src/shared/set.c |4 ++ src/shared/set.h |1 + src/shared/util.c |2 +- src/systemctl/systemctl.c |2 +- 18 files changed, 153 insertions(+), 16 deletions(-) diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 1946d85..c587847 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -580,6 +580,20 @@ /varlistentry varlistentry +termvarnameRestartPreventExitStatus=/varname/term +listitemparaSpecify exit status list, which +will prevent service from restart. Codes are +separated by whitespace (e.g. 1 6 SIGKILL)./para/listitem +/varlistentry + +varlistentry +termvarnameSuccessfulExitStatus=/varname/term +listitemparaSpecify exit status list, which +will be considered as successful exit. Codes are +separated by whitespace (e.g. 1 6 SIGKILL)./para/listitem +/varlistentry + +varlistentry termvarnamePermissionsStartOnly=/varname/term listitemparaTakes a boolean argument. If true, the permission diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index e738213..0674fe6 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -158,6 +158,8 @@ Service.PermissionsStartOnly,config_parse_bool, 0, Service.RootDirectoryStartOnly, config_parse_bool, 0, offsetof(Service, root_directory_start_only) Service.RemainAfterExit, config_parse_bool, 0, offsetof(Service, remain_after_exit) Service.GuessMainPID,config_parse_bool, 0, offsetof(Service, guess_main_pid) +Service.RestartPreventExitStatus, config_parse_set_status, 0, offsetof(Service, restart_ignore_status) +Service.SuccessfulExitStatus,config_parse_set_status,0, offsetof(Service, successful_status) m4_ifdef(`HAVE_SYSV_COMPAT', `Service.SysVStartPriority, config_parse_sysv_priority, 0, offsetof(Service, sysv_start_priority)', `Service.SysVStartPriority, config_parse_warn_compat,
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Wed, 08.08.12 21:14, Jóhann B. Guðmundsson (johan...@gmail.com) wrote: On 08/08/2012 05:30 PM, Lennart Poettering wrote: On Tue, 07.08.12 16:01, David Strauss (da...@davidstrauss.net) wrote: On Sat, Aug 4, 2012 at 4:46 AM, Lennart Poettering lenn...@poettering.net wrote: (But please, don't implement this bit just yet, let's wait for somebody actually needing this. Note though, that Upstart actually does have functionality like this). There are broken daemons like the cluster support in node.js that return non-zero on clean shutdowns. While this is broken behavior, it seems the systemd standard is to support broken/non-standard behavior with optional settings. (That is, the default in systemd is a well-behaved application.) Hmm, I was kinda waiting for use cases like this. i.e. before we add an option to reconfigure what clean exits are I wanted an explicit request for it, so that we don't end up adding something that is actually unncessary. Added this to the TODO list now. Should not this be fixed in the daemon instead of worked around in systemd? Yes, it should. Generally that's the rule we try to follow, but I think in this case it is OK to make an exception, simply because it's half a valid usecase to return other things than just errors with the exit code, even though I think it's shady... Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, 06.08.12 17:16, Lukáš Nykrýn (lnyk...@redhat.com) wrote: Again thanks for review. Here is modified patch. If you think that it would be better to add signal stuff before accepting this patch I will not disagree. +r = set_put(*set, INT_TO_PTR(val)); Hmm, so I was about to merge this, but there is a problem here: exit code 0 is added to the set as NULL, and that's what we return if something is *not* in the set. So we'll always mishandle exit code 0 like this. We probably need some code here that just adds one to all exit codes, so that we can safely distuingish exit code 0 from not in this set. And that probably means the parsing function needs to be renamed a bit, to include plus_one or so... Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On 08/08/2012 05:30 PM, Lennart Poettering wrote: On Tue, 07.08.12 16:01, David Strauss (da...@davidstrauss.net) wrote: On Sat, Aug 4, 2012 at 4:46 AM, Lennart Poettering lenn...@poettering.net wrote: (But please, don't implement this bit just yet, let's wait for somebody actually needing this. Note though, that Upstart actually does have functionality like this). There are broken daemons like the cluster support in node.js that return non-zero on clean shutdowns. While this is broken behavior, it seems the systemd standard is to support broken/non-standard behavior with optional settings. (That is, the default in systemd is a well-behaved application.) Hmm, I was kinda waiting for use cases like this. i.e. before we add an option to reconfigure what clean exits are I wanted an explicit request for it, so that we don't end up adding something that is actually unncessary. Added this to the TODO list now. Should not this be fixed in the daemon instead of worked around in systemd? JBG ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On 08/08/2012 11:14 PM, Jóhann B. Guðmundsson wrote: On 08/08/2012 05:30 PM, Lennart Poettering wrote: On Tue, 07.08.12 16:01, David Strauss (da...@davidstrauss.net) wrote: On Sat, Aug 4, 2012 at 4:46 AM, Lennart Poettering lenn...@poettering.net wrote: (But please, don't implement this bit just yet, let's wait for somebody actually needing this. Note though, that Upstart actually does have functionality like this). There are broken daemons like the cluster support in node.js that return non-zero on clean shutdowns. While this is broken behavior, it seems the systemd standard is to support broken/non-standard behavior with optional settings. (That is, the default in systemd is a well-behaved application.) Hmm, I was kinda waiting for use cases like this. i.e. before we add an option to reconfigure what clean exits are I wanted an explicit request for it, so that we don't end up adding something that is actually unncessary. Added this to the TODO list now. Should not this be fixed in the daemon instead of worked around in systemd? In principle yes, but some of the daemons that are broken are pretty heavy-weight: sshd [1], java [2]. [1] http://lists.freedesktop.org/archives/systemd-devel/2011-March/001656.html [2] http://lists.freedesktop.org/archives/systemd-devel/2011-December/004069.html I'm all for fixing things upstream, but sometimes it's hard. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Sat, Aug 4, 2012 at 4:46 AM, Lennart Poettering lenn...@poettering.net wrote: (But please, don't implement this bit just yet, let's wait for somebody actually needing this. Note though, that Upstart actually does have functionality like this). There are broken daemons like the cluster support in node.js that return non-zero on clean shutdowns. While this is broken behavior, it seems the systemd standard is to support broken/non-standard behavior with optional settings. (That is, the default in systemd is a well-behaved application.) I believe any specification of a service's normal exit values ought to be orthogonal to restart behavior. This is because a clean exit value affects more than restarts. If I do a systemctl stop on a node.js cluster application and it returns 1, I want that treated as a clean exit. Allowing specification of clean exit values is key to systemctl --failed having useful output in some situations. -- David Strauss | da...@davidstrauss.net ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
2012/8/6 Colin Guthrie gm...@colin.guthr.ie: 'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. fwiw, upstart calls that configuration option normal exit [1] Michael [1] http://upstart.ubuntu.com/cookbook/#normal-exit -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, Aug 6, 2012 at 2:14 AM, Colin Guthrie gm...@colin.guthr.ie wrote: 'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. We have: Restart=on-success | on-failure | ... Does this setting really binds to the Restart logic only? How about: ExitStatusFailure= ExitStatusSuccess= which matches the on-* values in Restart=, but also the C constants for exit(), and we could possibly re-use these lists in other conditions? Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, 06.08.12 01:14, Colin Guthrie (gm...@colin.guthr.ie) wrote: 'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. Hmm, using the word inhibit in this context might be confusing, since we already have this suspend/shutdown inibition framework and tools like systemd-inhibit, which are unrelated to this. But what about Prevent? PreventRestartExitStatus= makes a lot of sense to me, but then again, I am not a native speaker... Does that sound good to you, too? Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, 06.08.12 12:06, Kay Sievers (k...@vrfy.org) wrote: On Mon, Aug 6, 2012 at 2:14 AM, Colin Guthrie gm...@colin.guthr.ie wrote: 'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. We have: Restart=on-success | on-failure | ... Does this setting really binds to the Restart logic only? Yes it does. PreventRestartExitStatus= would configure something that is orthogonal to failure/success. How about: ExitStatusFailure= ExitStatusSuccess= I'd claim that failure/success should be orthogonal to the restart logic, and furthermore that success and failure should be parititions of the exit status set, i.e. all exits that are not failure are success and all that are success are not failure, and there is nothing but success/failure in the set. Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, Aug 6, 2012 at 1:12 PM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 06.08.12 01:14, Colin Guthrie (gm...@colin.guthr.ie) wrote: 'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. Hmm, using the word inhibit in this context might be confusing, since we already have this suspend/shutdown inibition framework and tools like systemd-inhibit, which are unrelated to this. But what about Prevent? PreventRestartExitStatus= makes a lot of sense to me, but then again, I am not a native speaker... Does that sound good to you, too? Starting key names with verbs or attributes is a mess. If it's really bound to Restart, it should probably be RestartIgnoreExitStatus or something like that instead. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, Aug 6, 2012 at 1:16 PM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 06.08.12 12:06, Kay Sievers (k...@vrfy.org) wrote: Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. We have: Restart=on-success | on-failure | ... Does this setting really binds to the Restart logic only? Yes it does. PreventRestartExitStatus= would configure something that is orthogonal to failure/success. That's why we have on-failure and on-success in the restart logic, right? :) Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
Lennart Poettering píše v Pá 03. 08. 2012 v 20:46 +0200:
On Tue, 24.07.12 16:43, Lukas Nykryn (lnyk...@redhat.com) wrote:
In some cases, like wrong configuration, restarting after error
exit code does not help, so administrator can specify RestartIgnoreCodes
which will not cause restart of a service.
I am not happy with the term Code here, it's a bit too generic to be
self explanatory.
And I think I agree with Zbigniew to a certain degree: we might want to
think about whether we should also allow configuring non-failure exit
codes or so. I mean, I am not suggesting we should implement that
right-away, but just keep in mind how that setting would look like so
that we can keep things uniform if we add it later. (In fact, I'd
recommend not to implement it right away: adding an option nobody so far
needed seems like a bad idea if we want to keep our set of options
minimal).
Also, I think we need to think further than just exit codes,
i.e. signals and stuff.
Maybe DontRestartExitStatus=? The libc calls the generalization of
exit code and exit signal the exit status, so that sounds like the
best term to use here.
And then people could write:
DontRestartExitStatus=SIGTERM 1 2 3 4
or something like this?
(Of course, the don't in the name is a negative option, which we try
to avoid, but it appears weird to have a positive option for this, so i
think it would be ok...)
One day, if we really need to make the failure/success sets configurable
too, we could then add:
DontFailExitStatus=SIGSEGV 1 2
(But please, don't implement this bit just yet, let's wait for somebody
actually needing this. Note though, that Upstart actually does have
functionality like this).
+*ignored = new(int, k);
Maybe use a bitfield here, like we do for the syscalls list?
(Actualy two bitfields, one for the exit codes, one for the exit signals).
+static int check_ignored_rc(Service *s){
Please don't use acronyms in symbols if it's easy to avoid them and no
strong incentive to use them.
+int n_restart_ignored_codes;
Generally we try to used unsigned rather than int for values where
negative values really never make sense, like in this case. It gives
readers a bit of a hint that this field never can be negative and no
special cases like that are used.
Otherwise looks good.
Lennart
Thanks for the feedback. I have altered name of the option to
RestartIgnoreExitStatus, but feel free to change to anything else and I
have also stored the codes to a set instead of an array.
Lukas
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
Lukáš Nykrýn píše v Po 06. 08. 2012 v 15:26 +0200:
Lennart Poettering píše v Pá 03. 08. 2012 v 20:46 +0200:
On Tue, 24.07.12 16:43, Lukas Nykryn (lnyk...@redhat.com) wrote:
In some cases, like wrong configuration, restarting after error
exit code does not help, so administrator can specify RestartIgnoreCodes
which will not cause restart of a service.
I am not happy with the term Code here, it's a bit too generic to be
self explanatory.
And I think I agree with Zbigniew to a certain degree: we might want to
think about whether we should also allow configuring non-failure exit
codes or so. I mean, I am not suggesting we should implement that
right-away, but just keep in mind how that setting would look like so
that we can keep things uniform if we add it later. (In fact, I'd
recommend not to implement it right away: adding an option nobody so far
needed seems like a bad idea if we want to keep our set of options
minimal).
Also, I think we need to think further than just exit codes,
i.e. signals and stuff.
Maybe DontRestartExitStatus=? The libc calls the generalization of
exit code and exit signal the exit status, so that sounds like the
best term to use here.
And then people could write:
DontRestartExitStatus=SIGTERM 1 2 3 4
or something like this?
(Of course, the don't in the name is a negative option, which we try
to avoid, but it appears weird to have a positive option for this, so i
think it would be ok...)
One day, if we really need to make the failure/success sets configurable
too, we could then add:
DontFailExitStatus=SIGSEGV 1 2
(But please, don't implement this bit just yet, let's wait for somebody
actually needing this. Note though, that Upstart actually does have
functionality like this).
+*ignored = new(int, k);
Maybe use a bitfield here, like we do for the syscalls list?
(Actualy two bitfields, one for the exit codes, one for the exit signals).
+static int check_ignored_rc(Service *s){
Please don't use acronyms in symbols if it's easy to avoid them and no
strong incentive to use them.
+int n_restart_ignored_codes;
Generally we try to used unsigned rather than int for values where
negative values really never make sense, like in this case. It gives
readers a bit of a hint that this field never can be negative and no
special cases like that are used.
Otherwise looks good.
Lennart
Thanks for the feedback. I have altered name of the option to
RestartIgnoreExitStatus, but feel free to change to anything else and I
have also stored the codes to a set instead of an array.
Lukas
forgotten patch
From 81ffeefb01aa73c3be2b2bba7500df5f8ebcdfc9 Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 6 Aug 2012 14:54:03 +0200
Subject: [PATCH] service: allow service to inhibit respawn with special
return code
In some cases, like wrong configuration, restarting after error
does not help, so administrator can specify statuses by RestartPreventExitStatus
which will not cause restart of a service.
---
man/systemd.service.xml |7
src/core/load-fragment-gperf.gperf.m4 |1 +
src/core/service.c|7 +++-
src/core/service.h|1 +
src/shared/conf-parser.c | 60 +
src/shared/conf-parser.h |1 +
6 files changed, 76 insertions(+), 1 deletions(-)
diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index f43201d..8533136 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -558,6 +558,13 @@
/varlistentry
varlistentry
+termvarnameRestartPreventExitStatus=/varname/term
+listitemparaSpecify return codes list, which
+will prevent service from restart. Codes are
+separated by whitespace./para/listitem
+/varlistentry
+
+varlistentry
termvarnamePermissionsStartOnly=/varname/term
listitemparaTakes a boolean
argument. If true, the permission
diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4
index 2b1cfa0..d077376 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -156,6 +156,7 @@ Service.PermissionsStartOnly,config_parse_bool, 0,
Service.RootDirectoryStartOnly, config_parse_bool, 0, offsetof(Service, root_directory_start_only)
Service.RemainAfterExit, config_parse_bool, 0, offsetof(Service, remain_after_exit)
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Mon, 06.08.12 16:45, Lukáš Nykrýn (lnyk...@redhat.com) wrote:
+if (!*set) {
+set_free(*set);
+*set = NULL;
+}
You probably mean if (*set) here, not (!*set). But honestly I think this
bit should just go entirely as for most other settings we actually do
allow adding in additional values later on if it makes sense (example:
Environment=).
+
+*set = set_new(trivial_hash_func, trivial_compare_func);
+if (!*set)
+return -ENOMEM;
We probably should start using log_oom() for things like this now that
it is available.
+
+FOREACH_WORD(w, l, rvalue, state)
+{
+char *temp = new0(char, l + 1);
+if (!temp)
+return -ENOMEM;
Please use strndup() here! (And also log_oom() here please).
+
+strncpy(temp, w, l);
+r = safe_atoi(temp, val);
+free(temp);
+if (r 0) {
+log_error([%s:%u] Failed to parse numeric value:
%s, filename, line, w);
+set_free(*set);
+*set = NULL;
I'd leave the hashmap set, as it is freed anyway when the service goes
away, and makes simpler and easier to read.
+return r;
+}
+r = set_put(*set, INT_TO_PTR(val));
+if (r 0) {
+log_error([%s:%u] Unable to store: %s, filename,
line, w);
+set_free(*set);
+*set = NULL;
Same here.
Otherwise looks good.
(We want the signal stuff eventually I guess, but we can easily add this later).
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
Lennart Poettering píše v Po 06. 08. 2012 v 16:52 +0200:
On Mon, 06.08.12 16:45, Lukáš Nykrýn (lnyk...@redhat.com) wrote:
+if (!*set) {
+set_free(*set);
+*set = NULL;
+}
You probably mean if (*set) here, not (!*set). But honestly I think this
bit should just go entirely as for most other settings we actually do
allow adding in additional values later on if it makes sense (example:
Environment=).
+
+*set = set_new(trivial_hash_func, trivial_compare_func);
+if (!*set)
+return -ENOMEM;
We probably should start using log_oom() for things like this now that
it is available.
+
+FOREACH_WORD(w, l, rvalue, state)
+{
+char *temp = new0(char, l + 1);
+if (!temp)
+return -ENOMEM;
Please use strndup() here! (And also log_oom() here please).
+
+strncpy(temp, w, l);
+r = safe_atoi(temp, val);
+free(temp);
+if (r 0) {
+log_error([%s:%u] Failed to parse numeric value:
%s, filename, line, w);
+set_free(*set);
+*set = NULL;
I'd leave the hashmap set, as it is freed anyway when the service goes
away, and makes simpler and easier to read.
+return r;
+}
+r = set_put(*set, INT_TO_PTR(val));
+if (r 0) {
+log_error([%s:%u] Unable to store: %s, filename,
line, w);
+set_free(*set);
+*set = NULL;
Same here.
Otherwise looks good.
(We want the signal stuff eventually I guess, but we can easily add this
later).
Lennart
Again thanks for review. Here is modified patch.
If you think that it would be better to add signal stuff before
accepting this patch I will not disagree.
Lukas
From 986dc67a74a4386e3e95f1b09cfa36dd71b77321 Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 6 Aug 2012 14:54:03 +0200
Subject: [PATCH] service: allow service to inhibit respawn with special
return code
In some cases, like wrong configuration, restarting after error
does not help, so administrator can specify statuses by RestartPreventExitStatus
which will not cause restart of a service.
---
man/systemd.service.xml |7
src/core/load-fragment-gperf.gperf.m4 |1 +
src/core/service.c|7 -
src/core/service.h|1 +
src/shared/conf-parser.c | 50 +
src/shared/conf-parser.h |1 +
6 files changed, 66 insertions(+), 1 deletions(-)
diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index f43201d..8533136 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -558,6 +558,13 @@
/varlistentry
varlistentry
+termvarnameRestartPreventExitStatus=/varname/term
+listitemparaSpecify return codes list, which
+will prevent service from restart. Codes are
+separated by whitespace./para/listitem
+/varlistentry
+
+varlistentry
termvarnamePermissionsStartOnly=/varname/term
listitemparaTakes a boolean
argument. If true, the permission
diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4
index 2b1cfa0..d077376 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -156,6 +156,7 @@ Service.PermissionsStartOnly,config_parse_bool, 0,
Service.RootDirectoryStartOnly, config_parse_bool, 0, offsetof(Service, root_directory_start_only)
Service.RemainAfterExit, config_parse_bool, 0, offsetof(Service, remain_after_exit)
Service.GuessMainPID,config_parse_bool, 0, offsetof(Service, guess_main_pid)
+Service.RestartPreventExitStatus, config_parse_set_int, 0, offsetof(Service, restart_ignore_status)
m4_ifdef(`HAVE_SYSV_COMPAT',
`Service.SysVStartPriority, config_parse_sysv_priority, 0, offsetof(Service, sysv_start_priority)',
`Service.SysVStartPriority, config_parse_warn_compat, 0, 0')
diff --git a/src/core/service.c b/src/core/service.c
index 1c127bd..ae894b7 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -293,6 +293,9 @@ static void service_done(Unit *u) {
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
2012/8/3 Lennart Poettering lenn...@poettering.net: On Tue, 24.07.12 16:43, Lukas Nykryn (lnyk...@redhat.com) wrote: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. And then people could write: DontRestartExitStatus=SIGTERM 1 2 3 4 RestartNormalExitStatus = ... One day, if we really need to make the failure/success sets configurable too, we could then add: DontFailExitStatus=SIGSEGV 1 2 NormalExitStatus = ... Doesn't sound too bad to me. Michael -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Monday 06 Aug 2012 12:16:02 Lennart Poettering wrote: On Mon, 06.08.12 12:06, Kay Sievers (k...@vrfy.org) wrote: How about: ExitStatusFailure= ExitStatusSuccess= success and failure should be parititions of the exit status set, i.e. all exits that are not failure are success and all that are success are not failure, and there is nothing but success/failure in the set. Could allow either, but make it an error to use both. So if you configure ExitStatusSuccess, then any values not listed are failures, but if you configure ExitStatusFailure, then any values not listed are successes. Or just have ExitStatusSuccess, but allow ! as the first character which negates the list, meaning any status except those listed is success Maybe? Adam ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
'Twas brillig, and Lennart Poettering at 03/08/12 19:46 did gyre and gimble: Maybe DontRestartExitStatus=? The libc calls the generalization of exit code and exit signal the exit status, so that sounds like the best term to use here. Would InhibitRestartExitStatus= work for you? Something feels wrong with Dont* (partly because don't is a contraction of do not and DoNotRestartExitStatus just feels wrong too). Not sure if Inhibit* is maybe used somewhere already that would make this less desirable tho'. Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On Tue, 24.07.12 16:43, Lukas Nykryn (lnyk...@redhat.com) wrote:
In some cases, like wrong configuration, restarting after error
exit code does not help, so administrator can specify RestartIgnoreCodes
which will not cause restart of a service.
I am not happy with the term Code here, it's a bit too generic to be
self explanatory.
And I think I agree with Zbigniew to a certain degree: we might want to
think about whether we should also allow configuring non-failure exit
codes or so. I mean, I am not suggesting we should implement that
right-away, but just keep in mind how that setting would look like so
that we can keep things uniform if we add it later. (In fact, I'd
recommend not to implement it right away: adding an option nobody so far
needed seems like a bad idea if we want to keep our set of options
minimal).
Also, I think we need to think further than just exit codes,
i.e. signals and stuff.
Maybe DontRestartExitStatus=? The libc calls the generalization of
exit code and exit signal the exit status, so that sounds like the
best term to use here.
And then people could write:
DontRestartExitStatus=SIGTERM 1 2 3 4
or something like this?
(Of course, the don't in the name is a negative option, which we try
to avoid, but it appears weird to have a positive option for this, so i
think it would be ok...)
One day, if we really need to make the failure/success sets configurable
too, we could then add:
DontFailExitStatus=SIGSEGV 1 2
(But please, don't implement this bit just yet, let's wait for somebody
actually needing this. Note though, that Upstart actually does have
functionality like this).
+*ignored = new(int, k);
Maybe use a bitfield here, like we do for the syscalls list?
(Actualy two bitfields, one for the exit codes, one for the exit signals).
+static int check_ignored_rc(Service *s){
Please don't use acronyms in symbols if it's easy to avoid them and no
strong incentive to use them.
+int n_restart_ignored_codes;
Generally we try to used unsigned rather than int for values where
negative values really never make sense, like in this case. It gives
readers a bit of a hint that this field never can be negative and no
special cases like that are used.
Otherwise looks good.
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
Zbigniew Jędrzejewski-Szmek píše v St 25. 07. 2012 v 01:19 +0200: On 07/24/2012 04:43 PM, Lukas Nykryn wrote: In some cases, like wrong configuration, restarting after error exit code does not help, so administrator can specify RestartIgnoreCodes which will not cause restart of a service. Hi, maybe this should be made more general, not only limited to restarting: wouldn't it be better to simply specify, which exit codes are supposed to mean success? Before there was a case with java, which would exit returning 1, even on success. It would be nice to solve that problem too. Zbyszek Thanks for the suggestion, I did it this way, because this should be solution for https://bugzilla.redhat.com/show_bug.cgi?id=807886. I am not sure if I should rewrite it in more general way. If I am not mistaken such behavior would also affect for example systemctl status and then when service fails with wrong configuration, it would show as success in status. But I do not say that specifying concrete return codes as success is not acceptable feature, but I would rather see this two option separately. I am going to rewrite this patch to use set instead of array, so I will also write the second patch with general approach. Lukas ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
In some cases, like wrong configuration, restarting after error
exit code does not help, so administrator can specify RestartIgnoreCodes
which will not cause restart of a service.
---
man/systemd.service.xml |7
src/core/load-fragment-gperf.gperf.m4 |1 +
src/core/load-fragment.c | 59 +
src/core/load-fragment.h |1 +
src/core/service.c| 16 -
src/core/service.h|2 +
6 files changed, 85 insertions(+), 1 deletions(-)
diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index f43201d..6b724ea 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -558,6 +558,13 @@
/varlistentry
varlistentry
+
termvarnameRestartIgnoreCodes=/varname/term
+listitemparaSpecify return codes list,
which
+will prevent service from restart. Codes are
+separated by whitespace./para/listitem
+/varlistentry
+
+varlistentry
termvarnamePermissionsStartOnly=/varname/term
listitemparaTakes a boolean
argument. If true, the permission
diff --git a/src/core/load-fragment-gperf.gperf.m4
b/src/core/load-fragment-gperf.gperf.m4
index d6a4711..ff40a64 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -151,6 +151,7 @@ Service.StartLimitBurst, config_parse_unsigned,
0,
Service.StartLimitAction,config_parse_start_limit_action,0,
offsetof(Service, start_limit_action)
Service.Type,config_parse_service_type, 0,
offsetof(Service, type)
Service.Restart, config_parse_service_restart, 0,
offsetof(Service, restart)
+Service.RestartIgnoreCodes, config_parse_service_restart_ignore, 0,
offsetof(Service, restart_ignored_codes)
Service.PermissionsStartOnly,config_parse_bool, 0,
offsetof(Service, permissions_start_only)
Service.RootDirectoryStartOnly, config_parse_bool, 0,
offsetof(Service, root_directory_start_only)
Service.RemainAfterExit, config_parse_bool, 0,
offsetof(Service, remain_after_exit)
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index bbd82b9..fa002e8 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -2084,6 +2084,65 @@ int config_parse_syscall_filter(
return 0;
}
+int config_parse_service_restart_ignore(
+const char *filename,
+unsigned line,
+const char *section,
+const char *lvalue,
+int ltype,
+const char *rvalue,
+void *data,
+void *userdata) {
+
+char *w;
+unsigned k=0;
+size_t l;
+char *state;
+int r;
+int **ignored = data;
+Service *s = userdata;
+
+assert(filename);
+assert(lvalue);
+assert(rvalue);
+assert(data);
+assert(userdata);
+
+FOREACH_WORD(w, l, rvalue, state)
+k++;
+
+if (*ignored) {
+s-n_restart_ignored_codes = 0;
+free(*ignored);
+*ignored = NULL;
+}
+
+*ignored = new(int, k);
+if (!*ignored)
+return -ENOMEM;
+
+s-n_restart_ignored_codes = k;
+
+k=0;
+FOREACH_WORD(w, l, rvalue, state){
+char *temp = new0(char, l+1);
+if (!temp)
+return -ENOMEM;
+strncpy(temp, w, l);
+if ((r = safe_atoi(temp, (*ignored)[k++])) 0) {
+log_error([%s:%u] Failed to parse numeric value: %s,
filename, line, w);
+s-n_restart_ignored_codes = 0;
+free(*ignored);
+*ignored = NULL;
+free(temp);
+return r;
+}
+free(temp);
+}
+
+return 0;
+}
+
#define FOLLOW_MAX 8
static int open_follow(char **filename, FILE **_f, Set *names, char **_final) {
diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h
index 24f7384..1064e95 100644
--- a/src/core/load-fragment.h
+++ b/src/core/load-fragment.h
@@ -82,6 +82,7 @@ int config_parse_unit_blkio_weight(const char *filename,
unsigned line, const ch
int config_parse_unit_blkio_bandwidth(const char *filename, unsigned line,
Re: [systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code
On 07/24/2012 04:43 PM, Lukas Nykryn wrote: In some cases, like wrong configuration, restarting after error exit code does not help, so administrator can specify RestartIgnoreCodes which will not cause restart of a service. Hi, maybe this should be made more general, not only limited to restarting: wouldn't it be better to simply specify, which exit codes are supposed to mean success? Before there was a case with java, which would exit returning 1, even on success. It would be nice to solve that problem too. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
