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
signature.asc
Description: PGP signature