[RFC] stopsignal stanza
Scott James Remnant
scott at netsplit.com
Mon May 2 23:28:00 UTC 2011
Looks to me like you're on exactly the right track here.
Just needs some more test coverage. I have no further comments ;-)
On Mon, May 2, 2011 at 2:50 PM, Marc - A. Dahlhaus <mad at wol.de> wrote:
> Am 02.05.2011 20:33, schrieb Scott James Remnant:
> > On Mon, May 2, 2011 at 11:31 AM, Marc - A. Dahlhaus <mad at wol.de
> > <mailto:mad at wol.de>> 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
>
> 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
> +++ 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);
> @@ -833,7 +835,7 @@ job_process_kill_timer (Job *job,
> 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 ();
> --- 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
> @@ -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
> +++ 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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/upstart-devel/attachments/20110502/bb16f9af/attachment.html>
More information about the upstart-devel
mailing list