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

[Cleber Rosa <crosa@redhat.com>]

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!