From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24256 invoked by alias); 2 Dec 2014 22:11:17 -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 24245 invoked by uid 89); 2 Dec 2014 22:11:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 02 Dec 2014 22:11:16 +0000 Received: from EUSAAHC007.ericsson.se (Unknown_Domain [147.117.188.93]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 28.E6.25146.6DCDD745; Tue, 2 Dec 2014 16:37:58 +0100 (CET) Received: from [142.133.110.254] (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.93) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 2 Dec 2014 17:11:13 -0500 Message-ID: <547E3900.6060704@ericsson.com> Date: Tue, 02 Dec 2014 22:11:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Sergio Durigan Junior 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> <871toraqme.fsf@redhat.com> In-Reply-To: <871toraqme.fsf@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-12/txt/msg00038.txt.bz2 On 2014-11-25 04:59 PM, Sergio Durigan Junior wrote: > 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 Oops, I had missed your reply. Thanks for the review, I just sent a V2. Simon