[PATCH] vt: properly ignore xterm-256 colour codes

[PATCH] vt: properly ignore xterm-256 colour codes

[Adam Borowski]

This is not a bug on our side, but a misdesign in ITU T.416, yet with
all popular terminals supporting these codes, people consider this to
be a bug in Linux.  By breaking the design principles behind SGR codes
(gracefully ignoring unsupported ones should not require knowing about
them), 256 colour ones tend to turn blinking on before invoking an
arbitrary unrelated command.

This commit doesn't add such support, merely skips such codes without
ill effects.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/vt.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index c677829..f7aaa28 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1300,13 +1300,27 @@ static void csi_m(struct vc_data *vc)
 			case 27:
 				vc->vc_reverse = 0;
 				break;
-			case 38: /* ANSI X3.64-1979 (SCO-ish?)
-				  * Enables underscore, white foreground
-				  * with white underscore (Linux - use
-				  * default foreground).
+			case 38:
+			case 48: /* ITU T.416
+				  * Higher colour modes.
+				  * They break the usual properties of SGR codes
+				  * and thus need to be detected and ignored by
+				  * hand.  Strictly speaking, that standard also
+				  * wants : rather than ; as separators, contrary
+				  * to ECMA-48, but no one produces such codes
+				  * and almost no one accepts them.
 				  */
-				vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
-				vc->vc_underline = 1;
+				i++;
+				if (i > vc->vc_npar)
+					break;
+				if (vc->vc_par[i] == 5)      /* 256 colours */
+					i++;                 /* ubiquitous */
+				else if (vc->vc_par[i] == 2) /* 24 bit colours */
+					i += 3;              /* extremely rare */
+				/* Subcommands 3 (CMY) and 4 (CMYK) are so insane
+				 * that detecting them is not worth the few extra
+				 * bytes of kernel's size.
+				 */
 				break;
 			case 39: /* ANSI X3.64-1979 (SCO-ish?)
 				  * Disable underline option.
-- 
1.8.4.rc3

Re: [PATCH] vt: properly ignore xterm-256 colour codes

[David Herrmann]

Hi

On Fri, Jul 12, 2013 at 10:23 PM, Adam Borowski <kilobyte@angband.pl> wrote:
> This is not a bug on our side, but a misdesign in ITU T.416, yet with
> all popular terminals supporting these codes, people consider this to
> be a bug in Linux.  By breaking the design principles behind SGR codes
> (gracefully ignoring unsupported ones should not require knowing about
> them), 256 colour ones tend to turn blinking on before invoking an
> arbitrary unrelated command.
>
> This commit doesn't add such support, merely skips such codes without
> ill effects.
>
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  drivers/tty/vt/vt.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index c677829..f7aaa28 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1300,13 +1300,27 @@ static void csi_m(struct vc_data *vc)
>                         case 27:
>                                 vc->vc_reverse = 0;
>                                 break;
> -                       case 38: /* ANSI X3.64-1979 (SCO-ish?)
> -                                 * Enables underscore, white foreground
> -                                 * with white underscore (Linux - use
> -                                 * default foreground).
> +                       case 38:
> +                       case 48: /* ITU T.416
> +                                 * Higher colour modes.
> +                                 * They break the usual properties of SGR codes
> +                                 * and thus need to be detected and ignored by
> +                                 * hand.  Strictly speaking, that standard also
> +                                 * wants : rather than ; as separators, contrary
> +                                 * to ECMA-48, but no one produces such codes
> +                                 * and almost no one accepts them.
>                                   */
> -                               vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
> -                               vc->vc_underline = 1;

You break the old behavior here. _Iff_ this is what you want, then
please do that in another commit. Explicitly state that "38" is used
for 256color and shouldn't turn on underline+default-col. The SCO-ish
behavior is weird, indeed, but breaking it silently is not ok.

> +                               i++;
> +                               if (i > vc->vc_npar)

