[RFC] stopsignal stanza
Marc - A. Dahlhaus
mad at wol.de
Wed May 4 10:52:35 UTC 2011
Am Montag, den 02.05.2011, 16:28 -0700 schrieb Scott James Remnant:
> Looks to me like you're on exactly the right track here.
>
>
> Just needs some more test coverage. I have no further comments ;-)
>
Version with tests attached...
IMO the naming of the kill stanza is now slightly inappropriate:
kill timeout VAL:
Was clear for defining the timeout before we send a SIGKILL after
stopping a job.
kill signal SIG:
We are not changing the SIGKILL signal here but the SIGNAL we send to a
Job that should be stopped before we send a kill signal if the job
doesn't stop before it stops longer as the defined timeout allows it to
do.
I suggest changing the name of the stanza from kill to stopping and add
an alias kill for backward compatibility and document it as such. It
would be a better and not misleading name as we send "stopping signal"
when we stop the job and we would send SIGKILL to it after the "stopping
timeout" is reached.
What do you think?
Marc
--- a/init/errors.h
+++ b/init/errors.h
@@ -40,6 +40,7 @@ enum {
/* Errors while parsing configuration files */
PARSE_ILLEGAL_INTERVAL,
PARSE_ILLEGAL_EXIT,
+ PARSE_ILLEGAL_SIGNAL,
PARSE_ILLEGAL_UMASK,
PARSE_ILLEGAL_NICE,
PARSE_ILLEGAL_OOM,
@@ -60,6 +61,7 @@ enum {
#define ENVIRON_MISMATCHED_BRACES_STR N_("Mismatched braces")
#define PARSE_ILLEGAL_INTERVAL_STR N_("Illegal interval, expected
number of seconds")
#define PARSE_ILLEGAL_EXIT_STR N_("Illegal exit status, expected
integer")
+#define PARSE_ILLEGAL_SIGNAL_STR N_("Illegal signal status, expected
integer")
#define PARSE_ILLEGAL_UMASK_STR N_("Illegal file creation mask,
expected octal integer")
#define PARSE_ILLEGAL_NICE_STR N_("Illegal nice value, expected -20 to
19")
#define PARSE_ILLEGAL_OOM_STR N_("Illegal oom adjustment, expected -16
to 15 or never")
--- a/init/job_class.c
+++ b/init/job_class.c
@@ -26,6 +26,7 @@
#include <errno.h>
#include <string.h>
+#include <signal.h>
#include <nih/macros.h>
#include <nih/alloc.h>
@@ -199,6 +200,7 @@ job_class_new (const void *parent,
class->task = FALSE;
class->kill_timeout = JOB_DEFAULT_KILL_TIMEOUT;
+ class->kill_signal = SIGTERM;
class->respawn = FALSE;
class->respawn_limit = JOB_DEFAULT_RESPAWN_LIMIT;
--- a/init/job_class.h
+++ b/init/job_class.h
@@ -129,6 +129,7 @@ typedef struct job_class {
int task;
time_t kill_timeout;
+ int kill_signal;
int respawn;
int respawn_limit;
--- a/init/job_process.c
+++ b/init/job_process.c
@@ -769,9 +769,9 @@ job_process_error_read (int fd)
* @process: process to be killed.
*
* This function forces a @job to leave its current state by sending
- * @process the TERM signal, and maybe later the KILL signal. The
actual
- * state changes are performed by job_child_reaper when the process
- * has actually terminated.
+ * @process the "kill signal" defined signal (TERM by default), and
maybe
+ * later the KILL signal. The actual state changes are performed by
+ * job_child_reaper when the process has actually terminated.
**/
void
job_process_kill (Job *job,
@@ -782,15 +782,17 @@ job_process_kill (Job *job,
nih_assert (job->kill_timer == NULL);
nih_assert (job->kill_process = -1);
- nih_info (_("Sending TERM signal to %s %s process (%d)"),
+ nih_info (_("Sending %s signal to %s %s process (%d)"),
+ nih_signal_to_name (job->class->kill_signal),
job_name (job), process_name (process), job->pid[process]);
- if (system_kill (job->pid[process], FALSE) < 0) {
+ if (system_send_signal (job->pid[process], job->class->kill_signal) <
0) {
NihError *err;
err = nih_error_get ();
if (err->number != ESRCH)
- nih_warn (_("Failed to send TERM signal to %s %s process (%d): %s"),
+ nih_warn (_("Failed to send %s signal to %s %s process (%d): %s"),
+ nih_signal_to_name (job->class->kill_signal),
job_name (job), process_name (process),
job->pid[process], err->message);
nih_free (err);
@@ -830,15 +832,17 @@ job_process_kill_timer (Job *job,
job->kill_timer = NULL;
job->kill_process = -1;
- nih_info (_("Sending KILL signal to %s %s process (%d)"),
+ nih_info (_("Sending %s signal to %s %s process (%d)"),
+ "KILL",
job_name (job), process_name (process), job->pid[process]);
- if (system_kill (job->pid[process], TRUE) < 0) {
+ if (system_send_signal (job->pid[process], SIGKILL) < 0) {
NihError *err;
err = nih_error_get ();
if (err->number != ESRCH)
- nih_warn (_("Failed to send KILL signal to %s %s process (%d): %s"),
+ nih_warn (_("Failed to send %s signal to %s %s process (%d): %s"),
+ "KILL",
job_name (job), process_name (process),
job->pid[process], err->message);
nih_free (err);
--- a/init/man/init.5
+++ b/init/man/init.5
@@ -563,10 +563,20 @@ may be specified for either.
.\"
.SS Miscellaneous
.TP
+.B kill signal \fISIGNAL
+Specifies the stopping signal,
+.I SIGTERM
+by default, a job's main process will reviece when stopping the
+running job.
+
+.nf
+kill signal INT
+.fi
+.\"
+.TP
.B kill timeout \fIINTERVAL
Specifies the interval between sending the job's main process the
-.I SIGTERM
-and
+"stopping" (see above) and
.I SIGKILL
signals when stopping the running job.
.\"
--- a/init/parse_job.c
+++ b/init/parse_job.c
@@ -1782,6 +1782,7 @@ stanza_kill (JobClass *class,
{
size_t a_pos, a_lineno;
int ret = -1;
+ char *endptr;
nih_local char *arg = NULL;
nih_assert (class != NULL);
@@ -1799,7 +1800,6 @@ stanza_kill (JobClass *class,
if (! strcmp (arg, "timeout")) {
nih_local char *timearg = NULL;
- char *endptr;
/* Update error position to the timeout value */
*pos = a_pos;
@@ -1816,14 +1816,40 @@ stanza_kill (JobClass *class,
if (errno || *endptr || (class->kill_timeout < 0))
nih_return_error (-1, PARSE_ILLEGAL_INTERVAL,
_(PARSE_ILLEGAL_INTERVAL_STR));
+ } else if (! strcmp (arg, "signal")) {
+ unsigned long status;
+ nih_local char *sigarg = NULL;
+ int signal;
- ret = nih_config_skip_comment (file, len, &a_pos, &a_lineno);
+ /* Update error position to the exit status */
+ *pos = a_pos;
+ if (lineno)
+ *lineno = a_lineno;
+
+ sigarg = nih_config_next_arg (NULL, file, len, &a_pos,
+ &a_lineno);
+ if (! sigarg)
+ goto finish;
+
+ signal = nih_signal_from_name (sigarg);
+ if (signal < 0) {
+ errno = 0;
+ status = strtoul (sigarg, &endptr, 10);
+ if (errno || *endptr || (status > INT_MAX))
+ nih_return_error (-1, PARSE_ILLEGAL_SIGNAL,
+ _(PARSE_ILLEGAL_SIGNAL_STR));
+ }
+
+ /* Set the signal */
+ class->kill_signal = signal;
} else {
nih_return_error (-1, NIH_CONFIG_UNKNOWN_STANZA,
_(NIH_CONFIG_UNKNOWN_STANZA_STR));
}
+ ret = nih_config_skip_comment (file, len, &a_pos, &a_lineno);
+
finish:
*pos = a_pos;
if (lineno)
--- a/init/system.c
+++ b/init/system.c
@@ -46,29 +46,23 @@
/**
- * system_kill:
+ * system_send_signal:
* @pid: process id of process,
- * @force: force the death.
+ * @signal: signal to send.
*
- * Kill all processes in the same process group as @pid, which may not
- * necessarily be the group leader.
- *
- * When @force is FALSE, the TERM signal is sent; when it is TRUE, KILL
- * is sent instead.
+ * Send all processes in the same process group as @pid, which may not
+ * necessarily be the group leader the @signal.
*
* Returns: zero on success, negative value on raised error.
**/
int
-system_kill (pid_t pid,
- int force)
+system_send_signal (pid_t pid,
+ int signal)
{
- int signal;
pid_t pgid;
nih_assert (pid > 0);
- signal = (force ? SIGKILL : SIGTERM);
-
pgid = getpgid (pid);
if (kill (pgid > 0 ? -pgid : pid, signal) < 0)
--- a/init/system.h
+++ b/init/system.h
@@ -29,7 +29,7 @@
NIH_BEGIN_EXTERN
-int system_kill (pid_t pid, int force)
+int system_send_signal (pid_t pid, int signal)
__attribute__ ((warn_unused_result));
int system_setup_console (ConsoleType type, int reset)
--- a/init/tests/test_parse_job.c
+++ b/init/tests/test_parse_job.c
@@ -4799,6 +4799,39 @@ test_stanza_kill (void)
}
+ /* Check that a kill stanza with the signal argument and signal,
+ * sets the right signal on the jobs class.
+ */
+ TEST_FEATURE ("with signal and single argument");
+ strcpy (buf, "kill signal INT\n");
+
+ TEST_ALLOC_FAIL {
+ pos = 0;
+ lineno = 1;
+ job = parse_job (NULL, "test", buf, strlen (buf),
+ &pos, &lineno);
+
+ if (test_alloc_failed) {
+ TEST_EQ_P (job, NULL);
+
+ err = nih_error_get ();
+ TEST_EQ (err->number, ENOMEM);
+ nih_free (err);
+
+ continue;
+ }
+
+ TEST_EQ (pos, strlen (buf));
+ TEST_EQ (lineno, 2);
+
+ TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+ TEST_EQ (job->kill_signal, SIGINT);
+
+ nih_free (job);
+ }
+
+
/* Check that the last of multiple kill stanzas is used.
*/
TEST_FEATURE ("with multiple timeout and single argument stanzas");
@@ -4832,6 +4865,37 @@ test_stanza_kill (void)
}
+ TEST_FEATURE ("with multiple signal and single argument stanzas");
+ strcpy (buf, "kill signal INT\n");
+ strcat (buf, "kill signal TERM\n");
+
+ TEST_ALLOC_FAIL {
+ pos = 0;
+ lineno = 1;
+ job = parse_job (NULL, "test", buf, strlen (buf),
+ &pos, &lineno);
+
+ if (test_alloc_failed) {
+ TEST_EQ_P (job, NULL);
+
+ err = nih_error_get ();
+ TEST_EQ (err->number, ENOMEM);
+ nih_free (err);
+
+ continue;
+ }
+
+ TEST_EQ (pos, strlen (buf));
+ TEST_EQ (lineno, 3);
+
+ TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+ TEST_EQ (job->kill_signal, SIGTERM);
+
+ nih_free (job);
+ }
+
+
/* Check that a kill stanza without an argument results in a syntax
* error.
*/
@@ -4889,6 +4953,25 @@ test_stanza_kill (void)
nih_free (err);
+ /* Check that a kill stanza with the timeout argument but no timeout
+ * results in a syntax error.
+ */
+ TEST_FEATURE ("with signal and missing argument");
+ strcpy (buf, "kill signal\n");
+
+ pos = 0;
+ lineno = 1;
+ job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+ TEST_EQ_P (job, NULL);
+
+ err = nih_error_get ();
+ TEST_EQ (err->number, NIH_CONFIG_EXPECTED_TOKEN);
+ TEST_EQ (pos, 11);
+ TEST_EQ (lineno, 1);
+ nih_free (err);
+
+
/* Check that a kill timeout stanza with a non-integer argument
* results in a syntax error.
*/
@@ -4965,6 +5048,25 @@ test_stanza_kill (void)
nih_free (err);
+ /* Check that a kill signal stanza with an unknown signal argument
+ * results in a syntax error.
+ */
+ TEST_FEATURE ("with signal and unknown signal argument");
+ strcpy (buf, "kill signal foo\n");
+
+ pos = 0;
+ lineno = 1;
+ job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+ TEST_EQ_P (job, NULL);
+
+ err = nih_error_get ();
+ TEST_EQ (err->number, PARSE_ILLEGAL_SIGNAL);
+ TEST_EQ (pos, 12);
+ TEST_EQ (lineno, 1);
+ nih_free (err);
+
+
/* Check that a kill stanza with the timeout argument and timeout,
* but with an extra argument afterwards results in a syntax
* error.
@@ -4974,6 +5076,26 @@ test_stanza_kill (void)
pos = 0;
lineno = 1;
+ job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+ TEST_EQ_P (job, NULL);
+
+ err = nih_error_get ();
+ TEST_EQ (err->number, NIH_CONFIG_UNEXPECTED_TOKEN);
+ TEST_EQ (pos, 16);
+ TEST_EQ (lineno, 1);
+ nih_free (err);
+
+
+ /* Check that a kill stanza with the signal argument and signal,
+ * but with an extra argument afterwards results in a syntax
+ * error.
+ */
+ TEST_FEATURE ("with signal and extra argument");
+ strcpy (buf, "kill signal INT foo\n");
+
+ pos = 0;
+ lineno = 1;
job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno);
TEST_EQ_P (job, NULL);
--- a/init/tests/test_system.c
+++ b/init/tests/test_system.c
@@ -35,7 +35,7 @@ test_kill (void)
pid_t pid1, pid2, pid3;
int ret, status;
- TEST_FUNCTION ("system_kill");
+ TEST_FUNCTION ("system_send_signal");
/* Check that when we normally kill the process, the TERM signal
* is sent to all processes in its process group.
@@ -51,7 +51,7 @@ test_kill (void)
setpgid (pid1, pid1);
setpgid (pid2, pid1);
- ret = system_kill (pid1, FALSE);
+ ret = system_send_signal (pid1, SIGTERM);
waitpid (pid1, &status, 0);
TEST_EQ (ret, 0);
@@ -79,7 +79,7 @@ test_kill (void)
setpgid (pid1, pid1);
setpgid (pid2, pid1);
- ret = system_kill (pid1, TRUE);
+ ret = system_send_signal (pid1, SIGKILL);
waitpid (pid1, &status, 0);
TEST_EQ (ret, 0);
@@ -114,7 +114,7 @@ test_kill (void)
kill (pid1, SIGTERM);
waitpid (pid1, &status, 0);
- ret = system_kill (pid2, FALSE);
+ ret = system_send_signal (pid2, SIGTERM);
waitpid (pid2, &status, 0);
TEST_EQ (ret, 0);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kill_signal_stanza.patch
Type: text/x-patch
Size: 11778 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/upstart-devel/attachments/20110504/cf7efcbc/attachment-0001.bin>
More information about the upstart-devel
mailing list