public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Takashi Yano <takashi.yano@nifty.ne.jp>
Cc: cygwin-patches@cygwin.com
Subject: Re: [PATCH] Cygwin: pty, console: Refactor the code processing special keys.
Date: Fri, 25 Feb 2022 14:37:59 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2202242023050.11118@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20220220111510.978-1-takashi.yano@nifty.ne.jp>

Hi Takashi,

On Sun, 20 Feb 2022, Takashi Yano wrote:

> - This patch commonize the code which processes special keys in pty
>   and console to improve maintanancibility.

I very much welcome the direction. Thank you for working on this!

> As a result, some small bugs have been fixed.

Whenever I read something like this in commit messages, I wish for more
details. Could you describe those bugs, and how exactly they were fixed?

> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index a914110fe..356d69d6a 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -1147,6 +1147,19 @@ ctrl_c_handler (DWORD type)
>      return TRUE;
>
>    tty_min *t = cygwin_shared->tty.get_cttyp ();
> +
> +  /* If process group leader is non-cygwin process or not exist,
> +     send signal to myself. */
> +  pinfo pi (t->getpgid ());
> +  if ((!pi || (pi->process_state & PID_NOTCYGWIN))
> +      && (!have_execed || have_execed_cygwin)
> +      && t->getpgid () == myself->pgid
> +      && type == CTRL_C_EVENT)
> +    {
> +      t->output_stopped = false;
> +      sig_send(myself, SIGINT);
> +    }
> +

From the commit message, I would have expected this patch to be
essentially a clean and obvious refactoring of the existing code.

However, I cannot find any removed code in the diff that would explain
this newly-added code.

When I see something like this in patches, I usually take to the commit
message to explain what is going on. But in this case, I am left even more
puzzled than before.

> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 4e86ab58a..3e0d7d5a6 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -1901,6 +1902,13 @@ class fhandler_termios: public fhandler_base
>    virtual void release_input_mutex_if_necessary (void) {};
>    virtual void discard_input () {};
>
> +  enum process_sig_state {
> +    signalled,
> +    not_signalled,
> +    not_signalled_but_done,
> +    not_signalled_with_cyg_reader
> +  };
> +

My hope for this refactor is that it will get much easier to understand
for developers who are unfamiliar with the pseudo console code.

In this instance, I am wishing for a code comment above the `enum` that
explains its role, and the meaning of the four values. In particular the
difference of the last two compared to the second is quite lost on me.

> @@ -1943,6 +1954,8 @@ class fhandler_termios: public fhandler_base
>    }
>    static bool path_iscygexec_a (LPCSTR n, LPSTR c);
>    static bool path_iscygexec_w (LPCWSTR n, LPWSTR c);
> +  virtual bool is_pty_master_with_pcon () { return false; }

It would probably make sense to document the role of a pty master with
respect to pseudo consoles somewhere. If no better location can be found,
then it would probably make most sense to add a comment above this new
line.