This should be ">=", but the for()-loop does allow your ">". So unless
someone fixes the for-loop to use "<" (do a ++vc->vc_npar before it,
if it's correct. But blindly doing "<=" is really irritating) I think
this is ok.

> +                                       break;
> +                               if (vc->vc_par[i] == 5)      /* 256 colours */
> +                                       i++;                 /* ubiquitous */
> +                               else if (vc->vc_par[i] == 2) /* 24 bit colours */
> +                                       i += 3;              /* extremely rare */
> +                               /* Subcommands 3 (CMY) and 4 (CMYK) are so insane
> +                                * that detecting them is not worth the few extra
> +                                * bytes of kernel's size.
> +                                */

I can confirm that 38/48 are "parsed" correctly here. Skipping '3' and
'4' seems right, too. Haven't seen anyone implementing them. Even '2'
seems to be almost unused.

Btw., you should put Greg Kroah-Hartman and Andrew Morton on CC. Both
are the most likely to pick this up.

Regards
David

>                                 break;
>                         case 39: /* ANSI X3.64-1979 (SCO-ish?)
>                                   * Disable underline option.
> --
> 1.8.4.rc3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] vt: properly ignore xterm-256 colour codes

[Adam Borowski]

This is not a bug on our side, but a misdesign in ITU T.416, yet with
all popular terminals supporting these codes, people consider this to
be a bug in Linux.  By breaking the design principles behind SGR codes
(gracefully ignoring unsupported ones should not require knowing about
them), 256 colour ones tend to turn blinking on before invoking an
arbitrary unrelated command.

This commit doesn't add such support, merely skips such codes without
ill effects.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/vt.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index ba528bb..bc441cf 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1300,6 +1300,28 @@ static void csi_m(struct vc_data *vc)
 			case 27:
 				vc->vc_reverse = 0;
 				break;
+			case 38:
+			case 48: /* ITU T.416
+				  * Higher colour modes.
+				  * They break the usual properties of SGR codes
+				  * and thus need to be detected and ignored by
+				  * hand.  Strictly speaking, that standard also
+				  * wants : rather than ; as separators, contrary
+				  * to ECMA-48, but no one produces such codes
+				  * and almost no one accepts them.
+				  */
+				i++;
+				if (i > vc->vc_npar)
+					break;
+				if (vc->vc_par[i] == 5)      /* 256 colours */
+					i++;                 /* ubiquitous */
+				else if (vc->vc_par[i] == 2) /* 24 bit colours */
+					i += 3;              /* extremely rare */
+				/* Subcommands 3 (CMY) and 4 (CMYK) are so insane
+				 * that detecting them is not worth the few extra
+				 * bytes of kernel's size.
+				 */
+				break;
 			case 39:
 				vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
 				break;
-- 
1.7.10.4

Re: [PATCH] vt: properly ignore xterm-256 colour codes

[Adam Borowski]

On Mon, Sep 09, 2013 at 05:53:19PM +0200, David Herrmann wrote:
> On Fri, Jul 12, 2013 at 10:23 PM, Adam Borowski <kilobyte@angband.pl> wrote:
> >  drivers/tty/vt/vt.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index c677829..f7aaa28 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -1300,13 +1300,27 @@ static void csi_m(struct vc_data *vc)
[...]
> > -                       case 38: /* ANSI X3.64-1979 (SCO-ish?)
> > -                                 * Enables underscore, white foreground
> > -                                 * with white underscore (Linux - use
> > -                                 * default foreground).
> > -                               vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
> > -                               vc->vc_underline = 1;
> 
> You break the old behavior here. _Iff_ this is what you want, then
> please do that in another commit. Explicitly state that "38" is used
> for 256color and shouldn't turn on underline+default-col. The SCO-ish
> behavior is weird, indeed, but breaking it silently is not ok.

This is implied by the description; none among modern terminal emulators
support this.  Would an additional comment in the commit message be
enough, or do I need to change the replacement into a pair of commits?
 
> > +                               i++;
> > +                               if (i > vc->vc_npar)
> 
> This should be ">=", but the for()-loop does allow your ">". So unless
> someone fixes the for-loop to use "<" (do a ++vc->vc_npar before it,
> if it's correct. But blindly doing "<=" is really irritating) I think
> this is ok.

The loop this switch is in does:
	for (i = 0; i <= vc->vc_npar; i++)
which is obviously contrary to what we're used to, but I did not want to
rewrite nearby code to match my preferences.

The change you suggest would deoptimize the code by a single unnecessary
dereference and increment, which is negligible, but since the whole cost
of speedier version is having <= instead of < in the loop, I'm not so
certain this is a good idea.

[...]
> Btw., you should put Greg Kroah-Hartman and Andrew Morton on CC. Both
> are the most likely to pick this up.

Thanks for the suggestion.  I've sent the patch two days ago to Jiri Slaby
(listed as a maintainer besides Greg) together with a newbie question, but
he's apparently busy.

I've got more changes for the vt, but there's no hurry, I wanted to test
the waters with a single minor one in 3.12 first.

-- 
ᛊᚨᚾᛁᛏᚣ᛫ᛁᛊ᛫ᚠᛟᚱ᛫ᚦᛖ᛫ᚹᛖᚨᚲ

Re: [PATCH] vt: properly ignore xterm-256 colour codes

[David Herrmann]

Hi Adam

On Mon, Sep 9, 2013 at 6:46 PM, Adam Borowski <kilobyte@angband.pl> wrote:
> On Mon, Sep 09, 2013 at 05:53:19PM +0200, David Herrmann wrote:
>> On Fri, Jul 12, 2013 at 10:23 PM, Adam Borowski <kilobyte@angband.pl> wrote:
>> > +                               i++;
>> > +                               if (i > vc->vc_npar)
>>
>> This should be ">=", but the for()-loop does allow your ">". So unless
>> someone fixes the for-loop to use "<" (do a ++vc->vc_npar before it,
>> if it's correct. But blindly doing "<=" is really irritating) I think
>> this is ok.
>
> The loop this switch is in does:
>         for (i = 0; i <= vc->vc_npar; i++)
> which is obviously contrary to what we're used to, but I did not want to
> rewrite nearby code to match my preferences.
>
> The change you suggest would deoptimize the code by a single unnecessary
> dereference and increment, which is negligible, but since the whole cost
> of speedier version is having <= instead of < in the loop, I'm not so
> certain this is a good idea.

This was just my comment on the weird for-loop. No need to touch it at
all, I just wondered whether anyone knows why we use <= instead of
just < here.

> [...]
>> Btw., you should put Greg Kroah-Hartman and Andrew Morton on CC. Both
>> are the most likely to pick this up.
>
> Thanks for the suggestion.  I've sent the patch two days ago to Jiri Slaby
> (listed as a maintainer besides Greg) together with a newbie question, but
> he's apparently busy.
>
> I've got more changes for the vt, but there's no hurry, I wanted to test
> the waters with a single minor one in 3.12 first.

Jiri Slaby maintains the TTY subsystem (together with Greg). This does
not include the VT layer, though. drivers/tty/vt/ and
drivers/video/console/ are unmaintained. You need to get the attention
of any maintainer who is willing to take it through their tree (hint:
most maintainers don't dare touching the VT layer. Greg and Andrew
were brave enough in the past.).

I'm willing to review your patches, but history taught me touching the
VT layer is a waste of time. Still, good luck.

Regards
David

Re: [PATCH] vt: properly ignore xterm-256 colour codes

[Adam Borowski]

On Thu, Sep 12, 2013 at 02:37:26PM +0200, David Herrmann wrote: 
> On Mon, Sep 9, 2013 at 6:46 PM, Adam Borowski <kilobyte@angband.pl> wrote:
> > On Mon, Sep 09, 2013 at 05:53:19PM +0200, David Herrmann wrote:
> > [...]
> >> Btw., you should put Greg Kroah-Hartman and Andrew Morton on CC. Both
> >> are the most likely to pick this up.
> >
> > Thanks for the suggestion.  I've sent the patch two days ago to Jiri Slaby
> > (listed as a maintainer besides Greg) together with a newbie question, but
> > he's apparently busy.
> 
> Jiri Slaby maintains the TTY subsystem (together with Greg). This does
> not include the VT layer, though. drivers/tty/vt/ and
> drivers/video/console/ are unmaintained. You need to get the attention
> of any maintainer who is willing to take it through their tree (hint:
> most maintainers don't dare touching the VT layer. Greg and Andrew
> were brave enough in the past.).
> 
> I'm willing to review your patches

Thanks.  So I shouldn't bother anyone else for now to get My First Kernel
Patch(tm) into 3.12 (or, if it's too late, into something included in 3.13),
right?

> > I've got more changes for the vt, but there's no hurry, I wanted to test
> > the waters with a single minor one in 3.12 first.

> drivers/tty/vt/ and drivers/video/console/ are unmaintained.

> [...] but history taught me touching the VT layer is a waste of time.

Could you tell me why?  The console is an important tool when something
fails.  Of course, fancy schmancy stuff like combining characters, etc,
could be better done in that legendary userspace alternate console layer,
but the built-in VT must remain at least functional.  And getting corrupted
text is not nice; the VT has fallen woefully behind what works on any other
modern terminal.

Back by ~2000, I'd say it worked better than rxvt, xterm, or, Cthulhu help
us, Solaris' terminal.  It just needs some maintenance.

I had a bunch of other improvements planned; if you say that's a waste of
time perhaps I should scale that back.  But I'd still want to at least
make sure programs that don't use terminfo (terminfo is a bad joke) won't
spew ANSI codes to the screen; at least more popular ones like "set window
title", etc.

You might know of other clean-up and fixes that need to be done here,
though.


-- 
ᛊᚨᚾᛁᛏᚣ᛫ᛁᛊ᛫ᚠᛟᚱ᛫ᚦᛖ᛫ᚹᛖᚨᚲ

Re: [PATCH] vt: properly ignore xterm-256 colour codes

[David Herrmann]

Hi

On Thu, Sep 12, 2013 at 3:24 PM, Adam Borowski <kilobyte@angband.pl> wrote:
> On Thu, Sep 12, 2013 at 02:37:26PM +0200, David Herrmann wrote:
>> On Mon, Sep 9, 2013 at 6:46 PM, Adam Borowski <kilobyte@angband.pl> wrote:
>> > On Mon, Sep 09, 2013 at 05:53:19PM +0200, David Herrmann wrote:
>> > [...]
>> >> Btw., you should put Greg Kroah-Hartman and Andrew Morton on CC. Both
>> >> are the most likely to pick this up.
>> >
>> > Thanks for the suggestion.  I've sent the patch two days ago to Jiri Slaby
>> > (listed as a maintainer besides Greg) together with a newbie question, but
>> > he's apparently busy.
>>
>> Jiri Slaby maintains the TTY subsystem (together with Greg). This does
>> not include the VT layer, though. drivers/tty/vt/ and
>> drivers/video/console/ are unmaintained. You need to get the attention
>> of any maintainer who is willing to take it through their tree (hint:
>> most maintainers don't dare touching the VT layer. Greg and Andrew
>> were brave enough in the past.).
>>
>> I'm willing to review your patches
>
> Thanks.  So I shouldn't bother anyone else for now to get My First Kernel
> Patch(tm) into 3.12 (or, if it's too late, into something included in 3.13),
> right?

Too late for 3.12. We want such stuff in linux-next before it is
merged. So yeah, 3.13 should be your aim.

>> > I've got more changes for the vt, but there's no hurry, I wanted to test
>> > the waters with a single minor one in 3.12 first.
>
>> drivers/tty/vt/ and drivers/video/console/ are unmaintained.
>
>> [...] but history taught me touching the VT layer is a waste of time.
>
> Could you tell me why?

Because few people care (did anyone but me respond to this?).
Furthermore, most people just want it to "not break", they often don't
care for improvements.

> The console is an important tool when something
> fails.

That's true for any recovery tool.

> Of course, fancy schmancy stuff like combining characters, etc,
> could be better done in that legendary userspace alternate console layer,
> but the built-in VT must remain at least functional. And getting corrupted
> text is not nice; the VT has fallen woefully behind what works on any other
> modern terminal.
>
> Back by ~2000, I'd say it worked better than rxvt, xterm, or, Cthulhu help
> us, Solaris' terminal.  It just needs some maintenance.

Yes, the linux-console used to be something people were proud of. But
there ought to be a reason why it has fallen behind. You might wanna
figure that out before spending time fixing the symptom.

> I had a bunch of other improvements planned; if you say that's a waste of
> time perhaps I should scale that back.  But I'd still want to at least
> make sure programs that don't use terminfo (terminfo is a bad joke) won't
> spew ANSI codes to the screen; at least more popular ones like "set window
> title", etc.
>
> You might know of other clean-up and fixes that need to be done here,
> though.

I'm not saying that I dislike the effort. Please go ahead. But you
might want to have a look at "git log drivers/tty/vt/vt.c". There
hasn't been any serious VT changes since 2010. Neither for
drivers/char/vt.c which it was back then. I think this effort is
better spent on user-space consoles, but I might be biased.

Regards
David

[PATCH 1/2] vt: break a couple of obsolete SCOish codes.

[Adam Borowski]

No modern terminal supports them, and SGR 38 conflicts with detecting
xterm-256 colours.  This also makes SGR 39 consistent with other popular
terminals.  Neither are used by ncurses' terminfo.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 drivers/tty/vt/vt.c |   15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index c677829..ba528bb 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1300,21 +1300,8 @@ static void csi_m(struct vc_data *vc)
 			case 27:
 				vc->vc_reverse = 0;
 				break;
-			case 38: /* ANSI X3.64-1979 (SCO-ish?)
-				  * Enables underscore, white foreground
-				  * with white underscore (Linux - use
-				  * default foreground).
-				  */
-				vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
-				vc->vc_underline = 1;
-				break;
-			case 39: /* ANSI X3.64-1979 (SCO-ish?)
-				  * Disable underline option.
-				  * Reset colour to default? It did this
-				  * before...
-				  */
+			case 39:
 				vc->vc_color = (vc->vc_def_color & 0x0f) | (vc->vc_color & 0xf0);
-				vc->vc_underline = 0;
 				break;
 			case 49:
 				vc->vc_color = (vc->vc_def_color & 0xf0) | (vc->vc_color & 0x0f);
-- 
1.7.10.4