Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On Sep  2 07:11, Takashi Yano wrote:
> - Pseudo console support introduced by commit
>   169d65a5774acc76ce3f3feeedcbae7405aa9b57 has some bugs which
>   cause mismatch between state variables and real pseudo console
>   state regarding console attaching and r/w pipe switching. This
>   patch fixes this issue by redesigning the state management.
> ---
>  winsup/cygwin/dtable.cc           |  15 +-
>  winsup/cygwin/fhandler.h          |   6 +-
>  winsup/cygwin/fhandler_console.cc |   6 +-
>  winsup/cygwin/fhandler_tty.cc     | 415 ++++++++++++++++--------------
>  winsup/cygwin/fork.cc             |  24 +-
>  winsup/cygwin/spawn.cc            |  65 +++--
>  6 files changed, 289 insertions(+), 242 deletions(-)
> [...]
>  class fhandler_pty_slave: public fhandler_pty_common
>  {
>    HANDLE inuse;			// used to indicate that a tty is in use
>    HANDLE output_handle_cyg, io_handle_cyg;
> +  DWORD pidRestore;

Please don't use camelback.  s/pidRestore/pid_restore/g

> -	      HeapAlloc (GetProcessHeap (), 0, num * sizeof (DWORD));
> -  num = GetConsoleProcessList (list, num);
> +	      HeapAlloc (GetProcessHeap (), 0, (num + 16) * sizeof (DWORD));
> +  num = GetConsoleProcessList (list, num + 16);

You're still going to change that, right?

> @@ -855,26 +868,6 @@ fhandler_pty_slave::cleanup ()
>  int
>  fhandler_pty_slave::close ()
>  {
> -#if 0
> -  if (getPseudoConsole ())
> -    {
> -      INPUT_RECORD inp[128];
> -      DWORD n;
> -      PeekFunc =
> -	PeekConsoleInputA_Orig ? PeekConsoleInputA_Orig : PeekConsoleInput;
> -      PeekFunc (get_handle (), inp, 128, &n);
> -      bool pipe_empty = true;
> -      while (n-- > 0)
> -	if (inp[n].EventType == KEY_EVENT && inp[n].Event.KeyEvent.bKeyDown)
> -	  pipe_empty = false;
> -      if (pipe_empty)
> -	{
> -	  /* Flush input buffer */
> -	  size_t len = UINT_MAX;
> -	  read (NULL, len);
> -	}
> -    }
> -#endif

Ideally stuff like that should be in a separate code cleanup patch.

> -      Sleep (60); /* Wait for pty_master_fwd_thread() */
> +      Sleep (20); /* Wait for pty_master_fwd_thread() */

Isn't that a separate issue as well?  A separate patch may be in order
here, kind of like "Cygwin: pseudo console: reduce time sleeping ..."
with a short description why that makes sense?

> +	  /* If not attached this pseudo console, try to attach temporarily. */
                           ^^^^
                            to

> -	  get_ttyp ()->hPseudoConsole = NULL;
> +	  //get_ttyp ()->hPseudoConsole = NULL; // Do not clear for safty.

Why don't you just drop the line?

Other than that, the patch looks good.

However, I have a few questions in terms of the code in general, namely
in terms of

  ALWAYS_USE_PCON
  USE_API_HOOK
  USE_OWN_NLS_FUNC

Can you describe again why you introduced these macros?

In terms of USE_API_HOOK:

- Shouldn't the hook_api function be moved to hookapi.cc?

- Do we really want to hook every invocation of WriteFile/ReadFile?
  Doesn't that potentially slow down an exec'ed process a lot?
  We're still not using the NT functions throughout outside of the
  console/tty code.

In terms of USE_OWN_NLS_FUNC:

- Why do we need this function at all?  Can't this be handled by
  __loadlocale instead?  If not, what is __loadlocale missing to make
  this work without duplicating the function?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: signature.asc
Description: PGP signature