[3/4] GDBServer: introduce --server-stderr command line option

Message ID 1426905265-8495-4-git-send-email-crosa@redhat.com
State New, archived
Headers

Commit Message

Cleber Rosa March 21, 2015, 2:34 a.m. UTC
  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.

gdb/doc/ChangeLog:
2015-03-20  Cleber Rosa  <crosa@redhat.com>

	* gdb.texinfo (info): Added section on command line and monitor
	commands.
	(gdbserver man): Added section on new command line option.

gdb/gdbserver/ChangeLog:
2015-03-20  Cleber Rosa  <crosa@redhat.com>

	* server.c (set_server_stderr): New utility function.
	(gdbserver_usage): Add help on '--server-stderr' option.
	(captured_main): Add command line option parsing for
	'--server-stderr'. Replace stderr with gdbserver's own
	stderr.
---
 gdb/doc/gdb.texinfo    | 10 ++++++++++
 gdb/gdbserver/server.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)
  

Comments

Eli Zaretskii March 21, 2015, 8:26 a.m. UTC | #1
> From: Cleber Rosa <crosa@redhat.com>
> 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.

For this reason, I suggest to name the option "--server-output".

And I think we want a NEWS entry for this change.

> +@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.

> +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.

Thanks.
  
Cleber Rosa March 23, 2015, 6:48 p.m. UTC | #2
On 03/21/2015 05:26 AM, Eli Zaretskii wrote:
>> From: Cleber Rosa <crosa@redhat.com>
>> 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!
  
Eli Zaretskii March 23, 2015, 7:12 p.m. UTC | #3
> Date: Mon, 23 Mar 2015 15:48:29 -0300
> From: Cleber Rosa <crosa@redhat.com>
> CC: gdb-patches@sourceware.org, areis@redhat.com
> 
> >> +@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

@option{--server-output=@var{path}} (and once again, please use
"file" or "filename", not "path").

Also, what happened to the @item?

> all its output to a file given by @var{path}. This can be useful, for 
                                              ^^
Two spaces between sentences.

> @smallexample
> $ gdbserver --server-output=server.log :2222 testprog >test.out 2>test.err
> @end smallexample

This line is too long; either try to make it shorter, e.g., by using
shorter file/program names, or break it into 2 lines.

Otherwise, this is fine, thanks.
> > 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, 

It's in the node "GNU Manuals":

     Please do not use the term "pathname" that is used in Unix
  documentation; use "file name" (two words) instead.  We use the term
  "path" only for search paths, which are lists of directory names.
  
Cleber Rosa March 23, 2015, 8:34 p.m. UTC | #4
On 03/23/2015 04:12 PM, Eli Zaretskii wrote:
>> Date: Mon, 23 Mar 2015 15:48:29 -0300
>> From: Cleber Rosa <crosa@redhat.com>
>> CC: gdb-patches@sourceware.org, areis@redhat.com
>>
>>>> +@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
> @option{--server-output=@var{path}} (and once again, please use
> "file" or "filename", not "path").

Sorry, I missed that in the first reply but it's covered in the updated 
patches (and inline for information purposes):

@cindex @option{--server-output}, @code{gdbserver} option
The @option{--server-output=@var{file}} option tells @code{gdbserver} to 
send
all its output to a file given by @var{file}.  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=log :2222 bin >bin.out 2>bin.err
@end smallexample

>
> Also, what happened to the @item?

@item --server-output=file
Instruct @code{gdbserver} to redirect its own output to @var{file}.

Which renders as:

        --server-output=file
            Instruct "gdbserver" to redirect its own output to file.

Too simplistic or is that OK?

>
>> all its output to a file given by @var{path}. This can be useful, for
>                                                ^^
> Two spaces between sentences.
>
>> @smallexample
>> $ gdbserver --server-output=server.log :2222 testprog >test.out 2>test.err
>> @end smallexample
> This line is too long; either try to make it shorter, e.g., by using
> shorter file/program names, or break it into 2 lines.

OK, how about (repeated from earlier):

@smallexample
$ gdbserver --server-output=log :2222 bin >bin.out 2>bin.err
@end smallexample

>
> Otherwise, this is fine, thanks.
>>> 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,
> It's in the node "GNU Manuals":
>
>       Please do not use the term "pathname" that is used in Unix
>    documentation; use "file name" (two words) instead.  We use the term
>    "path" only for search paths, which are lists of directory names.

Oh, thanks for the pointer and for having the manual on (brain) cache 
and catching that!
  
Eli Zaretskii March 23, 2015, 8:43 p.m. UTC | #5
> Date: Mon, 23 Mar 2015 17:34:46 -0300
> From: Cleber Rosa <crosa@redhat.com>
> CC: gdb-patches@sourceware.org, areis@redhat.com
> 
> > Also, what happened to the @item?
> 
> @item --server-output=file
> Instruct @code{gdbserver} to redirect its own output to @var{file}.
> 
> Which renders as:
> 
>         --server-output=file
>             Instruct "gdbserver" to redirect its own output to file.
> 
> Too simplistic or is that OK?

OK.

> >> @smallexample
> >> $ gdbserver --server-output=server.log :2222 testprog >test.out 2>test.err
> >> @end smallexample
> > This line is too long; either try to make it shorter, e.g., by using
> > shorter file/program names, or break it into 2 lines.
> 
> OK, how about (repeated from earlier):
> 
> @smallexample
> $ gdbserver --server-output=log :2222 bin >bin.out 2>bin.err
> @end smallexample

That's fine.

Thanks.
  

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 552da31..2834794 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19205,6 +19205,12 @@  The @option{--remote-debug} option tells @code{gdbserver} to display
 remote protocol debug output.  These options are intended for
 @code{gdbserver} development and for bug reports to the developers.
 
+@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}.
+
 @cindex @option{--debug-format}, @code{gdbserver} option
 The @option{--debug-format=option1[,option2,...]} option tells
 @code{gdbserver} to include additional information in each output.
@@ -40886,6 +40892,10 @@  Instruct @code{gdbserver} to include extra information in each line
 of debugging output.
 @xref{Other Command-Line Arguments for gdbserver}.
 
+@item --server-stderr
+Instruct @code{gdbserver} to redirect its own @code{stderr} to another
+file.
+
 @item --wrapper
 Specify a wrapper to launch programs
 for debugging.  The option should be followed by the name of the
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 9dec972..db26f24 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -723,6 +723,25 @@  get_features_xml (const char *annex)
   return NULL;
 }
 
+/* Opens a file and redirects the server own stderr to it */
+
+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)
 	{
 	  gdbserver_version ();
 	  exit (0);