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

[Cleber Rosa <crosa at redhat dot 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!