NetBSD Problem Report #58282
From gson@gson.org Sat May 25 12:43:05 2024
Return-Path: <gson@gson.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id B4BB41A923C
for <gnats-bugs@gnats.NetBSD.org>; Sat, 25 May 2024 12:43:04 +0000 (UTC)
Message-Id: <20240525124255.4EA2C253ED6@guava.gson.org>
Date: Sat, 25 May 2024 15:42:55 +0300 (EEST)
From: gson@gson.org (Andreas Gustafsson)
Reply-To: gson@gson.org (Andreas Gustafsson)
To: gnats-bugs@NetBSD.org
Subject: Sysinst terminal output size increased because curses
X-Send-Pr-Version: 3.95
>Number: 58282
>Category: lib
>Synopsis: Sysinst terminal output size increased because curses
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: lib-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat May 25 12:45:00 +0000 2024
>Last-Modified: Thu Feb 13 22:40:00 +0000 2025
>Originator: Andreas Gustafsson
>Release: NetBSD-current, source date >= 2022.04.12.07.03.29
>Organization:
>Environment:
System: NetBSD
Architecture: i386
Machine: i386
>Description:
According to the logs from the TNF i386 testbed, the amount of data
output to the terminal when performing a serial console installation
using sysinst(1) more than doubled after these commits:
2022.04.12.07.03.04 blymn src/lib/libcurses/add_wchstr.c 1.12
2022.04.12.07.03.04 blymn src/lib/libcurses/addbytes.c 1.62
2022.04.12.07.03.04 blymn src/lib/libcurses/attributes.c 1.34
2022.04.12.07.03.04 blymn src/lib/libcurses/background.c 1.29
2022.04.12.07.03.04 blymn src/lib/libcurses/border.c 1.23
2022.04.12.07.03.04 blymn src/lib/libcurses/clrtobot.c 1.29
2022.04.12.07.03.04 blymn src/lib/libcurses/clrtoeol.c 1.34
2022.04.12.07.03.04 blymn src/lib/libcurses/color.c 1.46
2022.04.12.07.03.04 blymn src/lib/libcurses/copywin.c 1.21
2022.04.12.07.03.04 blymn src/lib/libcurses/curses_private.h 1.78
2022.04.12.07.03.04 blymn src/lib/libcurses/delch.c 1.28
2022.04.12.07.03.04 blymn src/lib/libcurses/erase.c 1.35
2022.04.12.07.03.04 blymn src/lib/libcurses/get_wstr.c 1.11
2022.04.12.07.03.04 blymn src/lib/libcurses/ins_wch.c 1.19
2022.04.12.07.03.04 blymn src/lib/libcurses/ins_wstr.c 1.23
2022.04.12.07.03.04 blymn src/lib/libcurses/insdelln.c 1.21
2022.04.12.07.03.04 blymn src/lib/libcurses/mvwin.c 1.24
2022.04.12.07.03.04 blymn src/lib/libcurses/newwin.c 1.66
2022.04.12.07.03.04 blymn src/lib/libcurses/refresh.c 1.119
2022.04.12.07.03.04 blymn src/lib/libcurses/shlib_version 1.47
2022.04.12.07.03.04 blymn src/lib/libcurses/slk.c 1.20
2022.04.12.07.03.04 blymn src/lib/libcurses/touchwin.c 1.34
2022.04.12.07.03.29 blymn src/distrib/sets/lists/base/shl.mi 1.934
Looks like the optimization performed by curses got less optimal.
Here are copies of the installation console logs from before and after
the above commits for comparison:
https://www.gson.org/netbsd/bugs/curses/2022.04.11.21.23.07-install.log
https://www.gson.org/netbsd/bugs/curses/2022.04.12.07.03.29-install.log
The relevant part starts after the line saying
"erase ^?, werase ^W, kill ^U, intr ^C".
>How-To-Repeat:
>Fix:
>Audit-Trail:
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@netbsd.org
Cc: Brett Lymn <blymn@NetBSD.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Sun, 26 May 2024 05:16:20 +0300
[cc'ing Brett]
makech() in refresh.c has a block of code with the comment:
Work out if we can use a clear to end of line. If we are using
color then we can only erase the line if the terminal can erase to
the background color.
it uses (for the HAVE_WIDE case, which is the defautl) the helper
function _cursesi_celleq() that also checks if "attr" is the same, but
the next test in the condition narrows the attr comparison to
attr_mask. That looks suspicious.
PS: While there...
static __LDATA blank; probably should not be static.
And for the love of goodness, please, can we keep the parens balanced
inside/outside ifdefs? :)
-uwe
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 28 May 2024 16:48:48 +0930
On Sun, May 26, 2024 at 02:20:02AM +0000, Valery Ushakov wrote:
> The following reply was made to PR lib/58282; it has been noted by GNATS.
>
> From: Valery Ushakov <uwe@stderr.spb.ru>
> To: gnats-bugs@netbsd.org
> Cc: Brett Lymn <blymn@NetBSD.org>
> Subject: Re: lib/58282: Sysinst terminal output size increased because curses
> Date: Sun, 26 May 2024 05:16:20 +0300
>
> [cc'ing Brett]
>
> makech() in refresh.c has a block of code with the comment:
>
> Work out if we can use a clear to end of line. If we are using
> color then we can only erase the line if the terminal can erase to
> the background color.
>
> it uses (for the HAVE_WIDE case, which is the defautl) the helper
> function _cursesi_celleq() that also checks if "attr" is the same, but
> the next test in the condition narrows the attr comparison to
> attr_mask. That looks suspicious.
>
Having a look at the output all the spam comes from the foreground and
background colour being set for each cell. Running from memory the
narrowing to attr in that section of code is deliberate to force the
update of the background colour because the actual screen will not have
changed but the virtual screen image will match the update screen image
so the background will not be set. That secion of code caused me
considerable pain during the rolotill - I am not saying it is 100%
correct but it does have the correct effect and there was a net decrease
in output in the atf test captures.
The old code output is more compact because it didn't actually do the
setting properly so colour based curses applications were not working
correctly. The whole rototill was kicked off by someone rasing about a
bug in a pkgsrc program that was not setting the colours correctly.
> PS: While there...
>
> static __LDATA blank; probably should not be static.
>
Yes, probably.
> And for the love of goodness, please, can we keep the parens balanced
> inside/outside ifdefs? :)
>
That would be most excellent - I am probably guilty of that one :(
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 28 May 2024 11:58:23 +0200
Can we have a quirk mode that enables rough optimization here?
The exact background colour in sysinst should not matter a lot
but the increased time for redrawing the screen is highly visible
on a serial console at 9600bps.
Martin
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 28 May 2024 12:01:57 +0200
On Tue, May 28, 2024 at 10:00:03AM +0000, Martin Husemann wrote:
> Can we have a quirk mode that enables rough optimization here?
Or maybe even a heuristic that skips all the exact colour code outputs
if fg/bg colour have been set but nothing else really changed individual
cell attributes?
Martin
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@netbsd.org
Cc: Brett Lymn <blymn@internode.on.net>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 28 May 2024 14:42:06 +0300
On Tue, May 28, 2024 at 16:48:48 +0930, Brett Lymn wrote:
> On Sun, May 26, 2024 at 02:20:02AM +0000, Valery Ushakov wrote:
>
> > makech() in refresh.c has a block of code with the comment:
> >
> > Work out if we can use a clear to end of line. If we are using
> > color then we can only erase the line if the terminal can erase to
> > the background color.
> >
> > it uses (for the HAVE_WIDE case, which is the defautl) the helper
> > function _cursesi_celleq() that also checks if "attr" is the same, but
> > the next test in the condition narrows the attr comparison to
> > attr_mask. That looks suspicious.
> >
>
> Having a look at the output all the spam comes from the foreground and
> background colour being set for each cell. Running from memory the
> narrowing to attr in that section of code is deliberate to force the
> update of the background colour because the actual screen will not have
> changed but the virtual screen image will match the update screen image
> so the background will not be set.
I don't understand offhand what that code tries to do, but here are
two observations:
1. the wide case and non-wide case have different semantic (modulo
wideness).
2. the narrowing is a no-op in the wide case b/c celleq already tests
if cp->attr == win->battr, and then the narrowed tests redundantly
checks if
(cp->attr & attr_mask) == (win->battr & attr_mask)
if the intention was (as non-wide case seems to suggest) to test
the cell's text and only parts of "attr", then using
_cursesi_celleq is wrong.
-uwe
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Wed, 29 May 2024 08:22:54 +0930
On Tue, May 28, 2024 at 10:00:03AM +0000, Martin Husemann wrote:
>
> Can we have a quirk mode that enables rough optimization here?
>
Use a terminfo that doesn't have colour capabilities?
> The exact background colour in sysinst should not matter a lot
> but the increased time for redrawing the screen is highly visible
> on a serial console at 9600bps.
>
Right, but curses does not know that you don't care about the background.
I understand and support your concerns but I am not sure what we can do about it.
I believe using colour comes with a significant overhead which is what we are seeing but I
would be happy to be proven wrong here.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: Andreas Gustafsson <gson@gson.org>
To: Brett Lymn <blymn@internode.on.net>
Cc: gnats-bugs@netbsd.org
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Wed, 29 May 2024 21:44:10 +0300
Brett,
When sysinst first starts up and clears the screen, it generates some
20 kilobytes of output that repeatedly sets the foreground color, sets
the background color, and displays a single space:
^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m ^[[37m^[[44m...
Why can't this be optimized to a single instance of ^[[37m^[[44m
followed multiple spaces?
--
Andreas Gustafsson, gson@gson.org
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Thu, 30 May 2024 07:45:14 +0930
On Wed, May 29, 2024 at 06:45:02PM +0000, Andreas Gustafsson wrote:
>
> Why can't this be optimized to a single instance of ^[[37m^[[44m
> followed multiple spaces?
That sounds like a reasonable suggestion. I will have a look at making
that work.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because
curses
Date: Wed, 29 May 2024 22:39:49 +0000 (UTC)
On Wed, 29 May 2024, Andreas Gustafsson wrote:
> Why can't this be optimized to a single instance of ^[[37m^[[44m
> followed multiple spaces?
>
If the terminal supports `bce', I think `ed' can be used:
printf '\e[33m\e[44m%s' "$(tput ed)"
-RVP
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Fri, 31 May 2024 07:50:38 +0930
On Wed, May 29, 2024 at 10:40:02PM +0000, RVP wrote:
>
> If the terminal supports `bce', I think `ed' can be used:
>
> printf '\e[33m\e[44m%s' "$(tput ed)"
>
Yes I believe the code does that but it depends on the terminal
capabilities.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 9 Jul 2024 16:47:11 +0930
--qJffieIJ/2fyx+UO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, May 29, 2024 at 06:45:02PM +0000, Andreas Gustafsson wrote:
>
> Why can't this be optimized to a single instance of ^[[37m^[[44m
> followed multiple spaces?
>
Please try the attached patch, see if this helps.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
--qJffieIJ/2fyx+UO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="pr-58282.diff"
Index: color.c
===================================================================
RCS file: /cvsroot/src/lib/libcurses/color.c,v
retrieving revision 1.47
diff -u -r1.47 color.c
--- color.c 19 Oct 2022 06:09:27 -0000 1.47
+++ color.c 9 Jul 2024 07:14:43 -0000
@@ -123,6 +123,7 @@
_cursesi_screen->COLORS = COLORS;
_cursesi_screen->COLOR_PAIRS = COLOR_PAIRS;
+ _cursesi_screen->curpair = -1;
/* Reset terminal colour and colour pairs. */
if (orig_colors != NULL)
@@ -540,6 +541,10 @@
return;
pair = PAIR_NUMBER((uint32_t)attr);
+
+ if (pair == _cursesi_screen->curpair)
+ return;
+
__CTRACE(__CTRACE_COLOR, "__set_color: %d, %d, %d\n", pair,
_cursesi_screen->colour_pairs[pair].fore,
_cursesi_screen->colour_pairs[pair].back);
@@ -578,6 +583,8 @@
0, __cputchar);
break;
}
+
+ _cursesi_screen->curpair = pair;
curscr->wattr &= ~__COLOR;
curscr->wattr |= attr & __COLOR;
}
@@ -611,6 +618,8 @@
}
break;
}
+
+ _cursesi_screen->curpair = -1;
}
/*
@@ -620,6 +629,12 @@
void
__restore_colors(void)
{
+ /*
+ * forget foreground/background colour just in case it was
+ * changed. We will reset them if required.
+ */
+ _cursesi_screen->curpair = -1;
+
if (can_change != 0)
switch (_cursesi_screen->color_type) {
case COLOR_HP:
Index: curses_private.h
===================================================================
RCS file: /cvsroot/src/lib/libcurses/curses_private.h,v
retrieving revision 1.81
diff -u -r1.81 curses_private.h
--- curses_private.h 17 May 2024 23:32:50 -0000 1.81
+++ curses_private.h 9 Jul 2024 07:14:43 -0000
@@ -226,6 +226,7 @@
#define TABSIZE_DEFAULT 8 /* spaces. */
int COLORS; /* Maximum colors on the screen */
int COLOR_PAIRS; /* Maximum color pairs on the screen */
+ short curpair; /* current colour pair set on the terminal */
int My_term; /* Use Def_term regardless. */
char GT; /* Gtty indicates tabs. */
char NONL; /* Term can't hack LF doing a CR. */
Index: screen.c
===================================================================
RCS file: /cvsroot/src/lib/libcurses/screen.c,v
retrieving revision 1.39
diff -u -r1.39 screen.c
--- screen.c 27 May 2024 14:30:43 -0000 1.39
+++ screen.c 9 Jul 2024 07:14:43 -0000
@@ -156,6 +156,7 @@
new_screen->nca = A_NORMAL;
new_screen->color_type = COLOR_NONE;
new_screen->COLOR_PAIRS = 0;
+ new_screen->curpair = -1;
new_screen->old_mode = 1;
new_screen->stdbuf = NULL;
new_screen->stdscr = NULL;
--qJffieIJ/2fyx+UO--
From: Andreas Gustafsson <gson@gson.org>
To: Brett Lymn <blymn@internode.on.net>
Cc: gnats-bugs@netbsd.org
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Tue, 9 Jul 2024 13:06:41 +0300
Brett Lymn wrote:
> > Why can't this be optimized to a single instance of ^[[37m^[[44m
> > followed multiple spaces?
>
> Please try the attached patch, see if this helps.
It helps with the specific issue where the color is being set before
every space character during the intial clearing of the screen when
sysinst is first started. This part is now much faster, which should
help give new users on serial consoles a much better first impression.
Unfortunately, that's not the only issue that appeared with the curses
changes of April 2022. They also changed the way the screen is
cleared between the subsequent pages of the sysinst UI (after the
initial startup) from clearing a line at a time using ^[[K to clearing
a character at a time using spaces (but thankfully it was not setting
the color before each space like it did with the initial screen). It
looks like this latter change actually accounted for the bulk of the
output size growth, but it's not as noticeable to the user because the
added delay does not happen all at once at the very beginning, but is
divided among many screens.
Here are the two links from the original PR again, now with the
corresponding file sizes:
https://www.gson.org/netbsd/bugs/curses/2022.04.11.21.23.07-install.log 74365
https://www.gson.org/netbsd/bugs/curses/2022.04.12.07.03.29-install.log 164936
And here is the output from a current build with and without your patch,
with sizes:
https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install.log 250499
https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install-patched.log 226238
--
Andreas Gustafsson, gson@gson.org
From: "Brett Lymn" <blymn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/58282 CVS commit: src/lib/libcurses
Date: Thu, 11 Jul 2024 07:13:41 +0000
Module Name: src
Committed By: blymn
Date: Thu Jul 11 07:13:41 UTC 2024
Modified Files:
src/lib/libcurses: color.c curses_private.h screen.c
Log Message:
PR lib/58282
This is a partial fix for the issues raised. This change will
reduce the output by preventing the foreground and background
colours being set on each cell. The current colour pair applied
is tracked and requests to set the colour to the same pair is now
a no-op.
To generate a diff of this commit:
cvs rdiff -u -r1.47 -r1.48 src/lib/libcurses/color.c
cvs rdiff -u -r1.81 -r1.82 src/lib/libcurses/curses_private.h
cvs rdiff -u -r1.39 -r1.40 src/lib/libcurses/screen.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Fri, 12 Jul 2024 07:51:00 +0930
On Tue, Jul 09, 2024 at 10:10:01AM +0000, Andreas Gustafsson wrote:
>
> And here is the output from a current build with and without your patch,
> with sizes:
>
> https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install.log 250499
> https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install-patched.log 226238
>
OK, so the repeated setting of colour is gone which is good but I am
puzzled about the erase line being missing. Oddly, the curses testframe
does use el when setting up the screen.
What is the value of the TERM variable? I am assuming wsvt25 but I want
to check because this sounds like a capability difference.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: Andreas Gustafsson <gson@gson.org>
To: Brett Lymn <blymn@internode.on.net>
Cc: gnats-bugs@netbsd.org
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Fri, 12 Jul 2024 11:21:05 +0300
Brett Lymn wrote:
> OK, so the repeated setting of colour is gone which is good but I am
> puzzled about the erase line being missing. Oddly, the curses testframe
> does use el when setting up the screen.
>
> What is the value of the TERM variable? I am assuming wsvt25 but I want
> to check because this sounds like a capability difference.
The problem of the missing "erase line" affects both TERM=wsvt25
(which is used on the i386 testbed) and TERM=xterm (which is used on
the amd64 testbed). OTOH, with TERM=vt100, sysinst does use
"erase line".
Here's a simple way to reproduce the issue on an existing NetBSD
system without actually performing a new install:
# script log
# TERM=wsvt25 sysinst
(choose a language and then exit sysinst without installing)
# exit
# od -c log | more
(look for long stretches of spaces in the output)
--
Andreas Gustafsson, gson@gson.org
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Fri, 19 Jul 2024 16:47:35 +0930
--oTccp8BTmLkEdG++
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Fri, Jul 12, 2024 at 08:25:01AM +0000, Andreas Gustafsson wrote:
>
> Here's a simple way to reproduce the issue on an existing NetBSD
> system without actually performing a new install:
>
> # script log
> # TERM=wsvt25 sysinst
> (choose a language and then exit sysinst without installing)
> # exit
> # od -c log | more
> (look for long stretches of spaces in the output)
>
This seems to be something to do with how sysinst is initialising the
screen that is triggering this behaviour. Doing a simple start_color
results in the correct erase line being emitted for all the terminal
types I tried. The attached sample C demonstrates the incorrect
behaviour.
I am still investigating.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
--oTccp8BTmLkEdG++
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="pr-58282.c"
#include <curses.h>
int main() {
WINDOW *mainwin;
initscr();
start_color();
mainwin = newwin(getmaxy(stdscr) - 2, getmaxx(stdscr) - 2, 1, 1);
wbkgd(stdscr, COLOR_PAIR(1));
wbkgd(mainwin, COLOR_PAIR(1));
wattrset(stdscr, COLOR_PAIR(1));
wattrset(mainwin, COLOR_PAIR(1));
touchwin(stdscr);
touchwin(mainwin);
wrefresh(stdscr);
wrefresh(mainwin);
endwin();
}
--oTccp8BTmLkEdG++--
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Wed, 12 Feb 2025 14:45:26 +1030
--R55TPpUKwrNW2A+w
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Fri, Jul 12, 2024 at 08:25:01AM +0000, Andreas Gustafsson wrote:
>
> The problem of the missing "erase line" affects both TERM=wsvt25
> (which is used on the i386 testbed) and TERM=xterm (which is used on
> the amd64 testbed). OTOH, with TERM=vt100, sysinst does use
> "erase line".
>
Hi Andreas. Can you please try the attached patch? This should fix the
excessive output from sysinst. Please note that the curses automated
tests will fail with this version of the library due to output
differences. I need to review the tests and ensure the output is
correct in all cases but this update to refresh.c should go a long way
to fixing the excessive output.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
--R55TPpUKwrNW2A+w
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="refresh.patch"
Index: refresh.c
===================================================================
RCS file: /cvsroot/src/lib/libcurses/refresh.c,v
retrieving revision 1.129
diff -u -r1.129 refresh.c
--- refresh.c 23 Dec 2024 02:58:04 -0000 1.129
+++ refresh.c 12 Feb 2025 04:07:09 -0000
@@ -226,6 +226,10 @@
screen->__virtscr->flags &= ~__LEAVEOK;
screen->__virtscr->flags |= dwin->flags;
+ /* copy the background char and attributes from win to __virtscr */
+ screen->__virtscr->bch = win->bch;
+ screen->__virtscr->battr = win->battr;
+
if ((dwin->flags & __ISDERWIN) != 0)
endy = begy + maxy;
else
@@ -1143,18 +1147,16 @@
{
WINDOW *win;
static __LDATA blank;
- __LDATA *nsp, *csp, *cp, *cep, *fsp;
+ __LDATA *nsp, *csp, *cp, *fsp;
__LINE *wlp;
int nlsp; /* offset to first space at eol. */
- size_t mlsp;
int lch, wx, owx, chw;
const char *ce;
- attr_t lspc; /* Last space colour */
attr_t battr; /* background attribute bits */
attr_t attr_mask; /* attributes mask */
#ifdef __GNUC__
- nlsp = lspc = 0; /* XXX gcc -Wuninitialized */
+ nlsp = 0; /* XXX gcc -Wuninitialized */
#endif
if (_cursesi_screen->curwin)
win = curscr;
@@ -1200,6 +1202,8 @@
/* Is the cursor still on the end of the last line? */
if (wy > 0 && curscr->alines[wy - 1]->flags & __ISPASTEOL) {
+ /* XXX this looks wrong - bad things will happen if ly
+ is at maxy */
domvcur(win, _cursesi_screen->ly, _cursesi_screen->lx,
_cursesi_screen->ly + 1, 0);
_cursesi_screen->ly++;
@@ -1219,16 +1223,6 @@
if (lch >= (int) win->maxx)
lch = win->maxx - 1;
- if (_cursesi_screen->curwin) {
- csp = ␣
- __CTRACE(__CTRACE_REFRESH, "makech: csp is blank\n");
- } else {
- csp = &curscr->alines[wy]->line[wx];
- __CTRACE(__CTRACE_REFRESH,
- "makech: csp is on curscr:(%d,%d)\n", wy, wx);
- }
-
-
while (win->alines[wy]->line[wx].cflags & CA_CONTINUATION) {
wx--;
if (wx <= 0) {
@@ -1237,6 +1231,15 @@
}
}
+ if (_cursesi_screen->curwin) {
+ csp = ␣
+ __CTRACE(__CTRACE_REFRESH, "makech: csp is blank\n");
+ } else {
+ csp = &curscr->alines[wy]->line[wx];
+ __CTRACE(__CTRACE_REFRESH,
+ "makech: csp is on virtscr:(%d,%d)\n", wy, wx);
+ }
+
nsp = fsp = &win->alines[wy]->line[wx];
#ifdef DEBUG
@@ -1255,25 +1258,34 @@
*/
if (clr_eol && !_cursesi_screen->curwin && (!(__using_color)
|| (__using_color && back_color_erase))) {
+
+ if (__using_color && back_color_erase) {
+ assume_default_colors(
+ _cursesi_screen->colour_pairs[PAIR_NUMBER(win->battr)].fore,
+ _cursesi_screen->colour_pairs[PAIR_NUMBER(win->battr)].back);
+ }
+
nlsp = win->maxx - 1;
cp = &win->alines[wy]->line[win->maxx - 1];
#ifdef HAVE_WCHAR
- while ((_cursesi_celleq(cp, &blank) == 1) &&
-#else
- while (cp->ch == blank.ch &&
-#endif /* HAVE_WCHAR */
- ((cp->attr & attr_mask) == battr)) {
-#ifdef HAVE_WCHAR
+ while (((_cursesi_celleq(cp, &blank) == 1) &&
+ ((cp->attr & attr_mask) == battr)) || ((cp->cflags & CA_BACKGROUND) == CA_BACKGROUND)) {
nlsp -= cp->wcols;
cp -= cp->wcols;
+
+ if (nlsp <= 0)
+ break;
+ }
#else
+ while (cp->ch == blank.ch &&
+ ((cp->attr & attr_mask) == battr)) {
nlsp--;
cp--;
-#endif /* HAVE_WCHAR */
if (nlsp <= 0)
break;
}
+#endif /* HAVE_WCHAR */
if (nlsp < 0)
@@ -1294,11 +1306,12 @@
csp->ch, csp->attr, csp->wcols, win->bch, win->battr,
win->wcols, csp->nsp);
#endif
- if (!(wlp->flags & __ISFORCED) &&
-#ifdef HAVE_WCHAR
- ((nsp->cflags & CA_CONTINUATION) != CA_CONTINUATION) &&
-#endif
- _cursesi_celleq(nsp, csp))
+
+ /*
+ * If the update is not being forced then skip over
+ * all the unchanged characters.
+ */
+ if (!(wlp->flags & __ISFORCED) && _cursesi_celleq(nsp, csp))
{
if (wx <= lch) {
while (wx <= lch && _cursesi_celleq(nsp, csp)) {
@@ -1322,35 +1335,31 @@
domvcur(win, _cursesi_screen->ly, _cursesi_screen->lx, wy, wx);
__CTRACE(__CTRACE_REFRESH, "makech: 1: wx = %d, ly= %d, "
- "lx = %d, newy = %d, newx = %d, lch = %d\n",
- wx, _cursesi_screen->ly, _cursesi_screen->lx, wy, wx, lch);
+ "lx = %d, newy = %d, newx = %d, lch = %d, nlsp = %d\n",
+ wx, _cursesi_screen->ly, _cursesi_screen->lx, wy, wx, lch,
+ nlsp);
+
_cursesi_screen->ly = wy;
_cursesi_screen->lx = wx;
owx = wx;
- while (wx <= lch &&
- ((wlp->flags & __ISFORCED) || !_cursesi_celleq(nsp, csp)))
+
+ if (wx <= lch &&
+ ((wlp->flags & __ISFORCED) || !_cursesi_celleq(nsp, csp)))
{
- if ((ce != NULL) && (wx >= nlsp) &&
- (nsp->ch == blank.ch) &&
- (__do_color_init == 1 || nsp->attr == blank.attr))
+ /*
+ * Consider clearing the line, if:
+ * - we have a clear to eol capability
+ * - current x pos is past last non blank char
+ * - the win char is blank
+ * - either we are initing colour or the attributes
+ * match.
+ * - Or the character is marked background
+ */
+ if ((clr_eol != NULL) && (wx >= nlsp) &&
+ (((nsp->cflags & CA_BACKGROUND) == CA_BACKGROUND) ||
+ ((nsp->ch == blank.ch) &&
+ (__do_color_init == 1 || nsp->attr == blank.attr))))
{
- /* Check for clear to end-of-line. */
- cep = &win->alines[wy]->line[win->maxx - 1];
- while (cep->ch == blank.ch && cep->attr == battr)
- if (cep-- <= csp)
- break;
-
- mlsp = &win->alines[wy]->line[win->maxx - 1]
- - win->alines[wy]->line
- - win->begx * __LDATASIZE;
-
- __CTRACE(__CTRACE_REFRESH,
- "makech: nlsp = %d, max = %zu, strlen(ce) = %zu\n",
- nlsp, mlsp, strlen(ce));
- __CTRACE(__CTRACE_REFRESH,
- "makech: line = %p, cep = %p, begx = %u\n",
- win->alines[wy]->line, cep, win->begx);
-
/*
* work out how to clear the line. If:
* - clear len is greater than clear_to_eol len
@@ -1363,17 +1372,15 @@
* then emit the ce string.
*/
if (((wy == win->maxy - 1) ||
- ((mlsp - wx) > strlen(ce))) &&
+ (((win->maxx - 1) - wx) > strlen(ce))) &&
((__using_color && back_color_erase) ||
(! __using_color))) {
if (wlp->line[wx].attr & win->screen->nca) {
__unsetattr(0);
} else if (__using_color &&
- ((__do_color_init == 1) ||
- ((lspc & __COLOR) !=
- (curscr->wattr & __COLOR)))) {
- __set_color(curscr, lspc &
- __COLOR);
+ (__do_color_init == 1)) {
+ __set_color(curscr,
+ curscr->wattr & __COLOR);
}
tputs(ce, 0, __cputchar);
_cursesi_screen->lx = wx + win->begx;
@@ -1393,6 +1400,7 @@
#endif /* HAVE_WCHAR */
assert(csp != &blank);
}
+
return OK;
}
}
@@ -2073,9 +2081,6 @@
tputs(exit_alt_charset_mode, 0, __cputchar);
curscr->wattr &= ~__ALTCHARSET;
}
- /* Don't leave the screen with colour set (check against ms). */
- if (__using_color && isms)
- __unset_color(curscr);
}
/* compare two line segments */
--R55TPpUKwrNW2A+w--
From: Andreas Gustafsson <gson@gson.org>
To: Brett Lymn <blymn@internode.on.net>
Cc: gnats-bugs@netbsd.org,
lib-bug-people@netbsd.org
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Thu, 13 Feb 2025 18:03:52 +0200
Brett Lymn wrote:
> Hi Andreas. Can you please try the attached patch? This should fix the
> excessive output from sysinst.
Thanks, this does improve things a great deal overall.
One remaining oddity is that when sysinst starts and clears the
screen, it appears to do it three times, first sending 24 copies of
^[[K^M, then 24 copies of ^[[C^[[K^M, and finally 24 copies of ^[[K^M
again. But maybe that's just sysinst calling curses in strange ways
rather than a problem with curses itself.
Also, the patch is making the drawing of the boxes around the sysinst
menus slower than it was before. Without the patch, the horizontal
lines above and below the menu are drawn by switching to the line
drawing character set once, then outputting multiple line drawing
characters, and finally switching back to ASCII. With the patch, it
switches back and forth between the line drawing character set and
ASCII for every line drawing character printed.
Here is an updated set of links to the terminal output from different
versions. The columns on the right are the total file size (including
things like qemu messages and kernel boot messages) and the remaining
size after removing the parts prior to sysinst starting.
https://www.gson.org/netbsd/bugs/curses/2022.04.11.21.23.07-install.log 74365 52813
https://www.gson.org/netbsd/bugs/curses/2022.04.12.07.03.29-install.log 164936 143386
https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install.log 250499 157326
https://www.gson.org/netbsd/bugs/curses/2024.07.06.11.09.17-install-patched.log 226238 137345
https://www.gson.org/netbsd/bugs/curses/2025.02.12.05.15.39-install.log 230676 137377
https://www.gson.org/netbsd/bugs/curses/2025.02.12.05.15.39-install-patched.log 144771 55694
--
Andreas Gustafsson, gson@gson.org
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
Andreas Gustafsson <gson@gson.org>
Subject: Re: lib/58282: Sysinst terminal output size increased because curses
Date: Fri, 14 Feb 2025 07:39:26 +1030
On Thu, Feb 13, 2025 at 04:05:02PM +0000, Andreas Gustafsson via gnats wrote:
>
> One remaining oddity is that when sysinst starts and clears the
> screen, it appears to do it three times, first sending 24 copies of
> ^[[K^M, then 24 copies of ^[[C^[[K^M, and finally 24 copies of ^[[K^M
> again. But maybe that's just sysinst calling curses in strange ways
> rather than a problem with curses itself.
>
I will check this but I think the first two are due to the bkgd(stdscr) and then a
wbkgd(main). IIRC the bkgd family do an implicit refresh call. Perhaps they should
not.
> Also, the patch is making the drawing of the boxes around the sysinst
> menus slower than it was before. Without the patch, the horizontal
> lines above and below the menu are drawn by switching to the line
> drawing character set once, then outputting multiple line drawing
> characters, and finally switching back to ASCII. With the patch, it
> switches back and forth between the line drawing character set and
> ASCII for every line drawing character printed.
>
Yes, atf picked that up. Somehow the acs is being turned on and off per character
leading to that noise. I will fix this.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2025
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home