From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11384 invoked by alias); 25 Nov 2014 21:59:28 -0000 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 Received: (qmail 11369 invoked by uid 89); 25 Nov 2014 21:59:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 25 Nov 2014 21:59:26 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAPLxLsR025902 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 25 Nov 2014 16:59:22 -0500 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sAPLxLVU006903 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Tue, 25 Nov 2014 16:59:21 -0500 From: Sergio Durigan Junior To: Simon Marchi Cc: Subject: Re: [PATCH] Restore terminal state in mi_thread_exit (PR gdb/17627) References: <1416948612-5781-1-git-send-email-simon.marchi@ericsson.com> X-URL: http://blog.sergiodj.net Date: Tue, 25 Nov 2014 21:59:00 -0000 In-Reply-To: <1416948612-5781-1-git-send-email-simon.marchi@ericsson.com> (Simon Marchi's message of "Tue, 25 Nov 2014 15:50:12 -0500") Message-ID: <871toraqme.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00632.txt.bz2 On Tuesday, November 25 2014, Simon Marchi wrote: > When a thread exits, the terminal is left in mode "terminal_is_ours" > while the target executes. This patch fixes that. > > From my understanding, a function calling target_terminal_ours expects > that the terminal could be in any state at the moment it is called. > Therefore, it should be its reponsibility to put back the terminal in > whatever state it was before being called. > > I find that this fits quite well the cleanup model, so I implemented a > cleanup for that. Thanks for the patch. A few comments related to the coding style and cleanups. > gdb/ChangeLog: > > 2014-11-25 Simon Marchi > > PR gdb/17627 > * target.c (cleanup_restore_target_terminal): New function. > (make_cleanup_restore_target_terminal): New function. > * target.h (make_cleanup_restore_target_terminal): New > declaration. > * mi/mi-interp.c (mi_thread_exit): Use the new cleanup. > > Signed-off-by: Simon Marchi > --- > gdb/mi/mi-interp.c | 4 ++++ > gdb/target.c | 30 ++++++++++++++++++++++++++++++ > gdb/target.h | 4 ++++ > 3 files changed, 38 insertions(+) > > diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c > index df2b558..2a62e22 100644 > --- a/gdb/mi/mi-interp.c > +++ b/gdb/mi/mi-interp.c > @@ -386,6 +386,7 @@ mi_thread_exit (struct thread_info *t, int silent) > { > struct mi_interp *mi; > struct inferior *inf; > + struct cleanup *old_chain; > > if (silent) > return; > @@ -393,11 +394,14 @@ mi_thread_exit (struct thread_info *t, int silent) > inf = find_inferior_pid (ptid_get_pid (t->ptid)); > > mi = top_level_interpreter_data (); > + old_chain = make_cleanup_restore_target_terminal(); Space between function name and (). > target_terminal_ours (); > fprintf_unfiltered (mi->event_channel, > "thread-exited,id=\"%d\",group-id=\"i%d\"", > t->num, inf->num); > gdb_flush (mi->event_channel); > + > + do_cleanups(old_chain); Likewise. > } > > /* Emit notification on changing the state of record. */ > diff --git a/gdb/target.c b/gdb/target.c > index ab5f2b9..d6a06bd 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -528,6 +528,36 @@ target_supports_terminal_ours (void) > return 0; > } > > +static void cleanup_restore_target_terminal (void *arg) ^ Break the line here. New functions should have a comment describing them. > +{ > + enum terminal_state *previous_state = arg; > + > + switch (*previous_state) > + { > + case terminal_is_ours: > + target_terminal_ours(); > + break; > + case terminal_is_ours_for_output: > + target_terminal_ours_for_output(); > + break; > + case terminal_is_inferior: > + target_terminal_inferior(); > + break; The "case" should be indented below the "{". Missing whitespace between function names and (). > + } > + > + xfree(previous_state); Likewise. > +} > + > +struct cleanup * > +make_cleanup_restore_target_terminal (void) Missing function comment. Since you already described this one on the header file, you can just say: /* See definition in target.h. */ > +{ > + enum terminal_state *ts = xmalloc(sizeof(*ts)); Whitespace between function name and (). > + > + *ts = terminal_state; > + > + return make_cleanup (cleanup_restore_target_terminal, ts); You should use make_cleanup_dtor here, because "ts" may leak if the cleanup is not called. > +} > + > static void > tcomplain (void) > { > diff --git a/gdb/target.h b/gdb/target.h > index d363b61..a1c3b7b 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -1413,6 +1413,10 @@ extern void target_terminal_ours (void); > > extern int target_supports_terminal_ours (void); > > +/* Make a cleanup that restores the state of the terminal to the current > + * value. */ You just need to use "*" after and before the "/" in comments: /* This is a multi-line comment. */ It's also missing two spaces after period. > +extern struct cleanup *make_cleanup_restore_target_terminal (void); > + > /* Print useful information about our terminal status, if such a thing > exists. */ > > -- > 2.1.3 -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/