[PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
- When the I/O pipe is switched to the pseudo console side, the
  behaviour of Ctrl-C is unstable. This rarely happens, however,
  for example, shell sometimes crashes by Ctrl-C in that situation.
  This patch fixes that issue.

v3:
Fix mistake in v2.

v2:
Remove the code which accidentally clears ENABLE_ECHO_INPUT flag.

Takashi Yano (1):
  Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

 winsup/cygwin/fhandler.h      |   4 +-
 winsup/cygwin/fhandler_tty.cc |  33 +++++----
 winsup/cygwin/select.cc       |   2 +-
 winsup/cygwin/spawn.cc        | 128 +++++++++++++++++-----------------
 4 files changed, 89 insertions(+), 78 deletions(-)

--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
- When the I/O pipe is switched to the pseudo console side, the
  behaviour of Ctrl-C is unstable. This rarely happens, however,
  for example, shell sometimes crashes by Ctrl-C in that situation.
  This patch fixes that issue.
---
 winsup/cygwin/fhandler.h      |   4 +-
 winsup/cygwin/fhandler_tty.cc |  33 +++++----
 winsup/cygwin/select.cc       |   2 +-
 winsup/cygwin/spawn.cc        | 128 +++++++++++++++++-----------------
 4 files changed, 89 insertions(+), 78 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index e72e11f7a..1e3cada08 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2187,9 +2187,9 @@ class fhandler_pty_slave: public fhandler_pty_common
     get_ttyp ()->mask_switch_to_pcon = mask;
   }
   void fixup_after_attach (bool native_maybe);
-  pid_t get_pcon_pid (void)
+  pid_t getpgid (void)
   {
-    return get_ttyp ()->pcon_pid;
+    return get_ttyp ()->getpgid ();
   }
   bool is_line_input (void)
   {
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 2533e5618..f8d75fd92 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1018,20 +1018,26 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
       get_ttyp ()->need_clear_screen = false;
     }
 
-  if (ALWAYS_USE_PCON)
-    return;
   if (isHybrid)
-    {
-      this->set_switch_to_pcon ();
-      return;
-    }
+    this->set_switch_to_pcon ();
   if (get_ttyp ()->pcon_pid &&
       get_ttyp ()->pcon_pid != myself->pid &&
       kill (get_ttyp ()->pcon_pid, 0) == 0)
     /* There is a process which is grabbing pseudo console. */
     return;
-  if (get_ttyp ()->switch_to_pcon &&
-      get_ttyp ()->pcon_pid != myself->pid)
+  if (isHybrid)
+    {
+      if (ALWAYS_USE_PCON)
+ {
+  DWORD mode;
+  GetConsoleMode (get_handle (), &mode);
+  SetConsoleMode (get_handle (), mode & ~ENABLE_PROCESSED_INPUT);
+ }
+      get_ttyp ()->pcon_pid = 0;
+      init_console_handler (true);
+      return;
+    }
+  if (get_ttyp ()->switch_to_pcon)
     {
       DWORD mode;
       GetConsoleMode (get_handle (), &mode);
@@ -1040,6 +1046,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
     }
   get_ttyp ()->pcon_pid = 0;
   get_ttyp ()->switch_to_pcon = false;
+  init_console_handler (true);
 }
 
 void
@@ -1299,8 +1306,7 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
   if (ptr) /* Indicating not tcflush(). */
     {
       reset_switch_to_pcon ();
-      if (get_ttyp ()->pcon_pid != myself->pid)
- mask_switch_to_pcon (true);
+      mask_switch_to_pcon (true);
     }
 
   if (is_nonblocking () || !ptr) /* Indicating tcflush(). */
@@ -1420,7 +1426,7 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
     flags &= ~ENABLE_ECHO_INPUT;
   if ((get_ttyp ()->ti.c_lflag & ISIG) &&
       !(get_ttyp ()->ti.c_iflag & IGNBRK))
