CYGWIN file input redirection

  > Date: Sat, 15 Oct 2016 14:33:13 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: samsurfer117@gmail.com
> 
> > Date: Fri, 30 Sep 2016 12:22:13 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > CC: gdb-patches@sourceware.org
> > 
> > > How can we fix this bug?
> > 
> > Someone needs to write and submit the code to (a) parse the command
> > line, like "run < foo >> bar 2> baz" and glean the redirections from
> > it; and (b) actually implement the redirection in the CreateProcess
> > call.
> 
> Guess what? someone just did.
> 
> The patches below implement redirection support for native debugging
> of MinGW programs.  (I reused some of the code I wrote years ago for
> the DJGPP project.)
> 
> OK to push to master?

Sorry, forgot to attach the ChangeLog entries.  Here's the patch again
with the logs:

2016-10-15  Eli Zaretskii  <eliz@gnu.org>

	Support command-line redirection for Windows native debugging

	* windows-nat.c (redir_open, redir_set_redirection)
	(redirect_inferior_handles) [!__CYGWIN__]: New functions for
	redirecting standard handles of the inferior.
	(windows_create_inferior) [!__CYGWIN__]: Use them to redirect
	standard handles of the inferior based on redirection symbols on
	the command line.
  

Ping!  Is this OK to push to master, please?

