Re: RFC: [PATCH] ignore SIGINT&QUIT while waiting for external command

From: Jeff King <peff@peff.net>
Date: Oct 19, 2010, 19:50 UTC
Message-ID: <20101019195022.GA7287@sigill.intra.peff.net>
In-Reply-To: <20101019191638.GI25139@burratino>

On Tue, Oct 19, 2010 at 02:16:38PM -0500, Jonathan Nieder wrote:

> I think sigchain_push ought to accept a context object.

But signal() doesn't, so we would have to install a wrapper function that gets the signal and calls the sigchain_pushed callback with the context object. But we can't always install the wrapper. We need to check for SIG_IGN and SIG_DFL, and literally install those.

So I think it's do-able, but I tried to keep the original sigchain as simple as possible.

Show 23 quoted lines

> +static void interrupted_with_child(int sig)
> +{
> +	if (the_child && the_child->pid > 0) {
> +		while ((waiting = waitpid(pid, NULL, 0)) < 0 && errno == EINTR)
> +			;	/* nothing */
> +		the_child = NULL;
> +	}
> +	sigchain_pop(sig);
> +	raise(sig);
> +}
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -206,8 +220,11 @@ fail_pipe:
>  		notify_pipe[0] = notify_pipe[1] = -1;
>  
>  	fflush(NULL);
> -	sigchain_push(SIGINT, SIG_IGN);
> -	sigchain_push(SIGQUIT, SIG_IGN);
> +	if (the_child)
> +		die("What?  _Two_ children?");
> +	the_child = cmd;

Yuck. You can get around that by pushing onto a linked list of children, though.

Thinking about it more, though, I don't think we do necessarily want to always wait for the child. There are really two main types of run_command's we do:

  1. The run command is basically the new process. In an ideal world, we
     would exec into it, but we need the parent to hang around to do
     some kind of bookkeeping (like waiting for the pager to exit).

     E.g., running external dashed commands.

  2. We are running the command, and if we are killed, the command
     should go away too (because its point in running is to give us some
     information).

     E.g., running textconv filters.

And there are a few instances that don't fall into either category (e.g., running the pager).

In case (1), we probably want to SIG_IGN, wait for the command to finish, and then die with its exit code. If we do it right, the fact that _it_ was killed by signal will be propagated, and the fact that we weren't will be irrelevant.

In case (2), we probably want to keep a linked list of "expendable" processes, and on signal death and atexit, go through the list and make sure all are dead. This is how we handle tempfiles already in diff.c.

Given that there is only really one instance of (1), we can just code it there. For (2), there are many such callers, but I don't know that the mechanism necessarily needs to be included as part of run_command. A separate module to manage the list and set up the signal handler would be fine (though there is a race between fork() and signal death, so it perhaps pays to get the newly created pid on the "expendable" list as soon as possible, which may mean cooperating from run_command).

-Peff