From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16577 invoked by alias); 24 Nov 2011 00:09:33 -0000 Received: (qmail 16569 invoked by uid 22791); 24 Nov 2011 00:09:31 -0000 X-SWARE-Spam-Status: No, hits=-3.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-qw0-f41.google.com (HELO mail-qw0-f41.google.com) (209.85.216.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 Nov 2011 00:09:17 +0000 Received: by qap15 with SMTP id 15so1088769qap.0 for ; Wed, 23 Nov 2011 16:09:16 -0800 (PST) Received: by 10.224.217.131 with SMTP id hm3mr12037521qab.81.1322093355898; Wed, 23 Nov 2011 16:09:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.224.217.131 with SMTP id hm3mr12037515qab.81.1322093355748; Wed, 23 Nov 2011 16:09:15 -0800 (PST) Received: by 10.224.6.76 with HTTP; Wed, 23 Nov 2011 16:09:15 -0800 (PST) In-Reply-To: <20111124000052.14DC12461B1@ruffy.mtv.corp.google.com> References: <20111124000052.14DC12461B1@ruffy.mtv.corp.google.com> Date: Thu, 24 Nov 2011 00:09:00 -0000 Message-ID: Subject: Re: [RFC] Don't immediately SIGTERM the child of "target remote |". From: Doug Evans To: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-11/txt/msg00666.txt.bz2 Sorry for the followup. Minor comment inline. On Wed, Nov 23, 2011 at 4:00 PM, Doug Evans 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 =A0Doug Evans =A0 > > =A0 =A0 =A0 =A0* ser-pipe.c (sigalrm_handler): New function. > =A0 =A0 =A0 =A0(pipe_close): Don't immediately send SIGTERM to the child. > =A0 =A0 =A0 =A0Give it awhile to cleanly shutdown, but don't wait forever. > > Index: ser-pipe.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 =A04 Mar 2011 19:23:42 -0000 =A0 =A0 =A0 1.32 > +++ ser-pipe.c =A023 Nov 2011 23:46:55 -0000 > @@ -154,6 +154,12 @@ pipe_open (struct serial *scb, const cha > =A0} > > =A0static void > +sigalrm_handler (int signo) > +{ > + =A0/* nothing to do */ > +} > + > +static void > =A0pipe_close (struct serial *scb) > =A0{ > =A0 struct pipe_state *state =3D scb->state; > @@ -163,14 +169,45 @@ pipe_close (struct serial *scb) > > =A0 if (state !=3D NULL) > =A0 =A0 { > - =A0 =A0 =A0int status; > - =A0 =A0 =A0kill (state->pid, SIGTERM); > + =A0 =A0 =A0int rc, status; > + =A0 =A0 =A0void (*ofunc) (); =A0 =A0 =A0 =A0/* Previous SIGALRM handler= . =A0*/ > + > + =A0 =A0 =A0/* Don't kill the task right away, give it a chance to shut = down cleanly. > + =A0 =A0 =A0 =A0But don't wait forever though. =A0*/ > +#if defined (HAVE_SIGACTION) && defined (SA_RESTART) > + =A0 =A0 =A0{ > + =A0 =A0 =A0 struct sigaction sa, osa; > + =A0 =A0 =A0 sa.sa_handler =3D sigalrm_handler; > + =A0 =A0 =A0 sigemptyset (&sa.sa_mask); > + =A0 =A0 =A0 sa.sa_flags =3D 0; > + =A0 =A0 =A0 sigaction (SIGALRM, &sa, &osa); > + =A0 =A0 =A0 ofunc =3D osa.sa_handler; > + =A0 =A0 =A0} > +#else > + =A0 =A0 =A0ofunc =3D (void (*)()) signal (SIGALRM, sigalrm_handler); > +#endif > + > +#define PIPE_CLOSE_TIMEOUT 5 > + =A0 =A0 =A0alarm (PIPE_CLOSE_TIMEOUT); > + > + =A0 =A0 =A0rc =3D -1; > =A0#ifdef HAVE_WAITPID > =A0 =A0 =A0 /* Assume the program will exit after SIGTERM. =A0Might be > =A0 =A0 =A0 =A0 useful to print any remaining stderr output from > =A0 =A0 =A0 =A0 scb->error_fd while waiting. =A0*/ > - =A0 =A0 =A0waitpid (state->pid, &status, 0); > + =A0 =A0 =A0rc =3D waitpid (state->pid, &status, 0); > +#endif > + =A0 =A0 =A0if (rc !=3D 0) > + =A0 =A0 =A0 { > + =A0 =A0 =A0 =A0 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. > + =A0 =A0 =A0 =A0 waitpid (state->pid, &status, 0); > =A0#endif > + =A0 =A0 =A0 } > + > + =A0 =A0 =A0alarm (0); > + =A0 =A0 =A0signal (SIGALRM, ofunc); > + > =A0 =A0 =A0 if (scb->error_fd !=3D -1) > =A0 =A0 =A0 =A0close (scb->error_fd); > =A0 =A0 =A0 scb->error_fd =3D -1; >