> Date: Sat, 15 Oct 2016 15:39:42 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: samsurfer117@gmail.com
> 
> > Date: Sat, 15 Oct 2016 14:33:13 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > CC: samsurfer117@gmail.com
> > 
> > > Date: Fri, 30 Sep 2016 12:22:13 +0300
> > > From: Eli Zaretskii <eliz@gnu.org>
> > > CC: gdb-patches@sourceware.org
> > > 
> > > > How can we fix this bug?
> > > 
> > > Someone needs to write and submit the code to (a) parse the command
> > > line, like "run < foo >> bar 2> baz" and glean the redirections from
> > > it; and (b) actually implement the redirection in the CreateProcess
> > > call.
> > 
> > Guess what? someone just did.
> > 
> > The patches below implement redirection support for native debugging
> > of MinGW programs.  (I reused some of the code I wrote years ago for
> > the DJGPP project.)
> > 
> > OK to push to master?
> 
> Sorry, forgot to attach the ChangeLog entries.  Here's the patch again
> with the logs:
> 
> 2016-10-15  Eli Zaretskii  <eliz@gnu.org>
> 
> 	Support command-line redirection for Windows native debugging
> 
> 	* windows-nat.c (redir_open, redir_set_redirection)
> 	(redirect_inferior_handles) [!__CYGWIN__]: New functions for
> 	redirecting standard handles of the inferior.
> 	(windows_create_inferior) [!__CYGWIN__]: Use them to redirect
> 	standard handles of the inferior based on redirection symbols on
> 	the command line.
> 
> --- gdb/windows-nat.c~1	2016-10-09 12:37:04.538125000 +0300
> +++ gdb/windows-nat.c	2016-10-15 14:27:51.966125000 +0300
> @@ -2054,6 +2054,166 @@ clear_win32_environment (char **env)
>  }
>  #endif
>  
> +#ifndef __CYGWIN__
> +
> +/* Support routines for redirecting standard handles of the inferior.  */
> +
> +static int
> +redir_open (const char *redir_string, HANDLE *inp, HANDLE *out, HANDLE *err)
> +{
> +  HANDLE *hdl;
> +  int mode, fd;
> +  const char *fname = redir_string + 1;;
> +
> +  switch (*redir_string)
> +    {
> +    case '<':
> +      hdl = inp;
> +      mode = O_RDONLY;
> +      break;
> +    case '1': case '2':
> +      fname++;
> +      /* FALLTHROUGH */
> +    case '>':
> +      hdl = (*redir_string == '2') ? err : out;
> +      mode = O_WRONLY | O_CREAT;
> +      if (*fname == '>')
> +	{
> +	  fname++;
> +	  mode |= O_APPEND;
> +	}
> +      else
> +	mode |= O_TRUNC;
> +      break;
> +    default:
> +      return -1;
> +    }
> +  fname++;	/* skip the separator space */
> +  fd = _open (fname, mode, _S_IREAD | _S_IWRITE);
> +  if (fd < 0)
> +    return -1;
> +  /* _open just sets a flag for O_APPEND, which won't be passed to the
> +     inferior, so we need to actually move the file pointer.  */
> +  if ((mode & O_APPEND) != 0)
> +    _lseek (fd, 0L, SEEK_END);
> +  *hdl = (HANDLE)_get_osfhandle (fd);
> +  return 0;
> +}
> +
> +static int
> +redir_set_redirection (const char *s, HANDLE *inp, HANDLE *out, HANDLE *err)
> +{
> +  char buf[__PMAX + 5];	/* extra space for redirection string */
> +  char *d = buf;
> +  const char *start = s;
> +  int quote = 0;
> +
> +  *d++ = *s++;	/* copy the 1st character, < or > or a digit */
> +  if ((*start == '>' || *start == '1' || *start == '2')
> +      && *s == '>')
> +    {
> +      *d++ = *s++;
> +      if (*s == '>' && *start != '>')
> +	*d++ = *s++;
> +    }
> +  while (isspace (*s))	/* skip whitespace before file name */
> +    s++;
> +  *d++ = ' ';		/* separate file name with a single space */
> +
> +  /* Copy the file name.  */
> +  while (*s)
> +    {
> +      /* Remove quoting characters from the file name in buf[].  */
> +      if (*s == '"')	/* could support '..' quoting here */
> +	{
> +	  if (!quote)
> +	    quote = *s++;
> +	  else if (*s == quote)
> +	    {
> +	      quote = 0;
> +	      s++;
> +	    }
> +	  else
> +	    *d++ = *s++;
> +	}
> +      else if (*s == '\\')
> +	{
> +	  if (s[1] == '"')	/* could support '..' here */
> +	    s++;
> +	  *d++ = *s++;
> +	}
> +      else if (isspace (*s) && !quote)
> +	break;
> +      else
> +	*d++ = *s++;
> +      if (d - buf >= sizeof (buf) - 1)
> +	{
> +	  errno = ENAMETOOLONG;
> +	  return 0;
> +	}
> +    }
> +  *d = '\0';
> +
> +  /* Windows doesn't allow redirection characters in file names, so we
> +     can bail out early if they use them, or if there's no target file
> +     name after the redirection symbol.  */
> +  if (d[-1] == '>' || d[-1] == '<')
> +    {
> +      errno = ENOENT;
> +      return 0;
> +    }
> +  if (redir_open (buf, inp, out, err) == 0)
> +    return s - start;
> +  return 0;
> +}
> +
> +static bool
> +redirect_inferior_handles (const char *cmd_orig, char *cmd,
> +			   HANDLE *inp, HANDLE *out, HANDLE *err)
> +{
> +  const char *s = cmd_orig;
> +  char *d = cmd;
> +  int quote = 0;
> +  bool retval = false;
> +
> +  while (isspace (*s))
> +    *d++ = *s++;
> +
> +  while (*s)
> +    {
> +      if (*s == '"')	/* could also support '..' quoting here */
> +	{
> +	  if (!quote)
> +	    quote = *s;
> +	  else if (*s == quote)
> +	    quote = 0;
> +	}
> +      else if (*s == '\\')
> +	{
> +	  if (s[1] == '"')	/* escaped quote char */
> +	    s++;
> +	}
> +      else if (!quote)
> +	{
> +	  if (*s == '<' || *s == '>'
> +	      || ((*s == '1' || *s == '2') && s[1] == '>'))
> +	    {
> +	      int skip = redir_set_redirection (s, inp, out, err);
> +
> +	      if (skip <= 0)
> +		return false;
> +	      retval = true;
> +	      s += skip;
> +	    }
> +	}
> +      if (*s)
> +	*d++ = *s++;
> +    }
> +  *d = '\0';
> +  return retval;
> +}
> +#endif	/* !__CYGWIN__ */
> +
>  /* Start an inferior windows 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.
> @@ -2076,20 +2236,24 @@ windows_create_inferior (struct target_o
>    size_t len;
>    int tty;
>    int ostdin, ostdout, ostderr;
> -#else
> +#else  /* !__CYGWIN__ */
>    char real_path[__PMAX];
>    char shell[__PMAX]; /* Path to shell */
>    char *toexec;
> -  char *args;
> -  size_t args_len;
> -  HANDLE tty;
> +  char *args, *allargs_copy;
> +  size_t args_len, allargs_len;
> +  HANDLE tty = INVALID_HANDLE_VALUE;
> +  HANDLE inf_stdin = INVALID_HANDLE_VALUE;
> +  HANDLE inf_stdout = INVALID_HANDLE_VALUE;
> +  HANDLE inf_stderr = INVALID_HANDLE_VALUE;
> +  bool redirected = false;
>    char *w32env;
>    char *temp;
>    size_t envlen;
>    int i;
>    size_t envsize;
>    char **env;
> -#endif
> +#endif	/* !__CYGWIN__ */
>    PROCESS_INFORMATION pi;
>    BOOL ret;
>    DWORD flags = 0;
> @@ -2121,7 +2285,7 @@ windows_create_inferior (struct target_o
>  	error (_("Error starting executable: %d"), errno);
>        cygallargs = (wchar_t *) alloca (len * sizeof (wchar_t));
>        mbstowcs (cygallargs, allargs, len);
> -#else
> +#else  /* !__USEWIDE */
>        cygallargs = allargs;
>  #endif
>      }
> @@ -2137,12 +2301,12 @@ windows_create_inferior (struct target_o
>  	    + mbstowcs (NULL, allargs, 0) + 2;
>        cygallargs = (wchar_t *) alloca (len * sizeof (wchar_t));
>        swprintf (cygallargs, len, L" -c 'exec %s %s'", exec_file, allargs);
> -#else
> +#else  /* !__USEWIDE */
>        len = (sizeof (" -c 'exec  '") + strlen (exec_file)
>  	     + strlen (allargs) + 2);
>        cygallargs = (char *) alloca (len);
>        xsnprintf (cygallargs, len, " -c 'exec %s %s'", exec_file, allargs);
> -#endif
> +#endif	/* __USEWIDE */
>        toexec = shell;
>        flags |= DEBUG_PROCESS;
>      }
> @@ -2153,12 +2317,12 @@ windows_create_inferior (struct target_o
>    wcscpy (args, toexec);
>    wcscat (args, L" ");
>    wcscat (args, cygallargs);
> -#else
> +#else  /* !__USEWIDE */
>    args = (cygwin_buf_t *) alloca (strlen (toexec) + strlen (cygallargs) + 2);
>    strcpy (args, toexec);
>    strcat (args, " ");
>    strcat (args, cygallargs);
> -#endif
> +#endif	/* !__USEWIDE */
>  
>  #ifdef CW_CVT_ENV_TO_WINENV
>    /* First try to create a direct Win32 copy of the POSIX environment. */
> @@ -2167,7 +2331,7 @@ windows_create_inferior (struct target_o
>      flags |= CREATE_UNICODE_ENVIRONMENT;
>    else
>      /* If that fails, fall back to old method tweaking GDB's environment. */
> -#endif
> +#endif	/* CW_CVT_ENV_TO_WINENV */
>      {
>        /* Reset all Win32 environment variables to avoid leftover on next run. */
>        clear_win32_environment (environ);
> @@ -2232,21 +2396,26 @@ windows_create_inferior (struct target_o
>        close (ostdout);
>        close (ostderr);
>      }
> -#else
> -  toexec = exec_file;
> -  /* Build the command line, a space-separated list of tokens where
> -     the first token is the name of the module to be executed.
> -     To avoid ambiguities introduced by spaces in the module name,
> -     we quote it.  */
> -  args_len = strlen (toexec) + 2 /* quotes */ + strlen (allargs) + 2;
> -  args = (char *) alloca (args_len);
> -  xsnprintf (args, args_len, "\"%s\" %s", toexec, allargs);
> -
> -  flags |= DEBUG_ONLY_THIS_PROCESS;
> -
> -  if (!inferior_io_terminal)
> -    tty = INVALID_HANDLE_VALUE;
> -  else
> +#else  /* !__CYGWIN__ */
> +  allargs_len = strlen (allargs);
> +  allargs_copy = strcpy ((char *)alloca (allargs_len + 1), allargs);
> +  if (strpbrk (allargs_copy, "<>"))
> +    {
> +      int e = errno;
> +      errno = 0;
> +      redirected =
> +	redirect_inferior_handles (allargs, allargs_copy,
> +				   &inf_stdin, &inf_stdout, &inf_stderr);
> +      if (errno)
> +	warning (_("Error in redirection: %s."), strerror (errno));
> +      else
> +	errno = e;
> +      allargs_len = strlen (allargs_copy);
> +    }
> +  if (inferior_io_terminal
> +      && !(inf_stdin != INVALID_HANDLE_VALUE
> +	   && inf_stdout != INVALID_HANDLE_VALUE
> +	   && inf_stderr != INVALID_HANDLE_VALUE))
>      {
>        SECURITY_ATTRIBUTES sa;
>        sa.nLength = sizeof(sa);
> @@ -2257,15 +2426,32 @@ windows_create_inferior (struct target_o
>        if (tty == INVALID_HANDLE_VALUE)
>  	warning (_("Warning: Failed to open TTY %s, error %#x."),
>  		 inferior_io_terminal, (unsigned) GetLastError ());
> -      else
> -	{
> -	  si.hStdInput = tty;
> -	  si.hStdOutput = tty;
> -	  si.hStdError = tty;
> -	  si.dwFlags |= STARTF_USESTDHANDLES;
> -	}
> +    }
> +  if (redirected || tty != INVALID_HANDLE_VALUE)
> +    {
> +      si.hStdInput = inf_stdin == INVALID_HANDLE_VALUE ? tty : inf_stdin;
> +      if (si.hStdInput == INVALID_HANDLE_VALUE)
> +	si.hStdInput = GetStdHandle (STD_INPUT_HANDLE);
> +      si.hStdOutput = inf_stdout == INVALID_HANDLE_VALUE ? tty : inf_stdout;
> +      if (si.hStdOutput == INVALID_HANDLE_VALUE)
> +	si.hStdOutput = GetStdHandle (STD_OUTPUT_HANDLE);
> +      si.hStdError = inf_stderr == INVALID_HANDLE_VALUE ? tty : inf_stderr;
> +      if (si.hStdError == INVALID_HANDLE_VALUE)
> +	si.hStdError = GetStdHandle (STD_ERROR_HANDLE);
> +      si.dwFlags |= STARTF_USESTDHANDLES;
>      }
>  
> +  toexec = exec_file;
> +  /* Build the command line, a space-separated list of tokens where
> +     the first token is the name of the module to be executed.
> +     To avoid ambiguities introduced by spaces in the module name,
> +     we quote it.  */
> +  args_len = strlen (toexec) + 2 /* quotes */ + allargs_len + 2;
> +  args = (char *) alloca (args_len);
> +  xsnprintf (args, args_len, "\"%s\" %s", toexec, allargs_copy);
> +
> +  flags |= DEBUG_ONLY_THIS_PROCESS;
> +
>    /* CreateProcess takes the environment list as a null terminated set of
>       strings (i.e. two nulls terminate the list).  */
>  
> @@ -2304,7 +2490,13 @@ windows_create_inferior (struct target_o
>  			&pi);
>    if (tty != INVALID_HANDLE_VALUE)
>      CloseHandle (tty);
> -#endif
> +  if (inf_stdin != INVALID_HANDLE_VALUE)
> +    CloseHandle (inf_stdin);
> +  if (inf_stdout != INVALID_HANDLE_VALUE)
> +    CloseHandle (inf_stdout);
> +  if (inf_stderr != INVALID_HANDLE_VALUE)
> +    CloseHandle (inf_stderr);
> +#endif	/* !__CYGWIN__ */
>  
>    if (!ret)
>      error (_("Error creating process %s, (error %u)."),
>
  

On 10/22/2016 10:30 AM, Eli Zaretskii wrote:
> Ping!  Is this OK to push to master, please?

I think this should have a NEWS entry.  

Since on Windows, programs are expected to handle redirection
themselves, isn't there a chance that this will make it a bit
harder to debug such redirection code in inferiors that
do it themselves?  E.g., imagine if you're debugging cmd.exe's
redirection code.  Or maybe even debugging this new code
in gdb itself?  Do we need a knob to be able to disable
this feature?

I didn't bother to try to understand the patch completely.
I just trust that it works.  It's fine with me to put it in.

It'd be nice to add comments mentioning what syntax works and doesn't
work.  Is there something users should know about syntax, that
should be added to the manual?

Ideally some test would "prove" this all works, which would
also make it possible to more confidently change the implementation
later on if we find it necessary.  It's been years since I'd tried to
run the testsuite for mingw gdb (under cygwin/msys/msys2 of course)
and I have no idea whether people are doing that nowadays.  I have
the impression that maybe no one is..  And then, I can't seem to
find any existing test that exercises redirection, even
on Unix...  :-/  Oh well.

Please see below.

>> 2016-10-15  Eli Zaretskii  <eliz@gnu.org>
>>
>> 	Support command-line redirection for Windows native debugging
>>
>> 	* windows-nat.c (redir_open, redir_set_redirection)
>> 	(redirect_inferior_handles) [!__CYGWIN__]: New functions for
>> 	redirecting standard handles of the inferior.
>> 	(windows_create_inferior) [!__CYGWIN__]: Use them to redirect
>> 	standard handles of the inferior based on redirection symbols on
>> 	the command line.
>>
>> --- gdb/windows-nat.c~1	2016-10-09 12:37:04.538125000 +0300
>> +++ gdb/windows-nat.c	2016-10-15 14:27:51.966125000 +0300
>> @@ -2054,6 +2054,166 @@ clear_win32_environment (char **env)
>>  }
>>  #endif
>>  

>> +  fname++;	/* skip the separator space */
>> +  fd = _open (fname, mode, _S_IREAD | _S_IWRITE);
>> +  if (fd < 0)
>> +    return -1;
>> +  /* _open just sets a flag for O_APPEND, which won't be passed to the
>> +     inferior, so we need to actually move the file pointer.  */
>> +  if ((mode & O_APPEND) != 0)
>> +    _lseek (fd, 0L, SEEK_END);
>> +  *hdl = (HANDLE)_get_osfhandle (fd);

GDB puts a space after casts:

  *hdl = (HANDLE) _get_osfhandle (fd);


>> +  return 0;
>> +}
>> +
>> +static int
>> +redir_set_redirection (const char *s, HANDLE *inp, HANDLE *out, HANDLE *err)

It'd be good to have an intro comment for every new function describing
the interface.  What does this return, for example.

>> +#else  /* !__CYGWIN__ */
>> +  allargs_len = strlen (allargs);
>> +  allargs_copy = strcpy ((char *)alloca (allargs_len + 1), allargs);

Space after cast.

We should really get rid of all unbounded allocas, but it's
preexisting, so you get a pass. :-)

Nit, we already know the length after the strlen call, so
instead of strcpy it could be memcpy.


>> +  if (strpbrk (allargs_copy, "<>"))

      if (strpbrk (allargs_copy, "<>") != NULL)


>> +    {
>> +      int e = errno;
>> +      errno = 0;
>> +      redirected =
>> +	redirect_inferior_handles (allargs, allargs_copy,
>> +				   &inf_stdin, &inf_stdout, &inf_stderr);
>> +      if (errno)
>> +	warning (_("Error in redirection: %s."), strerror (errno));
>> +      else
>> +	errno = e;
>> +      allargs_len = strlen (allargs_copy);
>> +    }
>> +  if (inferior_io_terminal
>> +      && !(inf_stdin != INVALID_HANDLE_VALUE
>> +	   && inf_stdout != INVALID_HANDLE_VALUE
>> +	   && inf_stderr != INVALID_HANDLE_VALUE))

I find these double-negatives hard to read.  I'd suggest:

  if (inferior_io_terminal
      && (inf_stdin == INVALID_HANDLE_VALUE
	  || inf_stdout == INVALID_HANDLE_VALUE
	  || inf_stderr == INVALID_HANDLE_VALUE))

Thanks,
Pedro Alves
  

> Cc: samsurfer117@gmail.com
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 24 Oct 2016 13:36:34 +0100
> 
> On 10/22/2016 10:30 AM, Eli Zaretskii wrote:
> > Ping!  Is this OK to push to master, please?
> 
> I think this should have a NEWS entry.  

Yes, I thought about this myself.  Will do.

> Since on Windows, programs are expected to handle redirection
> themselves

??? No, they don't (I believe you thought about wildcard expansion?).
Redirection on Windows shell command line works like on Unix: the
shell invokes the program with standard streams redirected according
to the redirection symbols.  Which is exactly what this code emulates
inside GDB.

> isn't there a chance that this will make it a bit
> harder to debug such redirection code in inferiors that
> do it themselves?  E.g., imagine if you're debugging cmd.exe's
> redirection code.  Or maybe even debugging this new code
> in gdb itself?  Do we need a knob to be able to disable
> this feature?

I'm not sure I see the problem.  If you debug GDB's redirection code,
you will probably not redirect that GDB's standard streams, right?
IOW, you would

  > gdb ./gdb.exe
  (top-gdb) run foo.exe
  (gdb) run < bar > baz

Right?  Or did you mean something else?

> It'd be nice to add comments mentioning what syntax works and doesn't
> work.  Is there something users should know about syntax, that
> should be added to the manual?

The code is supposed to support everything cmd.exe supports, and
nothing else.  Ah, I see I didn't implement the likes of "2>&1".  Will
do.

The only thing beyond what cmd.exe supports is that one can use
Unix-style forward slashes in redirected file names, which comes in
handy because GDB's file-name completion helps.  Not sure if this
should be in the manual.  WDYT?

> Ideally some test would "prove" this all works, which would
> also make it possible to more confidently change the implementation
> later on if we find it necessary.  It's been years since I'd tried to
> run the testsuite for mingw gdb (under cygwin/msys/msys2 of course)
> and I have no idea whether people are doing that nowadays.  I have
> the impression that maybe no one is..  And then, I can't seem to
> find any existing test that exercises redirection, even
> on Unix...  :-/  Oh well.

Right.  I don't have a setup for running the test suite.  I did, of
course, test the code manually.

> >> +  if (inferior_io_terminal
> >> +      && !(inf_stdin != INVALID_HANDLE_VALUE
> >> +	   && inf_stdout != INVALID_HANDLE_VALUE
> >> +	   && inf_stderr != INVALID_HANDLE_VALUE))
> 
> I find these double-negatives hard to read.  I'd suggest:
> 
>   if (inferior_io_terminal
>       && (inf_stdin == INVALID_HANDLE_VALUE
> 	  || inf_stdout == INVALID_HANDLE_VALUE
> 	  || inf_stderr == INVALID_HANDLE_VALUE))

Interesting, I actually find the latter harder to grasp.  The former
says "if not all of the handles are valid".  I can add a macro
VALID_HANDLE, if that would help.

Thanks for the review, I will add the missing bits and fixes and
resubmit.
  

On 10/24/2016 02:24 PM, Eli Zaretskii wrote:

> I'm not sure I see the problem.  If you debug GDB's redirection code,
> you will probably not redirect that GDB's standard streams, right?
> IOW, you would
> 
>   > gdb ./gdb.exe
>   (top-gdb) run foo.exe
>   (gdb) run < bar > baz
> 
> Right?  Or did you mean something else?

OK.  I guess I was confused.  So let's just forget my comment.

> 
>> It'd be nice to add comments mentioning what syntax works and doesn't
>> work.  Is there something users should know about syntax, that
>> should be added to the manual?
> 
> The code is supposed to support everything cmd.exe supports, and
> nothing else.  Ah, I see I didn't implement the likes of "2>&1".  Will
> do.
> 
> The only thing beyond what cmd.exe supports is that one can use
> Unix-style forward slashes in redirected file names, which comes in
> handy because GDB's file-name completion helps.  Not sure if this
> should be in the manual.  WDYT?

Seems fine not to mention that, since gdb supports forward slashes
throughout, anyway.  If it didn't support them here probably someone 
would call it a bug.

> 
>> Ideally some test would "prove" this all works, which would
>> also make it possible to more confidently change the implementation
>> later on if we find it necessary.  It's been years since I'd tried to
>> run the testsuite for mingw gdb (under cygwin/msys/msys2 of course)
>> and I have no idea whether people are doing that nowadays.  I have
>> the impression that maybe no one is..  And then, I can't seem to
>> find any existing test that exercises redirection, even
>> on Unix...  :-/  Oh well.
> 
> Right.  I don't have a setup for running the test suite.  I did, of
> course, test the code manually.
> 
>>>> +  if (inferior_io_terminal
>>>> +      && !(inf_stdin != INVALID_HANDLE_VALUE
>>>> +	   && inf_stdout != INVALID_HANDLE_VALUE
>>>> +	   && inf_stderr != INVALID_HANDLE_VALUE))
>>
>> I find these double-negatives hard to read.  I'd suggest:
>>
>>   if (inferior_io_terminal
>>       && (inf_stdin == INVALID_HANDLE_VALUE
>> 	  || inf_stdout == INVALID_HANDLE_VALUE
>> 	  || inf_stderr == INVALID_HANDLE_VALUE))
> 
> Interesting, I actually find the latter harder to grasp.  The former
> says "if not all of the handles are valid".  I can add a macro
> VALID_HANDLE, if that would help.

Leave it be as you had it then.  It was just a minor suggestion.

> 
> Thanks for the review, I will add the missing bits and fixes and
> resubmit.

Thanks,
Pedro Alves