Re: [PATCH] Cygwin: pty, console: Refactor the code processing special keys.
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 wher
Re: [PATCH 0/2] Provide virtual /dev/fd and /dev/{stdin, stdout, stderr} symlinks
Hi Corinna, On Tue, 22 Feb 2022, Corinna Vinschen wrote: > On Feb 21 14:36, Johannes Schindelin wrote: > > These symbolic links are crucial e.g. to support process substitution > > (Bash's > > very nice `<(SOME-COMMAND)` feature). > > > > For various reasons, it is a bit cumbersome (or impossible) to generate > > these > > symbolic links in all circumstances where Git for Windows wants to use its > > close fork of the Cygwin runtime. > > > > Therefore, let's just handle these symbolic links as implicit, virtual ones. > > > > If there is appetite for it, I wonder whether we should do something similar > > for `/dev/shm` and `/dev/mqueue`? Are these even still used in Cygwin? > > "still used"? These are the dirs to store POSIX semaphors, message > queues and shared mem objects. Okay. I guess we do not really use them in Git for Windows ;-) > These have to be real on-disk dirs. Could I ask you to help me understand why? Do they have to be writable? Or do the things that are written into them have to be persisted between Cygwin sessions? I ask because it would be really helpful for Git for Windows if we could get away with _not_ having those directories. > > Johannes Schindelin (2): > > Implicitly support the /dev/fd symlink and friends > > Regenerate devices.cc > > > > winsup/cygwin/Makefile.am|1 + > > winsup/cygwin/devices.cc | 1494 -- > > winsup/cygwin/devices.h |3 +- > > winsup/cygwin/devices.in |4 + > > winsup/cygwin/dtable.cc |3 + > > winsup/cygwin/fhandler.h | 28 + > > winsup/cygwin/fhandler_dev_fd.cc | 53 ++ > > 7 files changed, 879 insertions(+), 707 deletions(-) > > create mode 100644 winsup/cygwin/fhandler_dev_fd.cc > > Pushed. Thank you! Dscho
Re: [PATCH 0/2] Provide virtual /dev/fd and /dev/{stdin,stdout,stderr} symlinks
On 2022-02-21 06:36, Johannes Schindelin wrote: These symbolic links are crucial e.g. to support process substitution (Bash's very nice `<(SOME-COMMAND)` feature). For various reasons, it is a bit cumbersome (or impossible) to generate these symbolic links in all circumstances where Git for Windows wants to use its close fork of the Cygwin runtime. Therefore, let's just handle these symbolic links as implicit, virtual ones. If there is appetite for it, I wonder whether we should do something similar for `/dev/shm` and `/dev/mqueue`? Are these even still used in Cygwin? Looks like that would make sense, as Cygwin appears to create all of those only on first startup (and probably rechecks if they need created every startup) e.g. Cygwin-32 $ ls -Fglot /dev/ | tail -6 lrwxrwxrwx 1 13 Apr 29 2012 fd -> /proc/self/fd/ lrwxrwxrwx 1 15 Apr 29 2012 stderr -> /proc/self/fd/2 lrwxrwxrwx 1 15 Apr 29 2012 stdout -> /proc/self/fd/1| lrwxrwxrwx 1 15 Apr 29 2012 stdin -> /proc/self/fd/0 drwxr-xr-x+ 10 Apr 29 2012 mqueue/ drwxr-xr-x+ 10 Apr 29 2012 shm/ Cygwin-64 $ ls -Fglot /dev/ | tail -6 drwxrwxrwt+ 10 Dec 2 2017 shm/ lrwxrwxrwx 1 13 May 14 2013 fd -> /proc/self/fd/ lrwxrwxrwx 1 15 May 14 2013 stderr -> /proc/self/fd/2 lrwxrwxrwx 1 15 May 14 2013 stdout -> /proc/self/fd/1| lrwxrwxrwx 1 15 May 14 2013 stdin -> /proc/self/fd/0 drwxrwxrwt+ 10 May 14 2013 mqueue/ so they would all get 2006-12-01 00:00:00+ birth time. [Looks like I ran something using shm in 2017!] -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in binary units and prefixes, physical quantities in SI.]
Re: [PATCH v2] Cygwin: pinfo: Fix exit code when non-cygwin app exits by Ctrl-C.
On Thu, 24 Feb 2022 23:24:29 +0900 Takashi Yano wrote: > - Previously, if non-cygwin app exits by Ctrl-C, exit code was > 0x7f00. With this patch, the exit code will be 0x0002, > which means process exited by SIGINT. > --- > winsup/cygwin/exceptions.cc | 6 +- > winsup/cygwin/pinfo.cc | 3 +++ > 2 files changed, 8 insertions(+), 1 deletion(-) I am sorry, but I pushed not v2 but v1 patch by mistake. I will submit new additional patch to cygwin-patches@cygwin.com. Please check it again. Thanks. -- Takashi Yano
[PATCH] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console.
- The recent commit "Cygwin: pinfo: Fix exit code when non-cygwin app
exits by Ctrl-C." did not fix enough the issue. If a non-cygwin app
is reading the console, it will not return STATUS_CONTROL_C_EXIT
even if it is terminated by Ctrl-C. As a result, the previous patch
does not take effect.
This patch solves this issue by setting sigExeced to SIGINT in
ctrl_c_handler(). In addition, sigExeced will be cleared if the app
does not terminated within predetermined time period. The reason is
that the app does not seem to be terminated by the signal sigExeced.
---
winsup/cygwin/exceptions.cc | 6 +-
winsup/cygwin/globals.cc| 2 +-
winsup/cygwin/spawn.cc | 10 +-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 73bf68939..f6a755b3c 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1139,7 +1139,11 @@ ctrl_c_handler (DWORD type)
}
if (ch_spawn.set_saw_ctrl_c ())
-return TRUE;
+{
+ if (myself->process_state & PID_NOTCYGWIN)
+ sigExeced = SIGINT;
+ return TRUE;
+}
/* We're only the process group leader when we have a valid pinfo structure.
If we don't have one, then the parent "stub" will handle the signal. */
diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc
index ac5ad0307..d3a2e11a4 100644
--- a/winsup/cygwin/globals.cc
+++ b/winsup/cygwin/globals.cc
@@ -20,7 +20,7 @@ HANDLE NO_COPY hProcImpToken;
HANDLE my_wr_proc_pipe;
HMODULE NO_COPY cygwin_hmodule;
HMODULE NO_COPY hntdll;
-int NO_COPY sigExeced;
+LONG NO_COPY sigExeced;
WCHAR windows_system_directory[MAX_PATH];
UINT windows_system_directory_length;
WCHAR system_wow64_directory[MAX_PATH];
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 3647580a6..df9ad84a7 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -953,7 +953,15 @@ child_info_spawn::worker (const char *prog_arg, const char
*const *argv,
if (sem)
__posix_spawn_sem_release (sem, 0);
if (ptys_need_cleanup || cons_need_cleanup)
- WaitForSingleObject (pi.hProcess, INFINITE);
+ {
+ LONG prev_sigExeced = sigExeced;
+ while (WaitForSingleObject (pi.hProcess, 100) == WAIT_TIMEOUT)
+ /* If child process does not exit in predetermined time
+ period, the process does not seem to be terminated by
+ the signal sigExeced. Therefore, clear sigExeced here. */
+ prev_sigExeced =
+ InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
+ }
if (ptys_need_cleanup)
{
fhandler_pty_slave::cleanup_for_non_cygwin_app (&ptys_handle_set,
--
2.35.1
