Re: lynx-dev lynx and ncurses and gpm

[Klaus Weide]

On Sat, 17 Apr 1999, Klaus Weide wrote:

> I changed the initialization to
>
     [ changed ncurses lib_mouse.c code ] 
> 
> recompiled ncurses, and got a lynx with mouse support under gpm that 
> works.  At least brief testing didn't shown problems that were specific
> to the gpm environment, only those that are also there in an xterm.

That was spoken too early; there are various remaining problems.

(For the following, assume ncurses with patches to 990410 and 
previously mentioned change for gpm_connect; gpm 1.17.6;
ncurses built --with-gpm; lynx -use_mouse on linux console.)

Problem 1a
----------
     Suspending and resuming twice in a row, without any intervening
     keystrokes - that is:  ^Z, fg, ^Z, fg
     After the second fg, linux exits abruptly.
     Does not happen if any keys are pressed between first fg and
     second ^Z.

Reason: gpm messes around with interrupt handlers.  It installs a
SIGTSTP handler (which in turn invokes the existing, i.e. ncurses
handler), uses sigaction() for this and doesn't set a SA_RESTART flag.
As a result SIGTSTP [as well as probably SIGWINCH, there is a similar 
situation] is non-restarting, although other signals may be restarting.

The ncurses wgetch(), actually fifo_push() in lib_getch.c, doesn't deal
with this situation, an interrupted wgetch() returns ERR, lynx concludes
there is a severe error and exits.  This is consistent with the comments
at the beginning of ncurses' lib_tstp.c:

 * The ncurses code needs signal-call restart to happen -- otherwise,
 * interrupted wgetch() calls will return FAIL, probably making the
 * application think the input stream has ended and it should
 * terminate.

There is a provision to deal with an interrupted read() in fifo_push()
if HIDE_EINTR is defined.  But HIDE_EINTR is explicitly #undef'd in
fifo_defs.h, with no explanation except the 980606 NEWS file entry
(which I assume corresponds to the #undef)

        + removed the cycle on EINTR, as it seems to be useless.

I don't know how someone came to that conclusion, and disagree with
it (as does the lib_tstp.c comment above).

Possible solutions:

1. Change gpm code to set SA_RESTART flag.

   Should be done, but this solves the problem only for ncurses+gpm, it
   will pop up again for ncurses+<some other code out of ncurses' control>. 

2. Change ncurses code to prevent gpm code from installing its own SIGTSTP
   handler; (temporarily) setting SIG_IGN at Gpm_Open() time would achieve
   this.

   Again this is a workaround specific to gpm, some other code out of
   ncurses' control can create the same kind of problem.
   Also, the gpm SIGTSTP handler, besides creating a problem, does something
   useful; its function would have to be duplicated in ncurses code.

3. Change ncurses code (back) to deal with interrupted read() system call as
   required.

   Obviously this is the preferred solution.

4. Change all applications where this may occur to somehow detect the
   situation, and work around it.

   Applications shouldn't have to bother with this kind of low-level
   stuff, that's one reason why they use higher-level functions instead
   of doing read() etc. directly.  [Although they may still have to
   anticipate this situation in order to be portable to other curses
   implemetations.]

Problem 1b
----------
     Directly after suspending and resuming once, without a following
     keystroke - that is:  ^Z, fg
     Mouse actions don't work (have no effect, except the default mouse
     cursor movement)

This is also caused by the non-restarting SIGTSTP handler installed by
gpm.  What gets interrupted by the first ^Z is actually the _nc_timed_wait()
in fifo_push(), it returns prematurely instead of waiting until new
input is actually available.  This also happens to 'protect' the read()
further below in fifo_push(), it is the reason why Problem 1a only
becomes visible on the *second* interrupt.

Possible solutions: 1., 2. as above, or 

3. Change ncurses code to deal with interrupted system call in
   _nc_timed_wait() as required.

   Obviously this is the preferred solution.

Problem 2
---------
     Shelling out from curses mode does not restore the default mouse
     behavior.  This would be needed so that ncurses mouse support
     using gpm acts more line ncurses mouse support using xterm.

The places to do this are marked with comments in lib_mouse.c
(_nc_mouse_wrap(), _nc_mouse_resume()), but it needs to be implemented.

(Once this is done, the gpm SIGTSTP handler may not be needed any
more and could be disabled, see 2. above.)

Alternatively or in addition, mousemask(0, ..) should tell the gpm
server to not send any events any more.

==================================================================

The patch below solves Problems 1a and 1b.

HIDE_EINTR is only forced on when compiling for GPM support, for which
it's definitely needed, although it should probably always be done.

The change in _nc_timed_wait() is NOT conditional on HIDE_EINTR
for the case when indefinite blocking is requested, since in that
case returning without a fd event may really confuse the logic of
callers.

   Klaus


--- ncurses.old/curses.priv.h.~1~       Tue Mar 16 06:33:39 1999
+++ ncurses/curses.priv.h       Wed Apr 21 01:24:29 1999
@@ -108,6 +108,7 @@
 #define USE_GPM_SUPPORT 1
 #else
 #define USE_GPM_SUPPORT 0
+#define HIDE_EINTR
 #endif
 
 /* QNX mouse support */
--- ncurses.old/fifo_defs.h.~1~ Wed Feb 11 06:13:56 1998
+++ ncurses/fifo_defs.h Wed Apr 21 01:24:25 1999
@@ -54,6 +54,4 @@
 #define cooked_key_in_fifo()   (head!=-1 && peek!=head)
 #define raw_key_in_fifo()      (head!=-1 && peek!=tail)
 
-#undef HIDE_EINTR
-
 #endif /* FIFO_DEFS_H */
--- ncurses.old/tty/lib_twait.c.~1~     Sat Jun  6 17:44:14 1998
+++ ncurses/tty/lib_twait.c     Thu Apr 22 07:29:38 1999
@@ -100,6 +100,7 @@
 int    fd;
 int    count;
 
+int saved_errno;
 int result;
 
 #if USE_FUNC_POLL
@@ -112,9 +113,7 @@
 
        T(("start twait: %d milliseconds, mode: %d", milliseconds, mode));
 
-#if PRECISE_GETTIME
 retry:
-#endif
        starttime = _nc_gettime();
 
        count = 0;
@@ -160,10 +159,32 @@
        }
 #endif
 
+       saved_errno = errno;    /* paranoid, so it won't be clobbered */
        returntime = _nc_gettime();
 
+       /*
+        * If a non-restarting signal was caught and an indefinite wait
+        * was requested, always retry.  The caller can expect this
+        * function to return only when an event on one of the fds
+        * really occurred.
+        */
+       if (milliseconds == -1 && result == -1 && saved_errno == EINTR) {
+           goto retry;
+       }
+
        if (milliseconds >= 0)
                milliseconds -= returntime-starttime;
+
+#ifdef HIDE_EINTR
+       /*
+        * If a non-restarting signal was caught, retry if there is
+        * still time left.  This should make the difference between
+        * non-restarting and restarting signals invisible.
+        */
+       if (milliseconds > 0 && result == -1 && saved_errno == EINTR) {
+           goto retry;
+       }
+#endif
 
 #if PRECISE_GETTIME
        /*