From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119689 invoked by alias); 23 Mar 2015 18:51:15 -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 119676 invoked by uid 89); 23 Mar 2015 18:51:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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; Mon, 23 Mar 2015 18:51:13 +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 (Postfix) with ESMTPS id C666B91C00; Mon, 23 Mar 2015 18:51:12 +0000 (UTC) Received: from x220.homelab.tallawa.org ([10.3.112.10]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2NIp28r009428 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 23 Mar 2015 14:51:09 -0400 Message-ID: <55105FFD.60204@redhat.com> Date: Mon, 23 Mar 2015 18:51:00 -0000 From: Cleber Rosa User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Eli Zaretskii CC: gdb-patches@sourceware.org, areis@redhat.com Subject: Re: [PATCH 3/4] GDBServer: introduce --server-stderr command line option References: <1426905265-8495-1-git-send-email-crosa@redhat.com> <1426905265-8495-4-git-send-email-crosa@redhat.com> <83384yvjr0.fsf@gnu.org> In-Reply-To: <83384yvjr0.fsf@gnu.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00734.txt.bz2 On 03/21/2015 05:26 AM, Eli Zaretskii wrote: >> From: Cleber Rosa >> Cc: crosa@redhat.com, areis@redhat.com >> Date: Fri, 20 Mar 2015 23:34:24 -0300 >> >> This command line option will redirect all of the gdbserver's own >> output (always sent to stderr) to a separate file. This feature >> makes it possible to distinguish between the inferior process stderr >> and gdbserver's own stderr. >> [...] >> +@cindex @option{--server-stderr}, @code{gdbserver} option >> +The @option{--server-stderr} option tells @code{gdbserver} that any content >> +that it would normally generate itself to its own @code{stderr} should be >> +redirected to another file. This is useful if you want to keep the >> +inferior @code{stderr} separate from the one generated by @code{gdbserver}. > Note how the text description of the rationale for the change you > posted to explain it to us is much more useful than the technical > description in the manual. Why not say in the manual what you told > us? > > From the user POV, the fact that gdbserver uses stderr for its own > output is an implementation detail that is almost unimportant. (It > could be important for redirection purposes, but the command-line > option you introduce eliminates the need to redirect in most cases, > right?) What _is_ important is that gdbserver's own output will be > redirected to that file, and that important information gets lost in > the confusing "it would normally generate itself to its own 'stderr'", > which leaves unanswered the question what part of gdbserver's output > is "normally" excluded from that. Your comments make a lot of sense. Indeed using "normally" introduces unnecessary doubt and confusion. Also, the user friendlier text in the commit message won't reach users. Good catches! > > For this reason, I suggest to name the option "--server-output". Agreed. BTW, to keep a more obvious correlation between variable name (server_stderr) and command line option (now --server-output), I'm going to rename the former unless someone feels strongly against it. > > And I think we want a NEWS entry for this change. OK. > >> +@item --server-stderr >> +Instruct @code{gdbserver} to redirect its own @code{stderr} to another >> +file. > The option requires an argument, so the argument should be mentioned > with the option and referenced in the text that describes it. Sure, I also feel an example could help. How do you feel about this: @cindex @option{--server-output}, @code{gdbserver} option The @option{--server-output=path} option tells @code{gdbserver} to send all its output to a file given by @var{path}. This can be useful, for instance, if you need to collect the server output and/or the inferior output, but want to keep them separate: @smallexample $ gdbserver --server-output=server.log :2222 testprog >test.out 2>test.err @end smallexample > >> +static int >> +set_server_stderr (char *path) >> +{ >> + FILE *tmp; >> + >> + tmp = fopen (path, "w"); >> + if (tmp == NULL) >> + { >> + fprintf (stderr, >> + "Could not open server stderr file '%s': %s\n", >> + path, strerror(errno)); >> + return -1; >> + } >> + server_stderr = tmp; >> + return 0; >> +} >> + >> void >> monitor_show_help (void) >> { >> @@ -3017,6 +3036,7 @@ gdbserver_usage (FILE *stream) >> " none\n" >> " timestamp\n" >> " --remote-debug Enable remote protocol debugging output.\n" >> + " --server-stderr=PATH Redirect server's STDERR to file at PATH.\n" >> " --version Display version information and exit.\n" >> " --wrapper WRAPPER -- Run WRAPPER to start new programs.\n" >> " --once Exit after the first connection has " >> @@ -3186,7 +3206,13 @@ captured_main (int argc, char *argv[]) >> >> while (*next_arg != NULL && **next_arg == '-') >> { >> - if (strcmp (*next_arg, "--version") == 0) >> + if (strncmp (*next_arg, "--server-stderr=", >> + sizeof ("--server-stderr=") - 1) == 0) >> + { >> + char *path = *next_arg + (sizeof ("--server-stderr=") - 1); >> + set_server_stderr (path); >> + } >> + else if (strcmp (*next_arg, "--version") == 0) > AFAIK, GNU Coding Standards frown on using "path" for anything that is > not PATH-style list of directories. So please use "file" or "file > name" here. I could not find a mention on the GNU Coding Standards manual itself, but yeah, it may be (informally?) frowned upon as it's indeed confusing. > > Thanks. Thank you for the comments and very valid points!