-    flags |= ENABLE_PROCESSED_INPUT;
+    flags |= ALWAYS_USE_PCON ? 0 : ENABLE_PROCESSED_INPUT;
   if (dwMode != flags)
     SetConsoleMode (get_handle (), flags);
   /* Read get_handle() instad of get_handle_cyg() */
@@ -2867,8 +2873,10 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe)
       get_ttyp ()->num_pcon_attached_slaves ++;
     }
  }
+      if (ALWAYS_USE_PCON && pcon_attached_to == get_minor ())
+ set_ishybrid_and_switch_to_pcon (get_output_handle ());
     }
-  if (pcon_attached_to == get_minor () && (native_maybe || ALWAYS_USE_PCON))
+  if (pcon_attached_to == get_minor () && native_maybe)
     {
       FlushConsoleInputBuffer (get_handle ());
       DWORD mode;
@@ -2883,6 +2891,7 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe)
   kill (get_ttyp ()->pcon_pid, 0) != 0)
  get_ttyp ()->pcon_pid = myself->pid;
       get_ttyp ()->switch_to_pcon = true;
+      init_console_handler(false);
     }
 }
 
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 4efc302df..3589ccabf 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -1294,7 +1294,7 @@ pty_slave_startup (select_record *me, select_stuff *stuff)
 {
   fhandler_base *fh = (fhandler_base *) me->fh;
   fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
-  if (me->read_selected && ptys->get_pcon_pid () != myself->pid)
+  if (me->read_selected)
     ptys->mask_switch_to_pcon (true);
 
   select_pipe_info *pi = stuff->device_specific_ptys;
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 15cba3610..2cd43f681 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -262,6 +262,67 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
   DWORD pidRestore = 0;
   bool attach_to_pcon = false;
 
+  /* Attach to pseudo console if pty salve is used */
+  pidRestore = fhandler_console::get_console_process_id
+    (GetCurrentProcessId (), false);
+  fhandler_pty_slave *ptys = NULL;
+  int chk_order[] = {1, 0, 2};
+  for (int i = 0; i < 3; i ++)
+    {
+      int fd = chk_order[i];
+      fhandler_base *fh = ::cygheap->fdtab[fd];
+      if (fh && fh->get_major () == DEV_PTYS_MAJOR)
+ {
+  ptys = (fhandler_pty_slave *) fh;
+  if (ptys->getPseudoConsole ())
+    {
+      DWORD dwHelperProcessId = ptys->getHelperProcessId ();
+      debug_printf ("found a PTY slave %d: helper_PID=%d",
+    fh->get_minor (), dwHelperProcessId);
+      if (fhandler_console::get_console_process_id
+  (dwHelperProcessId, true))
+ {
+  /* Already attached */
+  attach_to_pcon = true;
+  break;
+ }
+      else
+ {
+  FreeConsole ();
+  if (AttachConsole (dwHelperProcessId))
+    {
+      attach_to_pcon = true;
+      break;
+    }
+  else
+    {
+      /* Fallback */
+      DWORD target[3] = {
+ STD_INPUT_HANDLE,
+ STD_OUTPUT_HANDLE,
+ STD_ERROR_HANDLE
+      };
+      if (fd == 0)
+ {
+  ptys->set_handle (ptys->get_handle_cyg ());
+  SetStdHandle (target[fd],
+ ptys->get_handle ());
+ }
+      else if (fd < 3)
+ {
+  ptys->set_output_handle (
+       ptys->get_output_handle_cyg ());
+  SetStdHandle (target[fd],
+ ptys->get_output_handle ());
+ }
+    }
+ }
+    }
+ }
+    }
+  if (ptys)
+    ptys->fixup_after_attach (!iscygwin ());
+
   /* Check if we have been called from exec{lv}p or spawn{lv}p and mask
      mode to keep only the spawn mode. */
   bool p_type_exec = !!(mode & _P_PATH_TYPE_EXEC);
@@ -539,8 +600,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
  in a console will break native processes running in the background,
  because the Ctrl-C event is sent to all processes in the console, unless
  they ignore it explicitely.  CREATE_NEW_PROCESS_GROUP does that for us. */
-      if (!iscygwin () && fhandler_console::exists ()
-  && fhandler_console::tc_getpgid () != myself->pgid)
+      if (!iscygwin () &&
+  ((fhandler_console::exists ()
+ && fhandler_console::tc_getpgid () != myself->pgid)
+   || (ptys && ptys->getpgid () != myself->pgid)))
  c_flags |= CREATE_NEW_PROCESS_GROUP;
       refresh_cygheap ();
 
@@ -574,67 +637,6 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
  PROCESS_QUERY_LIMITED_INFORMATION))
  sa = &sec_none_nih;
 
