Re: [PATCH 1/4] GDB: inferior standard I/O redirection

[Pedro Alves <palves@redhat.com>]

On 10/05/2015 12:46 PM, Cleber Rosa wrote:

> +/* Helper function that attempts to open a file and if successful
> +   redirects another file descriptor to the one just opened.  If
> +   file_name is not given, this returns -1 as a simple way to flag
> +   a skip (user has not asked for a redirection).  Seems OK since
> +   this is a helper function.  */
> +
> +static int
> +set_std_io_helper (const char *file_name, int std_io_fd, int flags, mode_t mode)
> +{
> +  int fd;
> +
> +  if (file_name == NULL)
> +    return -1;
> +
> +  fd = open (file_name, flags, mode);
> +  if (fd < 0)
> +    return 0;
> +
> +  if (dup2 (fd, std_io_fd) == -1) {

{ goes on the next line, and the reindent block.

> +    close (fd);
> +    return 0;
> +  }
> +
> +  close (fd);
> +  return 1;
> +}
> +
> +/* Attempts to redirect stdin, stdout and stderr. If redirection was

Double-space after period.

> +   not requested by the user, it will be skipped in the helper function.
> +   In case of errors, each requested and failed redirection error will
> +   be passed on for individual error reporting (no details such as
> +   errno/perror though).  */
> +
> +#define SET_STDIO_ERROR_STDIN  0x01
> +#define SET_STDIO_ERROR_STDOUT 0x02
> +#define SET_STDIO_ERROR_STDERR 0x04
> +
> +static int
> +set_std_io (void)
> +{
> +  int result = 0;
> +  char *file_name = NULL;
> +
> +  if (set_std_io_helper (get_inferior_io_stdin(), 0,
> +                         O_RDONLY, S_IRUSR | S_IWUSR) == 0)
> +    result |= SET_STDIO_ERROR_STDIN;
> +
> +  if (set_std_io_helper (get_inferior_io_stdout(), 1,
> +                         O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR) == 0)
> +    result |= SET_STDIO_ERROR_STDOUT;
> +
> +  if (set_std_io_helper (get_inferior_io_stderr(), 2,
> +                         O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR) == 0)
> +    result |= SET_STDIO_ERROR_STDERR;

Shouldn't we specify O_APPEND?  I'm thinking of the case of starting
multiple inferiors simultaneously under gdb.  Not one after the other,
but really in parallel.  E.g., run&; add-inferior ...; inferior 2; run&; etc.

> +
> +  return result;
> +}
> +
>  /* Start an inferior Unix child process and sets inferior_ptid to its
>     pid.  EXEC_FILE is the file to run.  ALLARGS is a string containing
>     the arguments to the program.  ENV is the environment vector to
> @@ -141,6 +199,7 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>    struct inferior *inf;
>    int i;
>    int save_errno;
> +  int io_redir_errors = 0;
>  
>    /* If no exec file handed to us, get it from the exec-file command
>       -- with a good, common error message if none is specified.  */
> @@ -358,6 +417,28 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>           path to find $SHELL.  Rich Pixley says so, and I agree.  */
>        environ = env;
>  
> +      /* Sets the inferior process standard I/O (input, output, error)
> +	 redirection if set with the "set inferior std{in,out,err}"
> +	 commands.  */
> +      gdb_flush (gdb_stdout);
> +      gdb_flush (gdb_stderr);
> +      io_redir_errors = set_std_io();

Space before parens.  More instances below.

> +      if (io_redir_errors & SET_STDIO_ERROR_STDIN)
> +        fprintf_unfiltered (gdb_stderr,
> +                            "Could not set %s as stdin for %s\n",

Wrap user-visible strings in _() for i18n.  More instances below.

> +                            get_inferior_io_stdin(),
> +                            exec_file);
> +      if (io_redir_errors & SET_STDIO_ERROR_STDOUT)
> +        fprintf_unfiltered (gdb_stderr,
> +                            "Could not set %s as stdout for %s\n",
> +                            get_inferior_io_stdout(),
> +                            exec_file);
> +      if (io_redir_errors & SET_STDIO_ERROR_STDERR)
> +        fprintf_unfiltered (gdb_stderr,
> +                            "Could not set %s as stderr for %s\n",
> +                            get_inferior_io_stderr(),
> +                            exec_file);
> +
>        if (exec_fun != NULL)
>          (*exec_fun) (argv[0], argv, env);
>        else
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 4713490..9941a98 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -115,6 +115,11 @@ static char *inferior_args_scratch;
>  
>  static char *inferior_io_terminal_scratch;
>  
> +/* Scratch area where 'set inferior-{stdin,stdout,stderr}' will store
> +   user provided value.  */
> +
> +static char *inferior_io_std_scratch[3] = { NULL, NULL, NULL };
> +
>  /* Pid of our debugged inferior, or 0 if no inferior now.
>     Since various parts of infrun.c test this to see whether there is a program
>     being debugged it should be nonzero (currently 3 is used) for remote
> @@ -182,6 +187,117 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
>  		      "is \"%s\".\n"), inferior_io_terminal);
>  }
>  

New functions below need intro comments.  For extern functions,
use "/* See whatever.h.  */".

> +static void
> +set_inferior_io_helper (const char *file_name, int stdfd)
> +{
> +  gdb_assert (0 <= stdfd && stdfd <= 2);
> +  xfree (current_inferior ()->standard_io[stdfd]);
> +  current_inferior ()->standard_io[stdfd] =
> +    file_name != NULL ? xstrdup (file_name) : NULL;
> +}
> +
> +void
> +set_inferior_io_stdin (const char *file_name)
> +{
> +  set_inferior_io_helper(file_name, 0);

Space before parens.


> diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
> index 4395c98..2ae4f90 100644
> --- a/gdb/testsuite/gdb.base/default.exp
> +++ b/gdb/testsuite/gdb.base/default.exp
> @@ -798,6 +798,12 @@ gdb_test "thread find" "Command requires an argument." "thread find"
>  gdb_test "thread name" "No thread selected" "thread name"
>  #test tty
>  gdb_test "tty" "Argument required .filename to set it to\..*" "tty"
> +#test stdin
> +gdb_test "stdin" "Argument required .filename to set it to\..*" "stdin"
> +#test stdout
> +gdb_test "stdout" "Argument required .filename to set it to\..*" "stdout"
> +#test stderr
> +gdb_test "stderr" "Argument required .filename to set it to\..*" "stderr"
>  #test until "u" abbreviation
>  gdb_test "u" "The program is not being run." "until \"u\" abbreviation"
>  #test until
> 

It'd be great if we also tested that it actually works.

Thanks,
Pedro Alves