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

Cleber Rosa
2015-03-23T18:51:15Z

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!