-      /* Attach to pseudo console if pty salve is used */
-      pidRestore = fhandler_console::get_console_process_id
- (GetCurrentProcessId (), false);
-      fhandler_pty_slave *ptys = NULL;
-      int chk_order[] = {1, 0, 2};
-      for (int i = 0; i < 3; i ++)
- {
-  int fd = chk_order[i];
-  fhandler_base *fh = ::cygheap->fdtab[fd];
-  if (fh && fh->get_major () == DEV_PTYS_MAJOR)
-    {
-      ptys = (fhandler_pty_slave *) fh;
-      if (ptys->getPseudoConsole ())
- {
-  DWORD dwHelperProcessId = ptys->getHelperProcessId ();
-  debug_printf ("found a PTY slave %d: helper_PID=%d",
-    fh->get_minor (), dwHelperProcessId);
-  if (fhandler_console::get_console_process_id
-      (dwHelperProcessId, true))
-    {
-      /* Already attached */
-      attach_to_pcon = true;
-      break;
-    }
-  else
-    {
-      FreeConsole ();
-      if (AttachConsole (dwHelperProcessId))
- {
-  attach_to_pcon = true;
-  break;
- }
-      else
- {
-  /* Fallback */
-  DWORD target[3] = {
-    STD_INPUT_HANDLE,
-    STD_OUTPUT_HANDLE,
-    STD_ERROR_HANDLE
-  };
-  if (fd == 0)
-    {
-      ptys->set_handle (ptys->get_handle_cyg ());
-      SetStdHandle (target[fd],
-    ptys->get_handle ());
-    }
-  else if (fd < 3)
-    {
-      ptys->set_output_handle (
-   ptys->get_output_handle_cyg ());
-      SetStdHandle (target[fd],
-    ptys->get_output_handle ());
-    }
- }
-    }
- }
-    }
- }
-      if (ptys)
- ptys->fixup_after_attach (!iscygwin ());
-
       if (!iscygwin ())
  {
   init_console_handler (myself->ctty > 0);
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Ken Brown-6
In reply to this post by Takashi Yano
Hi Takashi,

On 9/6/2019 10:51 AM, Takashi Yano wrote:

> - When the I/O pipe is switched to the pseudo console side, the
>    behaviour of Ctrl-C is unstable. This rarely happens, however,
>    for example, shell sometimes crashes by Ctrl-C in that situation.
>    This patch fixes that issue.
>
> v3:
> Fix mistake in v2.
>
> v2:
> Remove the code which accidentally clears ENABLE_ECHO_INPUT flag.
>
> Takashi Yano (1):
>    Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.
>
>   winsup/cygwin/fhandler.h      |   4 +-
>   winsup/cygwin/fhandler_tty.cc |  33 +++++----
>   winsup/cygwin/select.cc       |   2 +-
>   winsup/cygwin/spawn.cc        | 128 +++++++++++++++++-----------------
>   4 files changed, 89 insertions(+), 78 deletions(-)

I had several problems after applying this patch.

1. I noticed some display glitches when building cygwin (with -j13 if that's
relevant).  For example, there were some unexpected blank lines and indented lines.

2. At one point the build wouldn't complete at all.  It hung and had to be
killed with Ctrl-C.

3. I used ssh from my normal account to log into an administrator account.  I
ran a script that produced a lot of output and piped it to less.  I pressed 'q'
after the first screen was displayed, and the displayed text didn't get cleared.

Ken

P.S. I'm leaving tomorrow for a short vacation, so I might not have time to
review any more patches until I return in about a week.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
Hi Ken,

I appreciate checking the patch.

On Fri, 6 Sep 2019 17:59:02 +0000
Ken Brown wrote:
> I had several problems after applying this patch.
>
> 1. I noticed some display glitches when building cygwin (with -j13 if that's
> relevant).  For example, there were some unexpected blank lines and indented lines.
>
> 2. At one point the build wouldn't complete at all.  It hung and had to be
> killed with Ctrl-C.

I could not reproduce these with 32-bit cygwin (which I usually use),
however, I noticed that they happen with 64-bit cygwin. I apologize
for the lack of enough test.

I will post fixed version as v4 patch.

> 3. I used ssh from my normal account to log into an administrator account.  I
> ran a script that produced a lot of output and piped it to less.  I pressed 'q'
> after the first screen was displayed, and the displayed text didn't get cleared.

Are you using non-cygwin program in the script? If so, this may happen
in test release 3.1.0-0.4 as well.

> P.S. I'm leaving tomorrow for a short vacation, so I might not have time to
> review any more patches until I return in about a week.

I see. So I have at least a week for consideration. :)

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
On Sat, 7 Sep 2019 12:20:15 +0900
Takashi Yano wrote:
> On Fri, 6 Sep 2019 17:59:02 +0000
> Ken Brown wrote:
> > 3. I used ssh from my normal account to log into an administrator account.  I
> > ran a script that produced a lot of output and piped it to less.  I pressed 'q'
> > after the first screen was displayed, and the displayed text didn't get cleared.
>
> Are you using non-cygwin program in the script? If so, this may happen
> in test release 3.1.0-0.4 as well.

