Re: [RFC] Don't immediately SIGTERM the child of "target remote |".

[Doug Evans <dje at google dot com>]

Sorry for the followup.
Minor comment inline.

On Wed, Nov 23, 2011 at 4:00 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> While testing a patch to allow gdbserver to communicate over stdio
> (e.g. target remote | gdbserver ...)
> I found that several tests from the testsuite were left running
> and traced it to gdb SIGTERMing gdbserver when the connection is closed.
>
> We could add a SIGTERM handler to gdbserver
> but I'm starting with this patch.
> I don't understand why gdb kills the child at all.
> In a properly written child connection, closing stdin should be a
> sufficient signal for the child to know to shutdown.
>
> AFAICT The kill() call was added in cvs revision 1.2 with the log entry:
> Replace ../include/wait.h with gdb_wait.h.
> Blech.
>
> Alas I can't remove this kill(SIGTERM) call
> for fear of breaking something so I'm trying a
> compromise and only killing the child after some timeout.
>
> Comments?
>
> I borrowed the SA_RESTART handling from other places in gdb.
> I don't actually know that this works (i.e. the alarm call
> will wake up waitpid) on systems without SA_RESTART,
> and I don't know if alarm() is portable enough
> (there are no current uses of it AFAICT).
>
> 2011-11-23 ?Doug Evans ?<dje@google.com>
>
> ? ? ? ?* ser-pipe.c (sigalrm_handler): New function.
> ? ? ? ?(pipe_close): Don't immediately send SIGTERM to the child.
> ? ? ? ?Give it awhile to cleanly shutdown, but don't wait forever.
>
> Index: ser-pipe.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ser-pipe.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 ser-pipe.c
> --- ser-pipe.c ?4 Mar 2011 19:23:42 -0000 ? ? ? 1.32
> +++ ser-pipe.c ?23 Nov 2011 23:46:55 -0000
> @@ -154,6 +154,12 @@ pipe_open (struct serial *scb, const cha
> ?}
>
> ?static void
> +sigalrm_handler (int signo)
> +{
> + ?/* nothing to do */
> +}
> +
> +static void
> ?pipe_close (struct serial *scb)
> ?{
> ? struct pipe_state *state = scb->state;
> @@ -163,14 +169,45 @@ pipe_close (struct serial *scb)
>
> ? if (state != NULL)
> ? ? {
> - ? ? ?int status;
> - ? ? ?kill (state->pid, SIGTERM);
> + ? ? ?int rc, status;
> + ? ? ?void (*ofunc) (); ? ? ? ?/* Previous SIGALRM handler. ?*/
> +
> + ? ? ?/* Don't kill the task right away, give it a chance to shut down cleanly.
> + ? ? ? ?But don't wait forever though. ?*/
> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> + ? ? ?{
> + ? ? ? struct sigaction sa, osa;
> + ? ? ? sa.sa_handler = sigalrm_handler;
> + ? ? ? sigemptyset (&sa.sa_mask);
> + ? ? ? sa.sa_flags = 0;
> + ? ? ? sigaction (SIGALRM, &sa, &osa);
> + ? ? ? ofunc = osa.sa_handler;
> + ? ? ?}
> +#else
> + ? ? ?ofunc = (void (*)()) signal (SIGALRM, sigalrm_handler);
> +#endif
> +
> +#define PIPE_CLOSE_TIMEOUT 5
> + ? ? ?alarm (PIPE_CLOSE_TIMEOUT);
> +
> + ? ? ?rc = -1;
> ?#ifdef HAVE_WAITPID
> ? ? ? /* Assume the program will exit after SIGTERM. ?Might be
> ? ? ? ? useful to print any remaining stderr output from
> ? ? ? ? scb->error_fd while waiting. ?*/
> - ? ? ?waitpid (state->pid, &status, 0);
> + ? ? ?rc = waitpid (state->pid, &status, 0);
> +#endif
> + ? ? ?if (rc != 0)
> + ? ? ? {
> + ? ? ? ? kill (state->pid, SIGTERM);
> +#ifdef HAVE_WAITPID

Hmmm, in order to be compatible with the previous code the alarm
should be cancelled before this call to waitpid.
OTOH, having an alarm could be a useful thing anyway.

> + ? ? ? ? waitpid (state->pid, &status, 0);
> ?#endif
> + ? ? ? }
> +
> + ? ? ?alarm (0);
> + ? ? ?signal (SIGALRM, ofunc);
> +
> ? ? ? if (scb->error_fd != -1)
> ? ? ? ?close (scb->error_fd);
> ? ? ? scb->error_fd = -1;
>