Re: [PATCH] Fix 'flash' for Bash

[Thomas Dickey]

On Wed, Sep 11, 2024 at 06:53:05PM +0200, Harm te Hennepe wrote:
> At the moment the time-delay of 100ms for 'flash' (as specified in my
> terminfo entry) doesn't work correctly with Bash/Readline (`set
> bell-style visible`). It makes use of `tputs`.
> 
> A strace shows that the time-delay isn't happening at the right moment:
> ```
> strace: Process 3872344 attached
> read(0, "\10", 1)                       = 1
> clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100000000},
> 0x7ffca150b340) = 0
> write(2, "\33[?5h\33[?5l", 10)          = 10
> ```
> 
> In v6_2_20201128 a `fflush(stdout)` was added for applications using
> low-level calls [1]
> 
> Unfortunately, Bash uses stderr /and/ sets it to buffered. I suggest
> fflush'ing stderr as well. With the patch below applied, the flash
> works as expected again:
> ```
> strace: Process 3876152 attached
> read(0, "\10", 1)                       = 1
> write(2, "\33[?5h", 5)                  = 5
> clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100000000},
> 0x7ffdf8a19110) = 0
> write(2, "\33[?5l", 5)                  = 5
> ```
> 
> I guess a better solution would be to have a `tputs2`, which also
> accepts a callback for flushing the stream.
> But, as this has already been broken since at least 2006 (I started a
> VM with an Ubuntu 6.06 live cd), and probably never worked, I think
> the incentive for implementing that would be low.

agreed.  The answer to anything like that is to use terminfo.
 
> On a sidenote, as `SP_PARM->_ofd > 0`, shouldn't [2] be 
> `fflush(SP_PARM->_ofd)`?

SP_PARM->_ofd is an int, while fflush() needs a FILE*

SP_PARM->_ofp is related, but that's oversimplified.
That's set via newterm, which calls setupterm -- for non-termcap applications,
and in that route sets _ofd, while initializing I/O.

bash is using the termcap interface, which doesn't do anything useful for
I/O initialization, so ncurses has to assume (from post-5.9 changes) that
the termcap functions are using stdout.

        20120825
                + change output buffering scheme, using buffer maintained by 
ncurses
                  rather than stdio, to avoid problems with SIGTSTP handling 
(report
                  by Brian Bloniarz).

But that's only an assumption, since tputs passes a function pointer,
rather than a stream pointer:

       int tputs(const char *str, int affcnt, int (*putc)(int));

So... ncurses doesn't really know that bash has decided to pass everything
through a buffered stderr.  I see that's using _rl_output_character_function,
which in turn uses _rl_out_stream, etc., and _that_ has used stderr since
the current repository was created in 1996 from bash 1.14.7 -- perhaps
there was a changelog note explaining why that was done, but I don't see
it in the current sources (nor in the 1.14.7 sources). I suspect no one
can provide that information.

But flushing stderr is harmless, since it will have no effect on virtually
all other users.

I did this for stdout:

        20201128
                + add another fflush(stdout) in _nc_flush() to handle 
time-delays in
                  the middle of strings such as flash when the application uses
                  low-level calls rather than curses (cf: 20161217).

termcap applications won't set SP_PARM (I'm reminded of some really bizarre
code that pretends to be a termcap application, but we can ignore that at
the moment), so bash would be using the second fflush(stderr).  The first
might help with that other application :-)
 
> [1] 
> https://github.com/ThomasDickey/ncurses-snapshots/blob/21c551ae1d2aa970ceae868fffd80922f6263565/NEWS#L53
> [2] 
> https://github.com/ThomasDickey/ncurses-snapshots/blob/a480458efb0662531287f0c75116c0e91fe235cb/ncurses/tinfo/lib_tputs.c#L154
> 
> diff --git a/ncurses/tinfo/lib_tputs.c b/ncurses/tinfo/lib_tputs.c
> index f834532..10ccb3f 100644
> --- a/ncurses/tinfo/lib_tputs.c
> +++ b/ncurses/tinfo/lib_tputs.c
> @@ -150,12 +150,14 @@ NCURSES_SP_NAME(_nc_flush) (NCURSES_SP_DCL0)
>                 }
>             }
>         } else if (SP_PARM->out_buffer == 0) {
> -           TR(TRACE_CHARPUT, ("flushing stdout"));
> +           TR(TRACE_CHARPUT, ("flushing stdout/stderr"));
>             fflush(stdout);
> +           fflush(stderr);
>         }
>      } else {
> -       TR(TRACE_CHARPUT, ("flushing stdout"));
> +       TR(TRACE_CHARPUT, ("flushing stdout/stderr"));
>         fflush(stdout);
> +       fflush(stderr);
>      }
>      if (SP_PARM != 0)
>         SP_PARM->out_inuse = 0;
> 
> 

-- 
Thomas E. Dickey <dickey@invisible-island.net>
https://invisible-island.net

signature.asc
Description: PGP signature