> +  virtual void cleanup_before_exit () {}
>  };
>
>  enum ansi_intensity
> diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
> index 50f350c49..475c1acdb 100644
> --- a/winsup/cygwin/fhandler_console.cc
> +++ b/winsup/cygwin/fhandler_console.cc
> @@ -188,13 +188,28 @@ cons_master_thread (VOID *arg)
>  void
>  fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp)
>  {
> +  termios &ti = ttyp->ti;
>    DWORD output_stopped_at = 0;
>    while (con.owner == myself->pid)
>      {
>        DWORD total_read, n, i;
>        INPUT_RECORD input_rec[INREC_SIZE];
>
> -      if (con.disable_master_thread)
> +      bool nat_fg = false;
> +      bool nat_child_fg = false;
> +      winpids pids ((DWORD) 0);
> +      for (unsigned i = 0; i < pids.npids; i++)
> +	{
> +	  _pinfo *pi = pids[i];
> +	  if (pi && pi->ctty == ttyp->ntty && pi->pgid == ttyp->getpgid ()
> +	      && (pi->process_state & PID_NOTCYGWIN)
> +	      && !(pi->process_state & PID_NEW_PG))
> +	    nat_fg = true;
> +	  if (pi && pi->ctty == ttyp->ntty && pi->pgid == ttyp->getpgid ()
> +	      && !(pi->process_state & PID_CYGPARENT))
> +	    nat_child_fg = true;
> +	}
> +      if (nat_fg && !nat_child_fg)

It is unclear to me whether this code fixes a bug, or refactors other
code, or both. Not to forget: it is still unclear to be what `nat` should
mean, which suggests to me that either the name could be improved or a
code comment would be in order.

Since it is already unclear to me what fundamental goal this part has (as
well as much of the remainder of this quite large patch), I fear that even
for a motivated reviewer like myself, it is rather difficult to provide
any useful review.

Therefore I think I need to stop here, as much as I want to help.

Maybe this code could be augmented with more comments? And maybe the patch
could be split up further for clarity? I have a sense that much of it is
actually refactoring that could be done incrementally, with an explanation
in the commit message that makes things utterly clear, and other parts are
bug fixes where the bug in question could be described, and the approach
taken to fix it (as well as other approaches that were considered and a
discussion why those approaches were rejected).

Thank you for working on this, I appreciate it a lot,
Johannes

>  	{
>  	  cygwait (40);
>  	  continue;
> @@ -233,90 +248,35 @@ fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp)
>  	}
>        for (i = 0; i < total_read; i++)
>  	{
> -	  const wchar_t wc = input_rec[i].Event.KeyEvent.uChar.UnicodeChar;
> -	  if ((wint_t) wc >= 0x80)
> -	    continue;
> -	  char c = (char) wc;
> +	  wchar_t wc;
> +	  char c;
> +	  bool was_output_stopped;
>  	  bool processed = false;
> -	  termios &ti = ttyp->ti;
> -	  pinfo pi (ttyp->getpgid ());
> -	  if (pi && pi->ctty == ttyp->ntty
> -	      && (pi->process_state & PID_NOTCYGWIN)
> -	      && input_rec[i].EventType == KEY_EVENT && c == '\003')
> -	    {
> -	      bool not_a_sig = false;
> -	      if (!CCEQ (ti.c_cc[VINTR], c)
> -		  && !CCEQ (ti.c_cc[VQUIT], c)
> -		  && !CCEQ (ti.c_cc[VSUSP], c))
> -		not_a_sig = true;
> -	      if (input_rec[i].Event.KeyEvent.bKeyDown)
> -		{
> -		  /* CTRL_C_EVENT does not work for the process started with
> -		     CREATE_NEW_PROCESS_GROUP flag, so send CTRL_BREAK_EVENT
> -		     instead. */
> -		  if (pi->process_state & PID_NEW_PG)
> -		    GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT,
> -					      pi->dwProcessId);
> -		  else
> -		    GenerateConsoleCtrlEvent (CTRL_C_EVENT, 0);
> -		  if (not_a_sig)
> -		    goto skip_writeback;
> -		}
> -	      processed = true;
> -	      if (not_a_sig)
> -		goto remove_record;
> -	    }
>  	  switch (input_rec[i].EventType)
>  	    {
>  	    case KEY_EVENT:
> -	      if (ti.c_lflag & ISIG)
> -		{
> -		  int sig = 0;
> -		  if (CCEQ (ti.c_cc[VINTR], c))
> -		    sig = SIGINT;
> -		  else if (CCEQ (ti.c_cc[VQUIT], c))
> -		    sig = SIGQUIT;
> -		  else if (CCEQ (ti.c_cc[VSUSP], c))
> -		    sig = SIGTSTP;
> -		  if (sig && input_rec[i].Event.KeyEvent.bKeyDown)
> -		    {
> -		      ttyp->kill_pgrp (sig);
> -		      ttyp->output_stopped = false;
> -		      ti.c_lflag &= ~FLUSHO;
> -		      /* Discard type ahead input */
> -		      goto skip_writeback;
> -		    }
> -		}
> -	      if (ti.c_iflag & IXON)
> -		{
> -		  if (CCEQ (ti.c_cc[VSTOP], c))
> -		    {
> -		      if (!ttyp->output_stopped
> -			  && input_rec[i].Event.KeyEvent.bKeyDown)
> -			{
> -			  ttyp->output_stopped = true;
> -			  output_stopped_at = i;
> -			}
> -		      processed = true;
> -		    }
> -		  else if (CCEQ (ti.c_cc[VSTART], c))
> -		    {
> -		restart_output:
> -		      if (input_rec[i].Event.KeyEvent.bKeyDown)
> -			ttyp->output_stopped = false;
> -		      processed = true;
> -		    }
> -		  else if ((ti.c_iflag & IXANY) && ttyp->output_stopped
> -			   && c && i >= output_stopped_at)
> -		    goto restart_output;
> -		}
> -	      if ((ti.c_lflag & ICANON) && (ti.c_lflag & IEXTEN)
> -		  && CCEQ (ti.c_cc[VDISCARD], c))
> +	      if (!input_rec[i].Event.KeyEvent.bKeyDown)
> +		continue;
> +	      wc = input_rec[i].Event.KeyEvent.uChar.UnicodeChar;
> +	      if (!wc || (wint_t) wc >= 0x80)
> +		continue;
> +	      c = (char) wc;
> +	      switch (process_sigs (c, ttyp, NULL))
>  		{
> -		  if (input_rec[i].Event.KeyEvent.bKeyDown)
> -		    ti.c_lflag ^= FLUSHO;
> +		case signalled:
> +		case not_signalled_but_done:
>  		  processed = true;
> +		  ttyp->output_stopped = false;
> +		  if (ti.c_lflag & NOFLSH)
> +		    goto remove_record;
> +		  goto skip_writeback;
> +		default: /* not signalled */
> +		  break;
>  		}
> +	      was_output_stopped = ttyp->output_stopped;
> +	      processed = process_stop_start (c, ttyp, i > output_stopped_at);
> +	      if (!was_output_stopped && ttyp->output_stopped)
> +		output_stopped_at = i;
>  	      break;
>  	    case WINDOW_BUFFER_SIZE_EVENT:
>  	      SHORT y = con.dwWinSize.Y;
> @@ -447,7 +407,6 @@ fhandler_console::setup ()
>        con.cons_rapoi = NULL;
>        shared_console_info->tty_min_state.is_console = true;
>        con.cursor_key_app_mode = false;
> -      con.disable_master_thread = false;
>      }
>  }
>
> @@ -503,8 +462,6 @@ fhandler_console::set_input_mode (tty::cons_mode m, const termios *t,
>  	flags |= ENABLE_VIRTUAL_TERMINAL_INPUT;
>        else
>  	flags |= ENABLE_MOUSE_INPUT;
> -      if (shared_console_info)
> -	con.disable_master_thread = false;
>        break;
>      case tty::native:
>        if (t->c_lflag & ECHO)
> @@ -517,8 +474,6 @@ fhandler_console::set_input_mode (tty::cons_mode m, const termios *t,
>  	flags &= ~ENABLE_ECHO_INPUT;
>        if (t->c_lflag & ISIG)
>  	flags |= ENABLE_PROCESSED_INPUT;
> -      if (shared_console_info)
> -	con.disable_master_thread = true;
>        break;
>      }
>    SetConsoleMode (p->input_handle, flags);
> @@ -1394,9 +1349,6 @@ fhandler_console::open (int flags, mode_t)
>  	setenv ("TERM", "cygwin", 1);
>      }
>
> -  set_input_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> -  set_output_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> -
>    debug_printf ("opened conin$ %p, conout$ %p", get_handle (),
>  		get_output_handle ());
>
> @@ -1421,6 +1373,17 @@ fhandler_console::open_setup (int flags)
>    return fhandler_base::open_setup (flags);
>  }
>
> +void
> +fhandler_console::post_open_setup (int fd)
> +{
> +  if (fd == 0)
> +    set_input_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> +  else if (fd == 1 || fd == 2)
> +    set_output_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> +
> +  fhandler_base::post_open_setup (fd);
> +}
> +
>  int
>  fhandler_console::close ()
>  {
> diff --git a/winsup/cygwin/fhandler_termios.cc b/winsup/cygwin/fhandler_termios.cc
> index b935a70bc..477d75b96 100644
> --- a/winsup/cygwin/fhandler_termios.cc
> +++ b/winsup/cygwin/fhandler_termios.cc
> @@ -130,8 +130,9 @@ is_flush_sig (int sig)
>  }
>
>  void
> -tty_min::kill_pgrp (int sig)
> +tty_min::kill_pgrp (int sig, pid_t target_pgid)
>  {
> +  target_pgid = target_pgid ?: pgid;
>    bool killself = false;
>    if (is_flush_sig (sig) && cygheap->ctty)
>      cygheap->ctty->sigflush ();
> @@ -145,7 +146,7 @@ tty_min::kill_pgrp (int sig)
>    for (unsigned i = 0; i < pids.npids; i++)
>      {
>        _pinfo *p = pids[i];
> -      if (!p || !p->exists () || p->ctty != ntty || p->pgid != pgid)
> +      if (!p || !p->exists () || p->ctty != ntty || p->pgid != target_pgid)
>  	continue;
>        if (p->process_state & PID_NOTCYGWIN)
>  	continue;
> @@ -308,6 +309,156 @@ fhandler_termios::echo_erase (int force)
>      doecho ("\b \b", 3);
>  }
>
> +fhandler_termios::process_sig_state
> +fhandler_termios::process_sigs (char c, tty* ttyp, fhandler_termios *fh)
> +{
> +  termios &ti = ttyp->ti;
> +  pid_t pgid = ttyp->pgid;
> +
> +  pinfo leader (pgid);
> +  bool cyg_leader = leader && !(leader->process_state & PID_NOTCYGWIN);
> +  bool ctrl_c_event_sent = false;
> +  bool need_discard_input = false;
> +  bool pg_with_nat = false;
> +  bool need_send_sig = false;
> +  bool nat_shell = false;
> +  bool cyg_reader = false;
> +
> +  winpids pids ((DWORD) 0);
> +  for (unsigned i = 0; i < pids.npids; i++)
> +    {
> +      _pinfo *p = pids[i];
> +      if (c == '\003' && p && p->ctty == ttyp->ntty && p->pgid == pgid
> +	  && ((p->process_state & PID_NOTCYGWIN)
> +	      || !(p->process_state & PID_CYGPARENT)))
> +	{
> +	  pinfo pinfo_resume = pinfo (myself->ppid);
> +	  DWORD resume_pid = 0;
> +	  if (pinfo_resume)
> +	    resume_pid = pinfo_resume->dwProcessId;
> +	  else
> +	    resume_pid = fhandler_pty_common::get_console_process_id
> +	      (myself->dwProcessId, false);
> +	  if (resume_pid && fh && !fh->is_console ())
> +	    {
> +	      FreeConsole ();
> +	      AttachConsole (p->dwProcessId);
> +	    }
> +	  /* CTRL_C_EVENT does not work for the process started with
> +	     CREATE_NEW_PROCESS_GROUP flag, so send CTRL_BREAK_EVENT
> +	     instead. */
> +	  if (p->process_state & PID_NEW_PG)
> +	    GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT,
> +				      p->dwProcessId);
> +	  else if ((!fh || !fh->is_pty_master_with_pcon () || cyg_leader)
> +		   && !ctrl_c_event_sent)
> +	    {
> +	      GenerateConsoleCtrlEvent (CTRL_C_EVENT, 0);
> +	      ctrl_c_event_sent = true;
> +	    }
> +	  if (resume_pid && fh && !fh->is_console ())
> +	    {
> +	      FreeConsole ();
> +	      AttachConsole (resume_pid);
> +	    }
> +	  need_discard_input = true;
> +	}
> +      if (p && p->ctty == ttyp->ntty && p->pgid == pgid)
> +	{
> +	  if (p->process_state & PID_NOTCYGWIN)
> +	    pg_with_nat = true;
> +	  if (!(p->process_state & PID_NOTCYGWIN))
> +	    need_send_sig = true;
> +	  if (!p->cygstarted)
> +	    nat_shell = true;
> +	  if (p->process_state & PID_TTYIN)
> +	    cyg_reader = true;
> +	}
> +    }
> +  /* Send SIGQUIT to non-cygwin process. */
> +  if ((ti.c_lflag & ISIG) && CCEQ (ti.c_cc[VQUIT], c)
> +      && pg_with_nat && need_send_sig && !nat_shell)
> +    {
> +      for (unsigned i = 0; i < pids.npids; i++)
> +	{
> +	  _pinfo *p = pids[i];
> +	  if (p && p->ctty == ttyp->ntty && p->pgid == pgid
> +	      && (p->process_state & PID_NOTCYGWIN))
> +	    sig_send (p, SIGQUIT);
> +	}
> +    }
> +  if ((ti.c_lflag & ISIG) && need_send_sig)
> +    {
> +      int sig;
> +      if (CCEQ (ti.c_cc[VINTR], c))
> +	sig = SIGINT;
> +      else if (CCEQ (ti.c_cc[VQUIT], c))
> +	sig = SIGQUIT;
> +      else if (pg_with_nat)
> +	goto not_a_sig;
> +      else if (CCEQ (ti.c_cc[VSUSP], c))
> +	sig = SIGTSTP;
> +      else
> +	goto not_a_sig;
> +
> +      termios_printf ("got interrupt %d, sending signal %d", c, sig);
> +      if (!(ti.c_lflag & NOFLSH) && fh)
> +	{
> +	  fh->eat_readahead (-1);
> +	  fh->discard_input ();
> +	}
> +      if (fh)
> +	fh->release_input_mutex_if_necessary ();
> +      ttyp->kill_pgrp (sig, pgid);
> +      if (fh)
> +	fh->acquire_input_mutex_if_necessary (mutex_timeout);
> +      ti.c_lflag &= ~FLUSHO;
> +      return signalled;
> +    }
> +not_a_sig:
> +  if (need_discard_input)
> +    {
> +      if (!(ti.c_lflag & NOFLSH) && fh)
> +	{
> +	  fh->eat_readahead (-1);
> +	  fh->discard_input ();
> +	}
> +      ti.c_lflag &= ~FLUSHO;
> +      return not_signalled_but_done;
> +    }
> +  bool to_cyg = cyg_reader || !pg_with_nat;
> +  return to_cyg ? not_signalled_with_cyg_reader : not_signalled;
> +}
> +
> +bool
> +fhandler_termios::process_stop_start (char c, tty *ttyp, bool on_ixany)
> +{
> +  termios &ti = ttyp->ti;
> +  if (ti.c_iflag & IXON)
> +    {
> +      if (CCEQ (ti.c_cc[VSTOP], c))
> +	{
> +	  ttyp->output_stopped = true;
> +	  return true;
> +	}
> +      else if (CCEQ (ti.c_cc[VSTART], c))
> +	{
> +restart_output:
> +	  ttyp->output_stopped = false;
> +	  return true;
> +	}
> +      else if ((ti.c_iflag & IXANY) && ttyp->output_stopped && on_ixany)
> +	goto restart_output;
> +    }
> +  if ((ti.c_lflag & ICANON) && (ti.c_lflag & IEXTEN)
> +      && CCEQ (ti.c_cc[VDISCARD], c))
> +    {
> +      ti.c_lflag ^= FLUSHO;
> +      return true;
> +    }
> +  return false;
> +}
> +
>  line_edit_status
>  fhandler_termios::line_edit (const char *rptr, size_t nread, termios& ti,
>  			     ssize_t *bytes_read)
> @@ -328,92 +479,24 @@ fhandler_termios::line_edit (const char *rptr, size_t nread, termios& ti,
>
>        if (ti.c_iflag & ISTRIP)
>  	c &= 0x7f;
> -      winpids pids ((DWORD) 0);
> -      bool need_check_sigs = get_ttyp ()->pcon_input_state_eq (tty::to_cyg);
> -      if (get_ttyp ()->pcon_input_state_eq (tty::to_nat))
> +      bool disable_eof_key = true;
> +      switch (process_sigs (c, get_ttyp (), this))
>  	{
> -	  bool need_discard_input = false;
> -	  for (unsigned i = 0; i < pids.npids; i++)
> -	    {
> -	      _pinfo *p = pids[i];
> -	      if (c == '\003' && p && p->ctty == tc ()->ntty
> -		  && p->pgid == tc ()->getpgid ()
> -		  && (p->process_state & PID_NOTCYGWIN))
> -		{
> -		  /* CTRL_C_EVENT does not work for the process started with
> -		     CREATE_NEW_PROCESS_GROUP flag, so send CTRL_BREAK_EVENT
> -		     instead. */
> -		  if (p->process_state & PID_NEW_PG)
> -		    GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT,
> -					      p->dwProcessId);
> -		  else
> -		    GenerateConsoleCtrlEvent (CTRL_C_EVENT, 0);
> -		  need_discard_input = true;
> -		}
> -	      if (p->ctty == get_ttyp ()->ntty
> -		  && p->pgid == get_ttyp ()->getpgid () && !p->cygstarted)
> -		need_check_sigs = true;
> -	    }
> -	  if (!CCEQ (ti.c_cc[VINTR], c)
> -	      && !CCEQ (ti.c_cc[VQUIT], c)
> -	      && !CCEQ (ti.c_cc[VSUSP], c))
> -	    need_check_sigs = false;
> -	  if (need_discard_input && !need_check_sigs)
> -	    {
> -	      if (!(ti.c_lflag & NOFLSH))
> -		{
> -		  eat_readahead (-1);
> -		  discard_input ();
> -		}
> -	      ti.c_lflag &= ~FLUSHO;
> -	      continue;
> -	    }
> -	}
> -      if ((ti.c_lflag & ISIG) && need_check_sigs)
> -	{
> -	  int sig;
> -	  if (CCEQ (ti.c_cc[VINTR], c))
> -	    sig = SIGINT;
> -	  else if (CCEQ (ti.c_cc[VQUIT], c))
> -	    sig = SIGQUIT;
> -	  else if (CCEQ (ti.c_cc[VSUSP], c))
> -	    sig = SIGTSTP;
> -	  else
> -	    goto not_a_sig;
> -
> -	  termios_printf ("got interrupt %d, sending signal %d", c, sig);
> -	  if (!(ti.c_lflag & NOFLSH))
> -	    {
> -	      eat_readahead (-1);
> -	      discard_input ();
> -	    }
> -	  release_input_mutex_if_necessary ();
> -	  tc ()->kill_pgrp (sig);
> -	  acquire_input_mutex_if_necessary (mutex_timeout);
> -	  ti.c_lflag &= ~FLUSHO;
> +	case signalled:
>  	  sawsig = true;
> -	  goto restart_output;
> -	}
> -    not_a_sig:
> -      if (ti.c_iflag & IXON)
> -	{
> -	  if (CCEQ (ti.c_cc[VSTOP], c))
> -	    {
> -	      if (!tc ()->output_stopped)
> -		tc ()->output_stopped = true;
> -	      continue;
> -	    }
> -	  else if (CCEQ (ti.c_cc[VSTART], c))
> -	    {
> -    restart_output:
> -	      tc ()->output_stopped = false;
> -	      continue;
> -	    }
> -	  else if ((ti.c_iflag & IXANY) && tc ()->output_stopped)
> -	    goto restart_output;
> +	  fallthrough;
> +	case not_signalled_but_done:
> +	  get_ttyp ()->output_stopped = false;
> +	  continue;
> +	case not_signalled_with_cyg_reader:
> +	  disable_eof_key = false;
> +	  break;
> +	default: /* Not signalled */
> +	  break;
>  	}
> +      if (process_stop_start (c, get_ttyp (), true))
> +	continue;
>        /* Check for special chars */
> -
>        if (c == '\r')
>  	{
>  	  if (ti.c_iflag & IGNCR)
> @@ -432,12 +515,6 @@ fhandler_termios::line_edit (const char *rptr, size_t nread, termios& ti,
>  	    set_input_done (iscanon);
>  	}
>
> -      if (iscanon && ti.c_lflag & IEXTEN && CCEQ (ti.c_cc[VDISCARD], c))
> -	{
> -	  ti.c_lflag ^= FLUSHO;
> -	  continue;
> -	}
> -
>        if (!iscanon)
>  	/* nothing */;
>        else if (CCEQ (ti.c_cc[VERASE], c))
> @@ -474,7 +551,7 @@ fhandler_termios::line_edit (const char *rptr, size_t nread, termios& ti,
>  	    }
>  	  continue;
>  	}
> -      else if (CCEQ (ti.c_cc[VEOF], c) && need_check_sigs)
> +      else if (CCEQ (ti.c_cc[VEOF], c) && !disable_eof_key)
>  	{
>  	  termios_printf ("EOF");
>  	  accept_input ();
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index 5ba50cc73..a25690a0e 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -2291,47 +2291,12 @@ fhandler_pty_master::write (const void *ptr, size_t len)
>  			  &mbp);
>  	}
>
> -      if ((ti.c_lflag & ISIG) && memchr (buf, '\003', nlen))
> -	{
> -	  /* If the process is started with CREATE_NEW_PROCESS_GROUP
> -	     flag, Ctrl-C will not be sent to that process. Therefore,
> -	     send Ctrl-break event to that process here. */
> -	  DWORD wpid = 0;
> -	  winpids pids ((DWORD) 0);
> -	  for (unsigned i = 0; i < pids.npids; i++)
> -	    {
> -	      _pinfo *p = pids[i];
> -	      if (p->ctty == get_ttyp ()->ntty
> -		  && p->pgid == get_ttyp ()->getpgid ()
> -		  && (p->process_state & PID_NOTCYGWIN)
> -		  && (p->process_state & PID_NEW_PG))
> -		{
> -		  wpid = p->dwProcessId;
> -		  break;
> -		}
> -	    }
> -	  pinfo pinfo_resume = pinfo (myself->ppid);
> -	  DWORD resume_pid;
> -	  if (pinfo_resume)
> -	    resume_pid = pinfo_resume->dwProcessId;
> -	  else
> -	    resume_pid = get_console_process_id (myself->dwProcessId, false);
> -	  if (wpid && resume_pid)
> -	    {
> -	      WaitForSingleObject (pcon_mutex, INFINITE);
> -	      FreeConsole ();
> -	      AttachConsole (wpid);
> -	      /* CTRL_C_EVENT does not work for the process started with
> -		 CREATE_NEW_PROCESS_GROUP flag, so send CTRL_BREAK_EVENT
> -		 instead. */
> -	      GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT, wpid);
> -	      FreeConsole ();
> -	      AttachConsole (resume_pid);
> -	      ReleaseMutex (pcon_mutex);
> -	    }
> -	  if (!(ti.c_lflag & NOFLSH))
> -	    get_ttyp ()->discard_input = true;
> +      for (size_t i = 0; i < nlen; i++)
> +	{
> +	  fhandler_termios::process_sigs (buf[i], get_ttyp (), this);
> +	  process_stop_start (buf[i], get_ttyp (), true);
>  	}
> +
>        DWORD n;
>        WriteFile (to_slave_nat, buf, nlen, &n, NULL);
>        ReleaseMutex (input_mutex);
> @@ -2885,7 +2850,8 @@ fhandler_pty_master::pty_master_fwd_thread (const master_fwd_thread_param_t *p)
>        WaitForSingleObject (p->output_mutex, mutex_timeout);
>        while (rlen>0)
>  	{
> -	  if (!process_opost_output (p->to_master, ptr, wlen, false,
> +	  if (!process_opost_output (p->to_master, ptr, wlen,
> +				     true /* disable output_stopped */,
>  				     p->ttyp, false))
>  	    {
>  	      termios_printf ("WriteFile for forwarding failed, %E");
> @@ -4006,3 +3972,10 @@ fhandler_pty_slave::transfer_input (tty::xfer_dir dir, HANDLE from, tty *ttyp,
>    ttyp->pcon_input_state = dir;
>    ttyp->discard_input = false;
>  }
> +
> +void
> +fhandler_pty_slave::cleanup_before_exit ()
> +{
> +  if (myself->process_state & PID_NOTCYGWIN)
> +    get_ttyp ()->wait_pcon_fwd ();
> +}
> diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
> index 2cd12a665..87ff43ea2 100644
> --- a/winsup/cygwin/tty.h
> +++ b/winsup/cygwin/tty.h
> @@ -75,7 +75,7 @@ public:
>    void setpgid (int pid);
>    int getsid () const {return sid;}
>    void setsid (pid_t tsid) {sid = tsid;}
> -  void kill_pgrp (int);
> +  void kill_pgrp (int, pid_t target_pgid = 0);
>    int is_orphaned_process_group (int);
>    const __reg1 char *ttyname () __attribute (());
>  };
> --
> 2.35.1
>
>

  reply	other threads:[~2022-02-25 13:38 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-20 11:15 Takashi Yano
2022-02-25 13:37 ` Johannes Schindelin [this message]
2022-02-26 13:06   ` Takashi Yano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.2202242023050.11118@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=cygwin-patches@cygwin.com \
    --cc=takashi.yano@nifty.ne.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).