[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

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

[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
Pseudo console generates escape sequences on execution of non-cygwin
apps.  If the terminal does not support escape sequence, output will
be garbled. This patch prevents garbled output in dumb terminal by
disabling pseudo console.
---
 winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 8308bccf3..b6d58e97a 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
- if (ptys_primary->setup_pseudoconsole (&si_pcon,
-     mode != _P_OVERLAY && mode != _P_WAIT))
-  {
-    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-    si_tmp = &si_pcon.StartupInfo;
-    enable_pcon = true;
-  }
+ {
+  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+  /* If TERM is "dumb" or not set, disable pseudo console */
+  if (envblock)
+    {
+      bool term_is_set = false;
+      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
+ {
+  if (wcscmp (p, L"TERM=dumb") == 0)
+    nopcon = true;
+  if (wcsncmp (p, L"TERM=", 5) == 0)
+    term_is_set = true;
+ }
+      if (!term_is_set)
+ nopcon = true;
+    }
+  else
+    {
+      const char *term = getenv ("TERM");
+      if (!term || strcmp (term, "dumb") == 0)
+ nopcon = true;
+    }
+  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+    {
+      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+      si_tmp = &si_pcon.StartupInfo;
+      enable_pcon = true;
+    }
+ }
 
     loop:
       /* When ruid != euid we create the new process under the current original
--
2.28.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
Hi Takashi,

On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> Pseudo console generates escape sequences on execution of non-cygwin
> apps.  If the terminal does not support escape sequence, output will
> be garbled. This patch prevents garbled output in dumb terminal by
> disabling pseudo console.

I'm a bit puzzled by this patch.  We had code handling emacs and dumb
terminals explicitely in the early forms of the first incarnation of
the pseudo tty code, but fortunately you found a way to handle this
without hardcoding terminal types into Cygwin.  Why do you think we
have to do this now?


Thanks,
Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Wed, 26 Aug 2020 19:36:06 +0200
Corinna Vinschen wrote:

> On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > Pseudo console generates escape sequences on execution of non-cygwin
> > apps.  If the terminal does not support escape sequence, output will
> > be garbled. This patch prevents garbled output in dumb terminal by
> > disabling pseudo console.
>
> I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> terminals explicitely in the early forms of the first incarnation of
> the pseudo tty code, but fortunately you found a way to handle this
> without hardcoding terminal types into Cygwin.  Why do you think we
> have to do this now?

What previously disccussed was the problem that the clearing
screen at pty startup displays garbage (^[[H^[[2J) in emacs.
Finally, this was settled by eliminating clear-screen and
triggering redraw-screen instead at the first execution of
non-cygwin app.

However, the problem reported in
https://cygwin.com/pipermail/cygwin/2020-August/245983.html
still remains.

What's worse in the new implementation, pseudo console sends
ESC[6n (querying cursor position) internally on startup and
waits for a response. This causes hang if pseudo console is
started in dumb terminal.

This patch is for fixing this issue.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:

> On Wed, 26 Aug 2020 19:36:06 +0200
> Corinna Vinschen wrote:
> > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > Pseudo console generates escape sequences on execution of non-cygwin
> > > apps.  If the terminal does not support escape sequence, output will
> > > be garbled. This patch prevents garbled output in dumb terminal by
> > > disabling pseudo console.
> >
> > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > terminals explicitely in the early forms of the first incarnation of
> > the pseudo tty code, but fortunately you found a way to handle this
> > without hardcoding terminal types into Cygwin.  Why do you think we
> > have to do this now?
>
> What previously disccussed was the problem that the clearing
> screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> Finally, this was settled by eliminating clear-screen and
> triggering redraw-screen instead at the first execution of
> non-cygwin app.
>
> However, the problem reported in
> https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> still remains.
>
> What's worse in the new implementation, pseudo console sends
> ESC[6n (querying cursor position) internally on startup and
> waits for a response. This causes hang if pseudo console is
> started in dumb terminal.
>
> This patch is for fixing this issue.

Would it be feasible to implement this using a timeout instead?
If the response isn't sent within, say, 100ms, just skip it?


Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Thu, 27 Aug 2020 10:47:56 +0200
Corinna Vinschen wrote:

> On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:
> > On Wed, 26 Aug 2020 19:36:06 +0200
> > Corinna Vinschen wrote:
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> > >
> > > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > > terminals explicitely in the early forms of the first incarnation of
> > > the pseudo tty code, but fortunately you found a way to handle this
> > > without hardcoding terminal types into Cygwin.  Why do you think we
> > > have to do this now?
> >
> > What previously disccussed was the problem that the clearing
> > screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> > Finally, this was settled by eliminating clear-screen and
> > triggering redraw-screen instead at the first execution of
> > non-cygwin app.
> >
> > However, the problem reported in
> > https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> > still remains.
> >
> > What's worse in the new implementation, pseudo console sends
> > ESC[6n (querying cursor position) internally on startup and
> > waits for a response. This causes hang if pseudo console is
> > started in dumb terminal.
> >
> > This patch is for fixing this issue.
>
> Would it be feasible to implement this using a timeout instead?
> If the response isn't sent within, say, 100ms, just skip it?

Hang is caused at CreateProcessW() call, so there is no way to
use time out. It is possible to send ESC[6n before creating
pseudo console for a test and wait for responce with timeout,
however, if the terminal is dumb, garbage ^[[6n will be displayed.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
On Aug 27 17:59, Takashi Yano via Cygwin-patches wrote:

> On Thu, 27 Aug 2020 10:47:56 +0200
> Corinna Vinschen wrote:
> > On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:
> > > On Wed, 26 Aug 2020 19:36:06 +0200
> > > Corinna Vinschen wrote:
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > > >
> > > > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > > > terminals explicitely in the early forms of the first incarnation of
> > > > the pseudo tty code, but fortunately you found a way to handle this
> > > > without hardcoding terminal types into Cygwin.  Why do you think we
> > > > have to do this now?
> > >
> > > What previously disccussed was the problem that the clearing
> > > screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> > > Finally, this was settled by eliminating clear-screen and
> > > triggering redraw-screen instead at the first execution of
> > > non-cygwin app.
> > >
> > > However, the problem reported in
> > > https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> > > still remains.
> > >
> > > What's worse in the new implementation, pseudo console sends
> > > ESC[6n (querying cursor position) internally on startup and
> > > waits for a response. This causes hang if pseudo console is
> > > started in dumb terminal.
> > >
> > > This patch is for fixing this issue.
> >
> > Would it be feasible to implement this using a timeout instead?
> > If the response isn't sent within, say, 100ms, just skip it?
>
> Hang is caused at CreateProcessW() call, so there is no way to
> use time out. It is possible to send ESC[6n before creating
> pseudo console for a test and wait for responce with timeout,
> however, if the terminal is dumb, garbage ^[[6n will be displayed.

Doesn't sound so great either.  That would slow down process
startup on dumb terminal a lot, right?

Ok, if you don't see any other way to fix that, I'll push that patch
in a while.


Thanks,
Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
In reply to this post by cygwin-patches mailing list
Hi Takashi,

On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:

> Pseudo console generates escape sequences on execution of non-cygwin
> apps.  If the terminal does not support escape sequence, output will
> be garbled. This patch prevents garbled output in dumb terminal by
> disabling pseudo console.
> ---
>  winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 8308bccf3..b6d58e97a 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>        ZeroMemory (&si_pcon, sizeof (si_pcon));
>        STARTUPINFOW *si_tmp = &si;
>        if (!iscygwin () && ptys_primary && is_console_app (runpath))
> - if (ptys_primary->setup_pseudoconsole (&si_pcon,
> -     mode != _P_OVERLAY && mode != _P_WAIT))
> -  {
> -    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> -    si_tmp = &si_pcon.StartupInfo;
> -    enable_pcon = true;
> -  }
> + {
> +  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
> +  /* If TERM is "dumb" or not set, disable pseudo console */
> +  if (envblock)
> +    {
> +      bool term_is_set = false;
> +      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
> + {
> +  if (wcscmp (p, L"TERM=dumb") == 0)
> +    nopcon = true;
> +  if (wcsncmp (p, L"TERM=", 5) == 0)
> +    term_is_set = true;
> + }
> +      if (!term_is_set)
> + nopcon = true;
> +    }
> +  else
> +    {
> +      const char *term = getenv ("TERM");
> +      if (!term || strcmp (term, "dumb") == 0)
> + nopcon = true;
> +    }
> +  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
> +    {
> +      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> +      si_tmp = &si_pcon.StartupInfo;
> +      enable_pcon = true;
> +    }
> + }
>  
>      loop:
>        /* When ruid != euid we create the new process under the current original
> --
> 2.28.0

Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
method so the TERM specific stuff is done in the fhandler code, not
in spawn.cc?


Thanks,
Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
Hi Corinna,

On Fri, 28 Aug 2020 15:45:03 +0200
Corinna Vinschen wrote:

> Hi Takashi,
>
> On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > Pseudo console generates escape sequences on execution of non-cygwin
> > apps.  If the terminal does not support escape sequence, output will
> > be garbled. This patch prevents garbled output in dumb terminal by
> > disabling pseudo console.
> > ---
> >  winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > index 8308bccf3..b6d58e97a 100644
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >        ZeroMemory (&si_pcon, sizeof (si_pcon));
> >        STARTUPINFOW *si_tmp = &si;
> >        if (!iscygwin () && ptys_primary && is_console_app (runpath))
> > - if (ptys_primary->setup_pseudoconsole (&si_pcon,
> > -     mode != _P_OVERLAY && mode != _P_WAIT))
> > -  {
> > -    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> > -    si_tmp = &si_pcon.StartupInfo;
> > -    enable_pcon = true;
> > -  }
> > + {
> > +  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
> > +  /* If TERM is "dumb" or not set, disable pseudo console */
> > +  if (envblock)
> > +    {
> > +      bool term_is_set = false;
> > +      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
> > + {
> > +  if (wcscmp (p, L"TERM=dumb") == 0)
> > +    nopcon = true;
> > +  if (wcsncmp (p, L"TERM=", 5) == 0)
> > +    term_is_set = true;
> > + }
> > +      if (!term_is_set)
> > + nopcon = true;
> > +    }
> > +  else
> > +    {
> > +      const char *term = getenv ("TERM");
> > +      if (!term || strcmp (term, "dumb") == 0)
> > + nopcon = true;
> > +    }
> > +  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
> > +    {
> > +      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> > +      si_tmp = &si_pcon.StartupInfo;
> > +      enable_pcon = true;
> > +    }
> > + }
> >  
> >      loop:
> >        /* When ruid != euid we create the new process under the current original
> > --
> > 2.28.0
>
> Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> method so the TERM specific stuff is done in the fhandler code, not
> in spawn.cc?

Thansk for the suggestion. I will submit v2 patch.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
Hi Corinna,

On Sat, 29 Aug 2020 04:25:54 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> Hi Corinna,
>
> On Fri, 28 Aug 2020 15:45:03 +0200
> Corinna Vinschen wrote:
> > Hi Takashi,
> >
> > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > Pseudo console generates escape sequences on execution of non-cygwin
> > > apps.  If the terminal does not support escape sequence, output will
> > > be garbled. This patch prevents garbled output in dumb terminal by
> > > disabling pseudo console.
[...]
> >
> > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > method so the TERM specific stuff is done in the fhandler code, not
> > in spawn.cc?
>
> Thansk for the suggestion. I will submit v2 patch.

What do you think of v3 patch attached? With this patch,
terminal capability is checked by looking into terminfo
database rather than just checking terminal name. This
solution is more essential for the issue to be solved,
I think.

One downside of this solution, I noticed, is that tmux
sets TERM to "screen", which does not have CSI6n, by
default. As a result, pseudo console is disbled in tmux
by default. Setting TERM, such as screen.xterm-256color,
will solve the issue.

--
Takashi Yano <[hidden email]>

v3-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Sat, 29 Aug 2020 20:12:28 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> Hi Corinna,
>
> On Sat, 29 Aug 2020 04:25:54 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > Hi Corinna,
> >
> > On Fri, 28 Aug 2020 15:45:03 +0200
> > Corinna Vinschen wrote:
> > > Hi Takashi,
> > >
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> [...]
> > >
> > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > method so the TERM specific stuff is done in the fhandler code, not
> > > in spawn.cc?
> >
> > Thansk for the suggestion. I will submit v2 patch.
>
> What do you think of v3 patch attached? With this patch,
> terminal capability is checked by looking into terminfo
> database rather than just checking terminal name. This
> solution is more essential for the issue to be solved,
> I think.
>
> One downside of this solution, I noticed, is that tmux
> sets TERM to "screen", which does not have CSI6n, by
> default. As a result, pseudo console is disbled in tmux
> by default. Setting TERM, such as screen.xterm-256color,
> will solve the issue.
Attached is the v4 patch. Small bug was fixed.

--
Takashi Yano <[hidden email]>

v4-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Sat, 29 Aug 2020 22:14:20 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> On Sat, 29 Aug 2020 20:12:28 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > Hi Corinna,
> >
> > On Sat, 29 Aug 2020 04:25:54 +0900
> > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > Hi Corinna,
> > >
> > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > Corinna Vinschen wrote:
> > > > Hi Takashi,
> > > >
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > [...]
> > > >
> > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > in spawn.cc?
> > >
> > > Thansk for the suggestion. I will submit v2 patch.
> >
> > What do you think of v3 patch attached? With this patch,
> > terminal capability is checked by looking into terminfo
> > database rather than just checking terminal name. This
> > solution is more essential for the issue to be solved,
> > I think.
> >
> > One downside of this solution, I noticed, is that tmux
> > sets TERM to "screen", which does not have CSI6n, by
> > default. As a result, pseudo console is disbled in tmux
> > by default. Setting TERM, such as screen.xterm-256color,
> > will solve the issue.
>
> Attached is the v4 patch. Small bug was fixed.
Bug fixed again. v5 patch attached.

--
Takashi Yano <[hidden email]>

v5-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Sun, 30 Aug 2020 05:25:06 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> On Sat, 29 Aug 2020 22:14:20 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > On Sat, 29 Aug 2020 20:12:28 +0900
> > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > Hi Corinna,
> > >
> > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > Hi Corinna,
> > > >
> > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > Corinna Vinschen wrote:
> > > > > Hi Takashi,
> > > > >
> > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > disabling pseudo console.
> > > [...]
> > > > >
> > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > in spawn.cc?
> > > >
> > > > Thansk for the suggestion. I will submit v2 patch.
> > >
> > > What do you think of v3 patch attached? With this patch,
> > > terminal capability is checked by looking into terminfo
> > > database rather than just checking terminal name. This
> > > solution is more essential for the issue to be solved,
> > > I think.
> > >
> > > One downside of this solution, I noticed, is that tmux
> > > sets TERM to "screen", which does not have CSI6n, by
> > > default. As a result, pseudo console is disbled in tmux
> > > by default. Setting TERM, such as screen.xterm-256color,
> > > will solve the issue.
> >
> > Attached is the v4 patch. Small bug was fixed.
>
> Bug fixed again. v5 patch attached.
v6: Refactor the code a little.

--
Takashi Yano <[hidden email]>

v6-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Sun, 30 Aug 2020 06:13:17 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> On Sun, 30 Aug 2020 05:25:06 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > On Sat, 29 Aug 2020 22:14:20 +0900
> > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > On Sat, 29 Aug 2020 20:12:28 +0900
> > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > Hi Corinna,
> > > >
> > > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > > Hi Corinna,
> > > > >
> > > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > > Corinna Vinschen wrote:
> > > > > > Hi Takashi,
> > > > > >
> > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > > disabling pseudo console.
> > > > [...]
> > > > > >
> > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > > in spawn.cc?
> > > > >
> > > > > Thansk for the suggestion. I will submit v2 patch.
> > > >
> > > > What do you think of v3 patch attached? With this patch,
> > > > terminal capability is checked by looking into terminfo
> > > > database rather than just checking terminal name. This
> > > > solution is more essential for the issue to be solved,
> > > > I think.
> > > >
> > > > One downside of this solution, I noticed, is that tmux
> > > > sets TERM to "screen", which does not have CSI6n, by
> > > > default. As a result, pseudo console is disbled in tmux
> > > > by default. Setting TERM, such as screen.xterm-256color,
> > > > will solve the issue.
> > >
> > > Attached is the v4 patch. Small bug was fixed.
> >
> > Bug fixed again. v5 patch attached.
>
> v6: Refactor the code a little.
v7: Fix another bug again.

--
Takashi Yano <[hidden email]>

v7-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
On Sun, 30 Aug 2020 16:42:17 +0900
Takashi Yano via Cygwin-patches <[hidden email]> wrote:

> On Sun, 30 Aug 2020 06:13:17 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > On Sun, 30 Aug 2020 05:25:06 +0900
> > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > On Sat, 29 Aug 2020 22:14:20 +0900
> > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > On Sat, 29 Aug 2020 20:12:28 +0900
> > > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > > Hi Corinna,
> > > > >
> > > > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > > > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > > > > Hi Corinna,
> > > > > >
> > > > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > > > Corinna Vinschen wrote:
> > > > > > > Hi Takashi,
> > > > > > >
> > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > > > disabling pseudo console.
> > > > > [...]
> > > > > > >
> > > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > > > in spawn.cc?
> > > > > >
> > > > > > Thansk for the suggestion. I will submit v2 patch.
> > > > >
> > > > > What do you think of v3 patch attached? With this patch,
> > > > > terminal capability is checked by looking into terminfo
> > > > > database rather than just checking terminal name. This
> > > > > solution is more essential for the issue to be solved,
> > > > > I think.
> > > > >
> > > > > One downside of this solution, I noticed, is that tmux
> > > > > sets TERM to "screen", which does not have CSI6n, by
> > > > > default. As a result, pseudo console is disbled in tmux
> > > > > by default. Setting TERM, such as screen.xterm-256color,
> > > > > will solve the issue.
> > > >
> > > > Attached is the v4 patch. Small bug was fixed.
> > >
> > > Bug fixed again. v5 patch attached.
> >
> > v6: Refactor the code a little.
>
> v7: Fix another bug again.
v8: Yet another bug fix.

--
Takashi Yano <[hidden email]>

v8-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
In reply to this post by cygwin-patches mailing list
Hi Takashi,

On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote:

> Hi Corinna,
>
> On Sat, 29 Aug 2020 04:25:54 +0900
> Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > Hi Corinna,
> >
> > On Fri, 28 Aug 2020 15:45:03 +0200
> > Corinna Vinschen wrote:
> > > Hi Takashi,
> > >
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> [...]
> > >
> > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > method so the TERM specific stuff is done in the fhandler code, not
> > > in spawn.cc?
> >
> > Thansk for the suggestion. I will submit v2 patch.
>
> What do you think of v3 patch attached? With this patch,
> terminal capability is checked by looking into terminfo
> database rather than just checking terminal name. This
> solution is more essential for the issue to be solved,
> I think.
>
> One downside of this solution, I noticed, is that tmux
> sets TERM to "screen", which does not have CSI6n, by
> default. As a result, pseudo console is disbled in tmux
> by default. Setting TERM, such as screen.xterm-256color,
> will solve the issue.

I like the idea in general, but isn't there a noticable perfomance hit?


Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
Hi Corinna,

On Sun, 30 Aug 2020 14:49:25 +0200
Corinna Vinschen wrote:

> Hi Takashi,
>
> On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote:
> > Hi Corinna,
> >
> > On Sat, 29 Aug 2020 04:25:54 +0900
> > Takashi Yano via Cygwin-patches <[hidden email]> wrote:
> > > Hi Corinna,
> > >
> > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > Corinna Vinschen wrote:
> > > > Hi Takashi,
> > > >
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > [...]
> > > >
> > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > in spawn.cc?
> > >
> > > Thansk for the suggestion. I will submit v2 patch.
> >
> > What do you think of v3 patch attached? With this patch,
> > terminal capability is checked by looking into terminfo
> > database rather than just checking terminal name. This
> > solution is more essential for the issue to be solved,
> > I think.
> >
> > One downside of this solution, I noticed, is that tmux
> > sets TERM to "screen", which does not have CSI6n, by
> > default. As a result, pseudo console is disbled in tmux
> > by default. Setting TERM, such as screen.xterm-256color,
> > will solve the issue.
>
> I like the idea in general, but isn't there a noticable perfomance hit?
I have measured the startup time of non-cygwin process
with v2 and v8 patch.

mintty with v2    : 92.7ms
emacs-dumb with v2: 22.8ms

mintty with v8    : 94.6ms
emacs-dumb with v8: 22.7ms

There is not noticeable difference more than measurement
error.

By the way, I have implemented timeout strategy for CSI6n, you
mentioned in the previous comment, for a test. This check is
done only on the first execution of non-cygwin apps in the pty.
With this patch, first checks if the terminfo has cursor_home
(ESC [H). If terminfo has cursor_home, ANSI escape sequence is
supposed to be supported. In this case, I expect not to display
garbage "^[[6n" even if CSI6n is sent because the parser ignores
unsupported CSI sequences.

With this implementation, pseudo console works in tmux as well
even if TERM=screen.

Please have a look v9 patch attached.

The performance of v9 is also checked.

mintty with v9    : 93.9ms
emacs-dumb with v9: 22.8ms
ANSI without CSI6n: 22.8ms

[the first time in v9]
mintty            : 94.8ms
emacs-dumb        : 22.5ms
ANSI without CSI6n: 63.5ms

Most of the results are the same as v2 and v8 except for the
first execution of non-cygwin apps in ansi terminal without
CSI6n. It takes about 40ms (timeout) longer than dumb terminal
in ANSI terminal without CSI6n support.

However, this causes only on the first execution of non-cygwin
apps in pty.

I think this is the most reasonable one I have ever proposed.

--
Takashi Yano <[hidden email]>

v9-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

Corinna Vinschen-2
Hi Takashi,

On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:

> Hi Corinna,
>
> On Sun, 30 Aug 2020 14:49:25 +0200
> Corinna Vinschen wrote:
> > I like the idea in general, but isn't there a noticable perfomance hit?
>
> I have measured the startup time of non-cygwin process
> with v2 and v8 patch.
>
> mintty with v2    : 92.7ms
> emacs-dumb with v2: 22.8ms
>
> mintty with v8    : 94.6ms
> emacs-dumb with v8: 22.7ms
>
> There is not noticeable difference more than measurement
> error.

Great.

> By the way, I have implemented timeout strategy for CSI6n, you
> mentioned in the previous comment, for a test. This check is
> done only on the first execution of non-cygwin apps in the pty.
> With this patch, first checks if the terminfo has cursor_home
> (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> supposed to be supported. In this case, I expect not to display
> garbage "^[[6n" even if CSI6n is sent because the parser ignores
> unsupported CSI sequences.
>
> With this implementation, pseudo console works in tmux as well
> even if TERM=screen.
>
> Please have a look v9 patch attached.
>
> The performance of v9 is also checked.
>
> mintty with v9    : 93.9ms
> emacs-dumb with v9: 22.8ms
> ANSI without CSI6n: 22.8ms
>
> [the first time in v9]
> mintty            : 94.8ms
> emacs-dumb        : 22.5ms
> ANSI without CSI6n: 63.5ms
>
> Most of the results are the same as v2 and v8 except for the
> first execution of non-cygwin apps in ansi terminal without
> CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> in ANSI terminal without CSI6n support.
>
> However, this causes only on the first execution of non-cygwin
> apps in pty.
>
> I think this is the most reasonable one I have ever proposed.

This looks good, but I have a few nits:

- Don't use GetTickCount().  Use GetTickCount64().  Otherwise the code
  is prone to the 49 days tick counter overflow problem(*).

- get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().

- Following the maybe_dumb label, you're setting has_csi6n and
  has_set_title to false, but these are the default values anyway.
  Also, setting has_set_title to false in the preceeding else branch
  seems unnecessary.  Do you want that for clarity?  If not I'd drop
  them.

With these minor problems fixed, I'm happy to push the change.

(*) I just noticed belatedly that GetTickCount() is used in the tty code
    already.  Can you please change pcon_last_time to ULONGLONG and use
    GetTickCount64() instead?


Thanks,
Corinna
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

cygwin-patches mailing list
Hi Corinna,

Thank you for checking the patch.

On Mon, 31 Aug 2020 10:16:51 +0200
Corinna Vinschen wrote:

> Hi Takashi,
>
> On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:
> > Hi Corinna,
> >
> > On Sun, 30 Aug 2020 14:49:25 +0200
> > Corinna Vinschen wrote:
> > > I like the idea in general, but isn't there a noticable perfomance hit?
> >
> > I have measured the startup time of non-cygwin process
> > with v2 and v8 patch.
> >
> > mintty with v2    : 92.7ms
> > emacs-dumb with v2: 22.8ms
> >
> > mintty with v8    : 94.6ms
> > emacs-dumb with v8: 22.7ms
> >
> > There is not noticeable difference more than measurement
> > error.
>
> Great.
>
> > By the way, I have implemented timeout strategy for CSI6n, you
> > mentioned in the previous comment, for a test. This check is
> > done only on the first execution of non-cygwin apps in the pty.
> > With this patch, first checks if the terminfo has cursor_home
> > (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> > supposed to be supported. In this case, I expect not to display
> > garbage "^[[6n" even if CSI6n is sent because the parser ignores
> > unsupported CSI sequences.
> >
> > With this implementation, pseudo console works in tmux as well
> > even if TERM=screen.
> >
> > Please have a look v9 patch attached.
> >
> > The performance of v9 is also checked.
> >
> > mintty with v9    : 93.9ms
> > emacs-dumb with v9: 22.8ms
> > ANSI without CSI6n: 22.8ms
> >
> > [the first time in v9]
> > mintty            : 94.8ms
> > emacs-dumb        : 22.5ms
> > ANSI without CSI6n: 63.5ms
> >
> > Most of the results are the same as v2 and v8 except for the
> > first execution of non-cygwin apps in ansi terminal without
> > CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> > in ANSI terminal without CSI6n support.
> >
> > However, this causes only on the first execution of non-cygwin
> > apps in pty.
> >
> > I think this is the most reasonable one I have ever proposed.
>
> This looks good, but I have a few nits:
>
> - Don't use GetTickCount().  Use GetTickCount64().  Otherwise the code
>   is prone to the 49 days tick counter overflow problem(*).

This does not matter because the code is
  if (GetTickCount() - t0 > 40)
rather than
  if (GetTickCount() > t0 + 40)
and because DWORD is 32bit unsigned integer.

If the overflow occurs within the timeout period, for example,
t0 = 0xfffffff0 and GetTickCount() becomes 0x00000002,
GetTickCount() - t0 equals to 18 (0x12) as expected.

If the code is
  if (GetTickCount() > t0 + 40)
and t0 = 0xfffffff0 and GetTickCount() is 0xfffffff1,
t0 + 40 equals to 24 (0x18)
so GetTickCount() > t0 + 40 is true against expectation.

> - get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().

pcon_start is cleared to false in master write() if responce to CSI6n
is sent. Therefore, it is necessary to set again after the responce.
I will added the comment in the source.

> - Following the maybe_dumb label, you're setting has_csi6n and
>   has_set_title to false, but these are the default values anyway.
>   Also, setting has_set_title to false in the preceeding else branch
>   seems unnecessary.  Do you want that for clarity?  If not I'd drop
>   them.

You are right. I will submit the v10 patch.

> With these minor problems fixed, I'm happy to push the change.
>
> (*) I just noticed belatedly that GetTickCount() is used in the tty code
>     already.  Can you please change pcon_last_time to ULONGLONG and use
>     GetTickCount64() instead?

Same here. That does not matter.

--
Takashi Yano <[hidden email]>