3.1.0-0.3 as well.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
In reply to this post by Ken Brown-6
Hi Ken,

On Fri, 6 Sep 2019 17:59:02 +0000
Ken Brown wrote:
> P.S. I'm leaving tomorrow for a short vacation, so I might not have time to
> review any more patches until I return in about a week.

I submitted five patches during your absence.

Four of them are for pseudo console support, the other one is for console.

[PATCH v5 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.
[PATCH 0/1] Cygwin: pty: Fix screen alternation while pseudo console switching.
[PATCH 0/1] Cygwin: pty: Prevent the helper process from exiting by Ctrl-C
[PATCH v2 0/1] Cygwin: pty: Switch input and output pipes individually.
[PATCH 0/1] Cygwin: console: Fix read() in non-canonical mode.

Could you please review them?

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Ken Brown-6
Hi Takashi,

On 9/13/2019 5:52 PM, Takashi Yano wrote:
> I submitted five patches during your absence.
>
> Four of them are for pseudo console support, the other one is for console.
>
> [PATCH v5 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.
> [PATCH 0/1] Cygwin: pty: Fix screen alternation while pseudo console switching.
> [PATCH 0/1] Cygwin: pty: Prevent the helper process from exiting by Ctrl-C
> [PATCH v2 0/1] Cygwin: pty: Switch input and output pipes individually.
> [PATCH 0/1] Cygwin: console: Fix read() in non-canonical mode.

All pushed.  Thanks.

Do you think there have been enough changes that I should issue another test
release, or do you have more patches in the works?

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 0/1] Cygwin: pty: Fix the behaviour of Ctrl-C in the pseudo console mode.

Takashi Yano
On Sat, 14 Sep 2019 15:29:50 +0000
Ken Brown wrote:
> All pushed.  Thanks.

Thanks.

> Do you think there have been enough changes that I should issue another test
> release, or do you have more patches in the works?

I have submitted three more patches just now. Hopefully, the functional
fixes have been settled with these patches, so I think it is ready to
issue a new test release.

--
Takashi Yano <[hidden email]>