Looks to me like you're on exactly the right track here.<div> </div><div>Just needs some more test coverage. I have no further comments ;-) <div class="gmail_quote">On Mon, May 2, 2011 at 2:50 PM, Marc - A. Dahlhaus <<a href="mailto:mad@wol.de">mad@wol.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">Am 02.05.2011 20:33, schrieb Scott James Remnant: </div><div class="im">> On Mon, May 2, 2011 at 11:31 AM, Marc - A. Dahlhaus <<a href="mailto:mad@wol.de">mad@wol.de</a> </div><div><div></div><div class="h5">> <mailto:<a href="mailto:mad@wol.de">mad@wol.de</a>>> wrote: > > Am 02.05.2011 19:58, schrieb Scott James Remnant: > --8<-- > > Talked about the topic with SpamapS on irc and he suggested > that we > > choos another stanza for it as there is a bug open for sending > another > > signal than HUP on reload... > > > > Maybe we better use "signal {stop,reload} SIGNAL" for it?! > > > > I would prefer: > > > > kill signal HUP > > > > since that goes better with timeout, etc. There should already be the > > code to translate signal names (normal exit uses it) > > Nice. For the messages that now mention "sending TERM signal", we need a > intval to signalname translation, is there one in nih? > > Yup, in nih/signal.h - nih_signal_to_name > > > > > > Then we could also rename the system_kill function to > system_signal_send > > and reuse it for the reloading also... > > That renaming ok for you? > > system_send_signal () is less yoda ;-) > > Scott </div></div>This is the patch with a first added test that it does what it should. The comments are also changed to reflect the changes made. Will add some more tests to the testsuite tomorrow and get some sleep now. 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 <div class="im">+++ b/init/job_class.c </div><div class="im">@@ -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; </div>--- a/init/job_class.h <div class="im">+++ b/init/job_class.h </div><div class="im">@@ -129,6 +129,7 @@ typedef struct job_class { int task; time_t kill_timeout; + int kill_signal; int respawn; int respawn_limit; </div>--- a/init/job_process.c <div class="im">+++ b/init/job_process.c </div>@@ -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 <div class="im"> job_process_kill (Job *job, </div>@@ -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)"), <div class="im">+ 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); </div>@@ -833,7 +835,7 @@ job_process_kill_timer (Job *job, <div class="im"> nih_info (_("Sending KILL signal to %s %s process (%d)"), 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 (); </div>--- a/init/parse_job.c <div class="im">+++ b/init/parse_job.c </div><div><div></div><div class="h5">@@ -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)) </div></div>+ nih_return_error (-1, PARSE_ILLEGAL_SIGNAL, + _(PARSE_ILLEGAL_SIGNAL_STR)); <div class="im">+ } + + /* 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) </div>--- a/init/system.c +++ b/init/system.c <div class="im">@@ -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) </div>--- a/init/system.h +++ b/init/system.h <div class="im">@@ -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) </div>--- a/init/tests/test_parse_job.c +++ b/init/tests/test_parse_job.c @@ -4983,6 +4983,38 @@ test_stanza_kill (void) TEST_EQ (pos, 16); TEST_EQ (lineno, 1); nih_free (err); + + /* Check that a kill stanza with the signal argument and signal, + * sets the right signal on the jobs class. + */ + TEST_FEATURE ("with signal and 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); + } } void --- a/init/tests/test_system.c <div class="im">+++ b/init/tests/test_system.c </div><div><div></div><div class="h5">@@ -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); </div></div></blockquote></div> </div>