Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.
Hi Takashi, On Sep 4 10:44, Takashi Yano wrote: > - API hook used for pseudo console support causes slow down. > This patch limits API hook to only program which is linked > with the corresponding APIs. Normal cygwin program is not > linked with such APIs (such as WriteFile, etc...) directly, > therefore, no slow down occurs. However, console access by > cygwin.dll itself cannot switch the r/w pipe to pseudo console > side. Therefore, the code to switch it forcely to pseudo > console side is added to smallprint.cc and strace.cc. I'll push the other 3 patches from this series. For this patch, I wonder why you create set_ishybrid_and_switch_to_pcon while at the same time define a macro CHK_CONSOLE_ACCESS with identical functionality. Suggestion: Only define set_ishybrid_and_switch_to_pcon() as inline function (probably in winsup.h) and use only this througout. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v5 0/1] Fix PTY state management in pseudo console support.
On Sep 4 10:45, Takashi Yano wrote: > Pseudo console support in test release TEST: Cygwin 3.1.0-0.3, > 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. > > v5: > Revise based on > https://cygwin.com/ml/cygwin-patches/2019-q3/msg00111.html > > v4: > Small bug fix again. > > v3: > Fix the first issue (Bad file descriptor) reported in > https://cygwin.com/ml/cygwin-patches/2019-q3/msg00104.html > > v2: > Small bug fixed from v1. > > Takashi Yano (1): > Cygwin: pty: Fix state management for pseudo console support. > > winsup/cygwin/dtable.cc | 38 +-- > winsup/cygwin/fhandler.h | 6 +- > winsup/cygwin/fhandler_console.cc | 25 +- > winsup/cygwin/fhandler_tty.cc | 385 -- > winsup/cygwin/fork.cc | 24 +- > winsup/cygwin/spawn.cc| 65 ++--- > 6 files changed, 289 insertions(+), 254 deletions(-) > > -- > 2.21.0 Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 1/2] Cygwin: pty: Add a workaround for ^C handling.
On Sep 4 10:46, Takashi Yano wrote: > - Pseudo console support introduced by commit > 169d65a5774acc76ce3f3feeedcbae7405aa9b57 sometimes cause random > crash or freeze by pressing ^C while cygwin and non-cygwin > processes are executed simultaneously in the same pty. This > patch is a workaround for this issue. If this workaround works, what about making it the standard behaviour, rather than pseudo-console only? Would there be a downside? Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Sep 4 10:46, Takashi Yano wrote:
> - Pseudo console support introduced by commit
> 169d65a5774acc76ce3f3feeedcbae7405aa9b57 shows garbage ^[[H^[[J in
> some of emacs screens. These screens do not handle ANSI escape
> sequences. Therefore, clear screen is disabled on these screens.
> ---
> winsup/cygwin/fhandler_tty.cc | 26 +++---
> winsup/cygwin/tty.cc | 1 +
> winsup/cygwin/tty.h | 1 +
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index 283558985..a74c3eecf 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -972,6 +972,19 @@ skip_console_setting:
> void
> fhandler_pty_slave::reset_switch_to_pcon (void)
> {
> + if (get_ttyp ()->need_clear_screen)
> +{
> + const char *term = getenv ("TERM");
> + if (term && strcmp (term, "dumb") && !strstr (term, "emacs"))
Why do you check the TERMs again here? After all, need_clear_screen
is only true if one of these terms are used.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.
Hi Corinna, On Wed, 4 Sep 2019 12:03:51 +0200 Corinna Vinschen wrote: > I'll push the other 3 patches from this series. For this patch, > I wonder why you create set_ishybrid_and_switch_to_pcon while > at the same time define a macro CHK_CONSOLE_ACCESS with identical > functionality. Yah, indeed! > Suggestion: Only define set_ishybrid_and_switch_to_pcon() as > inline function (probably in winsup.h) and use only this througout. This function uses static variable isHybrid (sorry camelback again) and static function set_switch_to_pcon() defined in fhandler_tty.cc. To make it inline, a lot of changes will be necessary. How about non-inline function? -- Takashi Yano
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Wed, 4 Sep 2019 12:47:38 +0200 Corinna Vinschen wrote: > Why do you check the TERMs again here? After all, need_clear_screen > is only true if one of these terms are used. Because, emacs seems to set environment TERM after fixup_after_exec() is called. At the first check, TERM has the value of the terminal in which emacs is executed. The first check is just in case. -- Takashi Yano
Re: [PATCH 1/2] Cygwin: pty: Add a workaround for ^C handling.
On Wed, 4 Sep 2019 12:42:22 +0200
Corinna Vinschen wrote:
> If this workaround works, what about making it the standard behaviour,
> rather than pseudo-console only? Would there be a downside?
I am not sure why, but console does not have this issue.
However, I do not notice any downside.
If making it standard, the patch will be very simple as follows.
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index a3a7e7505..0a929dffd 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -213,7 +213,6 @@ frok::child (volatile char * volatile here)
- terminate the current fork call even if the child is initialized. */
sync_with_parent ("performed fork fixups and dynamic dll loading", true);
- init_console_handler (myself->ctty > 0);
ForceCloseHandle1 (fork_info->forker_finished, forker_finished);
pthread::atforkchild ();
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 4bb28c47b..15cba3610 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -635,6 +635,12 @@ child_info_spawn::worker (const char *prog_arg, const char
*const *argv,
if (ptys)
ptys->fixup_after_attach (!iscygwin ());
+ if (!iscygwin ())
+ {
+ init_console_handler (myself->ctty > 0);
+ myself->ctty = 0;
+ }
+
loop:
/* When ruid != euid we create the new process under the current original
account and impersonate in child, this way maintaining the different
--
Takashi Yano
Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.
Hi Takashi, On Sep 4 21:39, Takashi Yano wrote: > Hi Corinna, > > On Wed, 4 Sep 2019 12:03:51 +0200 > Corinna Vinschen wrote: > > I'll push the other 3 patches from this series. For this patch, > > I wonder why you create set_ishybrid_and_switch_to_pcon while > > at the same time define a macro CHK_CONSOLE_ACCESS with identical > > functionality. > > Yah, indeed! > > > Suggestion: Only define set_ishybrid_and_switch_to_pcon() as > > inline function (probably in winsup.h) and use only this througout. > > This function uses static variable isHybrid (sorry camelback again) > and static function set_switch_to_pcon() defined in fhandler_tty.cc. > > To make it inline, a lot of changes will be necessary. How about > non-inline function? That will add extra function calls, but, yeah, sure. We can streamline this later. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 1/2] Cygwin: pty: Add a workaround for ^C handling.
On Sep 4 22:30, Takashi Yano wrote:
> On Wed, 4 Sep 2019 12:42:22 +0200
> Corinna Vinschen wrote:
> > If this workaround works, what about making it the standard behaviour,
> > rather than pseudo-console only? Would there be a downside?
>
> I am not sure why, but console does not have this issue.
> However, I do not notice any downside.
>
> If making it standard, the patch will be very simple as follows.
>
>
> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
> index a3a7e7505..0a929dffd 100644
> --- a/winsup/cygwin/fork.cc
> +++ b/winsup/cygwin/fork.cc
> @@ -213,7 +213,6 @@ frok::child (volatile char * volatile here)
> - terminate the current fork call even if the child is initialized. */
>sync_with_parent ("performed fork fixups and dynamic dll loading", true);
>
> - init_console_handler (myself->ctty > 0);
>ForceCloseHandle1 (fork_info->forker_finished, forker_finished);
>
>pthread::atforkchild ();
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 4bb28c47b..15cba3610 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -635,6 +635,12 @@ child_info_spawn::worker (const char *prog_arg, const
> char *const *argv,
>if (ptys)
> ptys->fixup_after_attach (!iscygwin ());
>
> + if (!iscygwin ())
> + {
> + init_console_handler (myself->ctty > 0);
> + myself->ctty = 0;
> + }
> +
> loop:
>/* When ruid != euid we create the new process under the current
> original
> account and impersonate in child, this way maintaining the different
Sounds good to me.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
[PATCH v2 1/1] Cygwin: pty: Limit API hook to the program linked with the APIs.
- API hook used for pseudo console support causes slow down.
This patch limits API hook to only program which is linked
with the corresponding APIs. Normal cygwin program is not
linked with such APIs (such as WriteFile, etc...) directly,
therefore, no slow down occurs. However, console access by
cygwin.dll itself cannot switch the r/w pipe to pseudo console
side. Therefore, the code to switch it forcely to pseudo
console side is added to smallprint.cc and strace.cc.
---
winsup/cygwin/fhandler_tty.cc | 106 +++---
winsup/cygwin/smallprint.cc | 2 +
winsup/cygwin/strace.cc | 26 +
winsup/cygwin/winsup.h| 3 +
4 files changed, 66 insertions(+), 71 deletions(-)
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 262c41bfe..fadff59a3 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -88,6 +88,19 @@ set_switch_to_pcon (void)
}
}
+void
+set_ishybrid_and_switch_to_pcon (HANDLE h)
+{
+ DWORD dummy;
+ if (!isHybrid
+ && GetFileType (h) == FILE_TYPE_CHAR
+ && GetConsoleMode (h, &dummy))
+{
+ isHybrid = true;
+ set_switch_to_pcon ();
+}
+}
+
#define DEF_HOOK(name) static __typeof__ (name) *name##_Orig
DEF_HOOK (WriteFile);
DEF_HOOK (WriteConsoleA);
@@ -100,6 +113,7 @@ DEF_HOOK (WriteConsoleOutputW);
DEF_HOOK (WriteConsoleOutputCharacterA);
DEF_HOOK (WriteConsoleOutputCharacterW);
DEF_HOOK (WriteConsoleOutputAttribute);
+DEF_HOOK (SetConsoleTextAttribute);
DEF_HOOK (WriteConsoleInputA);
DEF_HOOK (WriteConsoleInputW);
DEF_HOOK (ReadConsoleInputA);
@@ -107,140 +121,137 @@ DEF_HOOK (ReadConsoleInputW);
DEF_HOOK (PeekConsoleInputA);
DEF_HOOK (PeekConsoleInputW);
-#define CHK_CONSOLE_ACCESS(h) \
-{ \
- DWORD dummy; \
- if (!isHybrid \
- && GetFileType (h) == FILE_TYPE_CHAR \
- && GetConsoleMode (h, &dummy)) \
-{ \
- isHybrid = true; \
- set_switch_to_pcon (); \
-} \
-}
static BOOL WINAPI
WriteFile_Hooked
(HANDLE h, LPCVOID p, DWORD l, LPDWORD n, LPOVERLAPPED o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteFile_Orig (h, p, l, n, o);
}
static BOOL WINAPI
WriteConsoleA_Hooked
(HANDLE h, LPCVOID p, DWORD l, LPDWORD n, LPVOID o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleA_Orig (h, p, l, n, o);
}
static BOOL WINAPI
WriteConsoleW_Hooked
(HANDLE h, LPCVOID p, DWORD l, LPDWORD n, LPVOID o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleW_Orig (h, p, l, n, o);
}
static BOOL WINAPI
ReadFile_Hooked
(HANDLE h, LPVOID p, DWORD l, LPDWORD n, LPOVERLAPPED o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return ReadFile_Orig (h, p, l, n, o);
}
static BOOL WINAPI
ReadConsoleA_Hooked
(HANDLE h, LPVOID p, DWORD l, LPDWORD n, LPVOID o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return ReadConsoleA_Orig (h, p, l, n, o);
}
static BOOL WINAPI
ReadConsoleW_Hooked
(HANDLE h, LPVOID p, DWORD l, LPDWORD n, LPVOID o)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return ReadConsoleW_Orig (h, p, l, n, o);
}
static BOOL WINAPI
WriteConsoleOutputA_Hooked
(HANDLE h, CONST CHAR_INFO *p, COORD s, COORD c, PSMALL_RECT r)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleOutputA_Orig (h, p, s, c, r);
}
static BOOL WINAPI
WriteConsoleOutputW_Hooked
(HANDLE h, CONST CHAR_INFO *p, COORD s, COORD c, PSMALL_RECT r)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleOutputW_Orig (h, p, s, c, r);
}
static BOOL WINAPI
WriteConsoleOutputCharacterA_Hooked
(HANDLE h, LPCSTR p, DWORD l, COORD c, LPDWORD n)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleOutputCharacterA_Orig (h, p, l, c, n);
}
static BOOL WINAPI
WriteConsoleOutputCharacterW_Hooked
(HANDLE h, LPCWSTR p, DWORD l, COORD c, LPDWORD n)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleOutputCharacterW_Orig (h, p, l, c, n);
}
static BOOL WINAPI
WriteConsoleOutputAttribute_Hooked
(HANDLE h, CONST WORD *a, DWORD l, COORD c, LPDWORD n)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleOutputAttribute_Orig (h, a, l, c, n);
}
static BOOL WINAPI
+SetConsoleTextAttribute_Hooked
+ (HANDLE h, WORD a)
+{
+ set_ishybrid_and_switch_to_pcon (h);
+ return SetConsoleTextAttribute_Orig (h, a);
+}
+static BOOL WINAPI
WriteConsoleInputA_Hooked
(HANDLE h, CONST INPUT_RECORD *r, DWORD l, LPDWORD n)
{
- CHK_CONSOLE_ACCESS (h);
+ set_ishybrid_and_switch_to_pcon (h);
return WriteConsoleInputA_Orig (h, r, l, n);
}
static BOOL WINAPI
WriteConsoleInputW_Hooked
(HANDLE h, CONST
[PATCH v2 0/1] Cygwin: pty: Limit API hook to the program linked with the APIs.
- API hook used for pseudo console support causes slow down. This patch limits API hook to only program which is linked with the corresponding APIs. Normal cygwin program is not linked with such APIs (such as WriteFile, etc...) directly, therefore, no slow down occurs. However, console access by cygwin.dll itself cannot switch the r/w pipe to pseudo console side. Therefore, the code to switch it forcely to pseudo console side is added to smallprint.cc and strace.cc. v2: Unify set_ishybrid_and_switch_to_pcon() and CHK_CONSOLE_ACCESS() because they have exactly the same functionality. Takashi Yano (1): Cygwin: pty: Limit API hook to the program linked with the APIs. winsup/cygwin/fhandler_tty.cc | 106 +++--- winsup/cygwin/smallprint.cc | 2 + winsup/cygwin/strace.cc | 26 + winsup/cygwin/winsup.h| 3 + 4 files changed, 66 insertions(+), 71 deletions(-) -- 2.21.0
[PATCH v2 0/1] Cygwin: pty: Add a workaround for ^C handling.
- Pseudo console support introduced by commit 169d65a5774acc76ce3f3feeedcbae7405aa9b57 sometimes cause random crash or freeze by pressing ^C while cygwin and non-cygwin processes are executed simultaneously in the same pty. This patch is a workaround for this issue. v2: Make the behaviour of pty and console identical. Takashi Yano (1): Cygwin: pty: Add a workaround for ^C handling. winsup/cygwin/fork.cc | 1 - winsup/cygwin/spawn.cc | 6 ++ 2 files changed, 6 insertions(+), 1 deletion(-) -- 2.21.0
[PATCH v2 1/1] Cygwin: pty: Add a workaround for ^C handling.
- Pseudo console support introduced by commit
169d65a5774acc76ce3f3feeedcbae7405aa9b57 sometimes cause random
crash or freeze by pressing ^C while cygwin and non-cygwin
processes are executed simultaneously in the same pty. This
patch is a workaround for this issue.
---
winsup/cygwin/fork.cc | 1 -
winsup/cygwin/spawn.cc | 6 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index a3a7e7505..0a929dffd 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -213,7 +213,6 @@ frok::child (volatile char * volatile here)
- terminate the current fork call even if the child is initialized. */
sync_with_parent ("performed fork fixups and dynamic dll loading", true);
- init_console_handler (myself->ctty > 0);
ForceCloseHandle1 (fork_info->forker_finished, forker_finished);
pthread::atforkchild ();
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 4bb28c47b..15cba3610 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -635,6 +635,12 @@ child_info_spawn::worker (const char *prog_arg, const char
*const *argv,
if (ptys)
ptys->fixup_after_attach (!iscygwin ());
+ if (!iscygwin ())
+ {
+ init_console_handler (myself->ctty > 0);
+ myself->ctty = 0;
+ }
+
loop:
/* When ruid != euid we create the new process under the current original
account and impersonate in child, this way maintaining the different
--
2.21.0
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Sep 4 21:49, Takashi Yano wrote: > On Wed, 4 Sep 2019 12:47:38 +0200 > Corinna Vinschen wrote: > > Why do you check the TERMs again here? After all, need_clear_screen > > is only true if one of these terms are used. > > Because, emacs seems to set environment TERM after fixup_after_exec() > is called. At the first check, TERM has the value of the terminal > in which emacs is executed. The first check is just in case. I still don't get it. The code in fixup_after_attach() is the only code snippet setting need_clear_screen = true. And that code also requires term != "dump" && term == "*emacs*" to set need_clear_screen. The code in reset_switch_to_pcon() requires that the need_clear_screen flag is true regardless of checking TERM. So this code depends on the successful TERM check from fixup_after_attach anyway. What am I missing? While at it, in fixup_after_attach(): + if (get_ttyp ()->num_pcon_attached_slaves == 0 && + term && strcmp (term, "dumb") && + term && !strstr (term, "emacs") && + !ALWAYS_USE_PCON) You're checking term for != NULL twice. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 1/1] Cygwin: pty: Limit API hook to the program linked with the APIs.
On Sep 4 22:46, Takashi Yano wrote: > - API hook used for pseudo console support causes slow down. > This patch limits API hook to only program which is linked > with the corresponding APIs. Normal cygwin program is not > linked with such APIs (such as WriteFile, etc...) directly, > therefore, no slow down occurs. However, console access by > cygwin.dll itself cannot switch the r/w pipe to pseudo console > side. Therefore, the code to switch it forcely to pseudo > console side is added to smallprint.cc and strace.cc. > --- > winsup/cygwin/fhandler_tty.cc | 106 +++--- > winsup/cygwin/smallprint.cc | 2 + > winsup/cygwin/strace.cc | 26 + > winsup/cygwin/winsup.h| 3 + > 4 files changed, 66 insertions(+), 71 deletions(-) Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 0/1] Cygwin: pty: Add a workaround for ^C handling.
On Sep 4 22:47, Takashi Yano wrote: > - Pseudo console support introduced by commit > 169d65a5774acc76ce3f3feeedcbae7405aa9b57 sometimes cause random > crash or freeze by pressing ^C while cygwin and non-cygwin > processes are executed simultaneously in the same pty. This > patch is a workaround for this issue. > > v2: > Make the behaviour of pty and console identical. > > Takashi Yano (1): > Cygwin: pty: Add a workaround for ^C handling. > > winsup/cygwin/fork.cc | 1 - > winsup/cygwin/spawn.cc | 6 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > -- > 2.21.0 Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Wed, 4 Sep 2019 15:55:03 +0200 Corinna Vinschen wrote: > The code in fixup_after_attach() is the only code snippet setting > need_clear_screen = true. And that code also requires term != "dump" && > term == "*emacs*" to set need_clear_screen. term != "*emacs*" > The code in reset_switch_to_pcon() requires that the need_clear_screen > flag is true regardless of checking TERM. So this code depends on the > successful TERM check from fixup_after_attach anyway. > > What am I missing? Two checking results may not be the same. Indeed, emacs changes TERM between two checks. fixup_after_attach() is called from fixup_after_exec(), which is called before executing the program code. reset_switch_to_pcon () is mainly called from PTY slave I/O functions. This is usually from the program code. The behaviour of the patch is as follows. First check : True True False False Second check: True False True False Clear screen: Yes NoNo No # True: neither dumb nor emacs* # False: either dumb or emacs* > + if (get_ttyp ()->num_pcon_attached_slaves == 0 && > + term && strcmp (term, "dumb") && > + term && !strstr (term, "emacs") && > + !ALWAYS_USE_PCON) > > You're checking term for != NULL twice. Oh my! -- Takashi Yano
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On 9/4/2019 10:42 AM, Takashi Yano wrote: > On Wed, 4 Sep 2019 15:55:03 +0200 > Corinna Vinschen wrote: >> The code in fixup_after_attach() is the only code snippet setting >> need_clear_screen = true. And that code also requires term != "dump" && >> term == "*emacs*" to set need_clear_screen. > > term != "*emacs*" > >> The code in reset_switch_to_pcon() requires that the need_clear_screen >> flag is true regardless of checking TERM. So this code depends on the >> successful TERM check from fixup_after_attach anyway. >> >> What am I missing? > > Two checking results may not be the same. Indeed, emacs changes > TERM between two checks. > > fixup_after_attach() is called from fixup_after_exec(), > which is called before executing the program code. > reset_switch_to_pcon () is mainly called from PTY slave I/O functions. > This is usually from the program code. But the first check (the one in fixup_after_attach()) could be dropped, couldn't it? Ken
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Sep 4 15:11, Ken Brown wrote: > On 9/4/2019 10:42 AM, Takashi Yano wrote: > > On Wed, 4 Sep 2019 15:55:03 +0200 > > Corinna Vinschen wrote: > >> The code in fixup_after_attach() is the only code snippet setting > >> need_clear_screen = true. And that code also requires term != "dump" && > >> term == "*emacs*" to set need_clear_screen. > > > > term != "*emacs*" > > > >> The code in reset_switch_to_pcon() requires that the need_clear_screen > >> flag is true regardless of checking TERM. So this code depends on the > >> successful TERM check from fixup_after_attach anyway. > >> > >> What am I missing? > > > > Two checking results may not be the same. Indeed, emacs changes > > TERM between two checks. > > > > fixup_after_attach() is called from fixup_after_exec(), > > which is called before executing the program code. > > reset_switch_to_pcon () is mainly called from PTY slave I/O functions. > > This is usually from the program code. > > But the first check (the one in fixup_after_attach()) could be dropped, > couldn't it? IIUC the second test first checks for need_clear_screen but then the TERM might have changed in the meantime which in turn requires to change the behaviour again. But yeah, this sound like the first patch is not actually required at all. Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Wed, 4 Sep 2019 17:19:05 +0200 Corinna Vinschen wrote: > > But the first check (the one in fixup_after_attach()) could be dropped, > > couldn't it? > > IIUC the second test first checks for need_clear_screen but then the > TERM might have changed in the meantime which in turn requires to change > the behaviour again. But yeah, this sound like the first patch is not > actually required at all. I was convinced. I will revise the patch. -- Takashi Yano
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On 2019-09-03 21:45, Takashi Yano wrote: > On Wed, 4 Sep 2019 12:34:31 +0900 > Takashi Yano wrote: >> Attached is the raw output from pseudo console when the screen shows >> the simple text below. >> >> from here >> [yano@Express5800-S70 ~]$ cmd >> Microsoft Windows [Version 10.0.18362.329] >> (c) 2019 Microsoft Corporation. All rights reserved. >> >> C:\cygwin\home\yano>exit >> [yano@Express5800-S70 ~]$ exit >> exit >> to here >> >> You will noticed that the screen is cleared if you execute >> cat pcon-output.log >> in a terminal which support ANSI escape sequences. > > This pcon-output.log was recorded in screen of 80x24 size. > Please try above in 80x24 terminal. That output seems to be generated by a shell or program running in the pty. If the recipient can not handle escape sequences, then the shell or program in the pty should be configured by setting e.g. TERM=dumb when launching the terminal, or in the shell environment. -- 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.
[PATCH v2 1/1] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
- Pseudo console support introduced by commit
169d65a5774acc76ce3f3feeedcbae7405aa9b57 shows garbage ^[[H^[[J in
some of emacs screens. These screens do not handle ANSI escape
sequences. Therefore, clear screen is disabled on these screens.
---
winsup/cygwin/fhandler_tty.cc | 19 ++-
winsup/cygwin/tty.cc | 1 +
winsup/cygwin/tty.h | 1 +
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index fadff59a3..a6844832b 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -962,6 +962,19 @@ skip_console_setting:
void
fhandler_pty_slave::reset_switch_to_pcon (void)
{
+ if (get_ttyp ()->need_clear_screen)
+{
+ const char *term = getenv ("TERM");
+ if (term && strcmp (term, "dumb") && !strstr (term, "emacs"))
+ {
+ /* FIXME: Clearing sequence may not be "^[[H^[[J"
+depending on the terminal type. */
+ DWORD n;
+ WriteFile (get_output_handle_cyg (), "\033[H\033[J", 6, &n, NULL);
+ }
+ get_ttyp ()->need_clear_screen = false;
+}
+
if (ALWAYS_USE_PCON)
return;
if (isHybrid)
@@ -2798,14 +2811,10 @@ fhandler_pty_slave::fixup_after_attach (bool
native_maybe)
/* Clear screen to synchronize pseudo console screen buffer
with real terminal. This is necessary because pseudo
console screen buffer is empty at start. */
- /* FIXME: Clearing sequence may not be "^[[H^[[J"
-depending on the terminal type. */
- DWORD n;
if (get_ttyp ()->num_pcon_attached_slaves == 0
&& !ALWAYS_USE_PCON)
/* Assume this is the first process using this pty slave. */
- WriteFile (get_output_handle_cyg (),
- "\033[H\033[J", 6, &n, NULL);
+ get_ttyp ()->need_clear_screen = true;
get_ttyp ()->num_pcon_attached_slaves ++;
}
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index 9244267c0..c94aee3ba 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -243,6 +243,7 @@ tty::init ()
pcon_pid = 0;
num_pcon_attached_slaves = 0;
TermCodePage = 20127; /* ASCII */
+ need_clear_screen = false;
}
HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index d59b2027d..c2b0490d0 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -104,6 +104,7 @@ private:
pid_t pcon_pid;
int num_pcon_attached_slaves;
UINT TermCodePage;
+ bool need_clear_screen;
public:
HANDLE from_master () const { return _from_master; }
--
2.21.0
[PATCH v2 0/1] Disable clear screen on new pty if TERM=dumb or emacs*.
- Pseudo console support introduced by commit 169d65a5774acc76ce3f3feeedcbae7405aa9b57 shows garbage ^[[H^[[J in some of emacs screens. These screens do not handle ANSI escape sequences. Therefore, clear screen is disabled on these screens. v2: Remove check for TERM in fixup_after_attach(). Takashi Yano (1): Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*. winsup/cygwin/fhandler_tty.cc | 19 ++- winsup/cygwin/tty.cc | 1 + winsup/cygwin/tty.h | 1 + 3 files changed, 16 insertions(+), 5 deletions(-) -- 2.21.0
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On Wed, 4 Sep 2019 11:22:42 -0600
Brian Inglis wrote:
> That output seems to be generated by a shell or program running in the pty.
> If the recipient can not handle escape sequences, then the shell or program in
> the pty should be configured by setting e.g. TERM=dumb when launching the
> terminal, or in the shell environment.
No. Output is almost same even if setting TERM=dumb.
See attached log.
Consider what should be the output of pseudo console if console
application like below is executed.
#include
int main()
{
HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
COORD p = {20, 10};
DWORD n;
SetConsoleCursorPosition(h, p);
SetConsoleTextAttribute(h, FOREGROUND_RED);
WriteConsole(h, "R", 1, &n, 0);
SetConsoleTextAttribute(h, FOREGROUND_GREEN);
WriteConsole(h, "G", 1, &n, 0);
SetConsoleTextAttribute(h, FOREGROUND_BLUE);
WriteConsole(h, "B", 1, &n, 0);
SetConsoleTextAttribute(h, FOREGROUND_RED|FOREGROUND_GREEN|FOREGROUND_BLUE);
WriteConsole(h, "W", 1, &n, 0);
WriteConsole(h, "\r\n", 2, &n, 0);
return 0;
}
Setting cursor posision and text color cannot be realized
without ANSI escape sequences.
Indeed, the output of the pseudo console by the program above is:
^[[?25l^[[11;21H^[[?25h^[[?25l^[[31mR^[[32mG^[[34mB^[[mW^M
^[[?25h
--
Takashi Yano
pcon-output-dumb.log
Description: Binary data
Re: [PATCH 2/2] Cygwin: pty: Disable clear screen on new pty if TERM=dumb or emacs*.
On 2019-09-04 19:13, Takashi Yano wrote:
> On Wed, 4 Sep 2019 11:22:42 -0600
> Brian Inglis wrote:
>> That output seems to be generated by a shell or program running in the pty.
>> If the recipient can not handle escape sequences, then the shell or program
>> in
>> the pty should be configured by setting e.g. TERM=dumb when launching the
>> terminal, or in the shell environment.
>
> No. Output is almost same even if setting TERM=dumb.
> See attached log.
>
> Consider what should be the output of pseudo console if console
> application like below is executed.
>
> #include
>
> int main()
> {
> HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
> COORD p = {20, 10};
> DWORD n;
> SetConsoleCursorPosition(h, p);
> SetConsoleTextAttribute(h, FOREGROUND_RED);
> WriteConsole(h, "R", 1, &n, 0);
> SetConsoleTextAttribute(h, FOREGROUND_GREEN);
> WriteConsole(h, "G", 1, &n, 0);
> SetConsoleTextAttribute(h, FOREGROUND_BLUE);
> WriteConsole(h, "B", 1, &n, 0);
> SetConsoleTextAttribute(h,
> FOREGROUND_RED|FOREGROUND_GREEN|FOREGROUND_BLUE);
> WriteConsole(h, "W", 1, &n, 0);
> WriteConsole(h, "\r\n", 2, &n, 0);
> return 0;
> }
>
> Setting cursor posision and text color cannot be realized
> without ANSI escape sequences.
>
> Indeed, the output of the pseudo console by the program above is:
> ^[[?25l^[[11;21H^[[?25h^[[?25l^[[31mR^[[32mG^[[34mB^[[mW^M
> ^[[?25h
So how do you tell the pseudo-console to generate only text not escape sequences
the recipient may not be prepared to deal with?
--
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.
[PATCH 0/1] Cygwin: pty: Fix select() with pseudo console support.
- select() did not work correctly when both read and except are polled simultaneously for the same fd and the r/w pipe is switched to pseudo console side. This patch fixes this isseu. Takashi Yano (1): Cygwin: pty: Fix select() with pseudo console support. winsup/cygwin/fhandler.h | 15 +++ winsup/cygwin/fhandler_tty.cc | 13 ++- winsup/cygwin/select.cc | 192 -- winsup/cygwin/select.h| 2 + 4 files changed, 207 insertions(+), 15 deletions(-) -- 2.21.0
[PATCH 1/1] Cygwin: pty: Fix select() with pseudo console support.
- select() did not work correctly when both read and except are
polled simultaneously for the same fd and the r/w pipe is switched
to pseudo console side. This patch fixes this isseu.
---
winsup/cygwin/fhandler.h | 15 +++
winsup/cygwin/fhandler_tty.cc | 13 ++-
winsup/cygwin/select.cc | 192 --
winsup/cygwin/select.h| 2 +
4 files changed, 207 insertions(+), 15 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index e8c165100..e72e11f7a 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2102,6 +2102,7 @@ class fhandler_pty_common: public fhandler_termios
{
return get_ttyp ()->hPseudoConsole;
}
+ bool to_be_read_from_pcon (void);
protected:
BOOL process_opost_output (HANDLE h,
@@ -2150,6 +2151,8 @@ class fhandler_pty_slave: public fhandler_pty_common
void fixup_after_exec ();
select_record *select_read (select_stuff *);
+ select_record *select_write (select_stuff *);
+ select_record *select_except (select_stuff *);
virtual char const *ttyname () { return pc.dev.name (); }
int __reg2 fstat (struct stat *buf);
int __reg3 facl (int, int, struct acl *);
@@ -2177,9 +2180,21 @@ class fhandler_pty_slave: public fhandler_pty_common
void push_to_pcon_screenbuffer (const char *ptr, size_t len);
void mask_switch_to_pcon (bool mask)
{
+if (!mask && get_ttyp ()->pcon_pid &&
+ get_ttyp ()->pcon_pid != myself->pid &&
+ kill (get_ttyp ()->pcon_pid, 0) == 0)
+ return;
get_ttyp ()->mask_switch_to_pcon = mask;
}
void fixup_after_attach (bool native_maybe);
+ pid_t get_pcon_pid (void)
+ {
+return get_ttyp ()->pcon_pid;
+ }
+ bool is_line_input (void)
+ {
+return get_ttyp ()->ti.c_lflag & ICANON;
+ }
};
#define __ptsname(buf, unit) __small_sprintf ((buf), "/dev/pty%d", (unit))
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index a6844832b..78c9c9128 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1223,6 +1223,13 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
return towrite;
}
+bool
+fhandler_pty_common::to_be_read_from_pcon (void)
+{
+ return get_ttyp ()->switch_to_pcon &&
+(!get_ttyp ()->mask_switch_to_pcon || ALWAYS_USE_PCON);
+}
+
void __reg3
fhandler_pty_slave::read (void *ptr, size_t& len)
{
@@ -1351,8 +1358,7 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
}
goto out;
}
- if (get_ttyp ()->switch_to_pcon &&
- (!get_ttyp ()->mask_switch_to_pcon || ALWAYS_USE_PCON))
+ if (to_be_read_from_pcon ())
{
if (!try_reattach_pcon ())
{
@@ -2129,8 +2135,7 @@ fhandler_pty_master::write (const void *ptr, size_t len)
/* Write terminal input to to_slave pipe instead of output_handle
if current application is native console application. */
- if (get_ttyp ()->switch_to_pcon &&
- (!get_ttyp ()->mask_switch_to_pcon || ALWAYS_USE_PCON))
+ if (to_be_read_from_pcon ())
{
char *buf;
size_t nlen;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index d29f3d2f4..4efc302df 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -667,9 +667,6 @@ peek_pipe (select_record *s, bool from_select)
fhm->flush_to_slave ();
}
break;
- case DEV_PTYS_MAJOR:
- ((fhandler_pty_slave *) fh)->reset_switch_to_pcon ();
- break;
default:
if (fh->get_readahead_valid ())
{
@@ -713,6 +710,7 @@ peek_pipe (select_record *s, bool from_select)
}
out:
+ h = fh->get_output_handle_cyg ();
if (s->write_selected && dev != FH_PIPER)
{
gotone += s->write_ready = pipe_data_available (s->fd, fh, h, true);
@@ -1176,33 +1174,173 @@ static int
verify_tty_slave (select_record *me, fd_set *readfds, fd_set *writefds,
fd_set *exceptfds)
{
- if (IsEventSignalled (me->h))
+ fhandler_pty_slave *ptys = (fhandler_pty_slave *) me->fh;
+ if (me->read_selected && !ptys->to_be_read_from_pcon () &&
+ IsEventSignalled (ptys->input_available_event))
me->read_ready = true;
return set_bits (me, readfds, writefds, exceptfds);
}
static int
-pty_slave_startup (select_record *s, select_stuff *)
+peek_pty_slave (select_record *s, bool from_select)
{
+ int gotone = 0;
fhandler_base *fh = (fhandler_base *) s->fh;
- ((fhandler_pty_slave *) fh)->mask_switch_to_pcon (true);
+ fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
+
+ ptys->reset_switch_to_pcon ();
+
+ if (s->read_selected)
+{
+ if (s->read_ready)
+ {
+ select_printf ("%s, already ready for read", fh->get_name ());
+ gotone = 1;
+ goto out;
+ }
+
+ if (fh->bg_check (SIGTTIN, true) <= bg_eof)
+ {
+ gotone = s->read_ready = true;
+ goto out;
+ }
+
+ if (ptys->to_be_read_from_pcon ())
+
