Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Patrick Mahoney
This is an attempt to fix the problem described by Matthew
Dempsky:
http://erlang.org/pipermail/erlang-questions/2007-December/031962.html

The erl shell in Unix does not properly handle terminal
resize events which can result in a misplaced cursor and
other confusion.

This patch adds two functions to the file
erts/emulator/drivers/unix/ttsl_drv.c

winch() is registered as the handler for SIGWINCH which is
  received on terminal resize events.  winch() sets a global
  flag "cols_needs_update = TRUE".

update_cols() is run at the start of each function that uses
  the COL() or LINE() macros for calculating cursor positions,
  namely the del_chars(), write_buf(), and move_cursor()
  functions.  It checks the "cols_needs_update" flag, and sets
  the "cols" global var to the number returned by
  ttysl_get_widow_size().

  If the terminal is resized after update_cols() but before
  use of the COL() macro, then we'll have the same incorrect
  cursor problems during that function.

We don't run ttysl_get_window_size() from the SIGWINCH
handler because it uses ioctl() which is not listed as a
"safe" function for use in signal handlers in the signal(7)
man page.

That said, the solution would be more elegant if we did call
ttysl_get_window_size() from the signal handler, avoiding
the polling done by update_cols().  The ncurses source
includes a simple test/view.c program that does use ioctl()
in its SIGWINCH handler.  It states "This uses functions
that are 'unsafe', but it seems to work on SunOS and Linux."
Doing this in erl works for me on Linux, but I have no idea
on which platforms if may fail.

I've only tested this with R12B-1 on Linux 2.6.22.

--
Patrick Mahoney <pat>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ttsl_drv_sigwinch.diff
Type: text/x-diff
Size: 2628 bytes
Desc: ttsl_drv_sigwinch.diff
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20071231/d361084e/attachment.bin>

Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Matthew Dempsky-3
On 12/31/07, Patrick Mahoney <pat> wrote:
> I've only tested this with R12B-1 on Linux 2.6.22.

I verified that this patch fixes the problem I reported for both
Terminal.app and xterm on OS X.  If it's merged, I'll award the $100
bounty to Patrick.


Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Mikael Pettersson
In reply to this post by Patrick Mahoney
Patrick Mahoney writes:
 > This is an attempt to fix the problem described by Matthew
 > Dempsky:
 > http://erlang.org/pipermail/erlang-questions/2007-December/031962.html
 >
 > The erl shell in Unix does not properly handle terminal
 > resize events which can result in a misplaced cursor and
 > other confusion.
 >
 > This patch adds two functions to the file
 > erts/emulator/drivers/unix/ttsl_drv.c
 >
 > winch() is registered as the handler for SIGWINCH which is
 >   received on terminal resize events.  winch() sets a global
 >   flag "cols_needs_update = TRUE".
 >
 > update_cols() is run at the start of each function that uses
 >   the COL() or LINE() macros for calculating cursor positions,
 >   namely the del_chars(), write_buf(), and move_cursor()
 >   functions.  It checks the "cols_needs_update" flag, and sets
 >   the "cols" global var to the number returned by
 >   ttysl_get_widow_size().
 >
 >   If the terminal is resized after update_cols() but before
 >   use of the COL() macro, then we'll have the same incorrect
 >   cursor problems during that function.
 >
 > We don't run ttysl_get_window_size() from the SIGWINCH
 > handler because it uses ioctl() which is not listed as a
 > "safe" function for use in signal handlers in the signal(7)
 > man page.
 >
 > That said, the solution would be more elegant if we did call
 > ttysl_get_window_size() from the signal handler, avoiding
 > the polling done by update_cols().  The ncurses source
 > includes a simple test/view.c program that does use ioctl()
 > in its SIGWINCH handler.  It states "This uses functions
 > that are 'unsafe', but it seems to work on SunOS and Linux."
 > Doing this in erl works for me on Linux, but I have no idea
 > on which platforms if may fail.

Calling ioctl() from a signal handler should work on any
proper *NIX, including Linux/Solaris/*BSD/AIX. I would however
worry about doing this in POSIX emulation environments.
I guess cygwin is the prime current example but I think
there also used to be one for VMS (if anyone cares).

But there is another reason for not blindly updating cols
from a signal handler: many procedures reference cols
multiple times, and an asynchronous change to cols would
break things. For example:

 > @@ -598,6 +607,8 @@
 >  {
 >      int dc, dl;
 >  
 > +    update_cols();
 > +
 >      dc = COL(to) - COL(from);
 >      dl = LINE(to) - LINE(from);
 >      if (dl > 0)

A change to cols between the assignments to dc and dl
would make them inconsistent, and a change to cols between
COL(to) and COL(from) would make dc bogus.

To poll at the beginning of each cols-sensitive output
procedure is probably the best short-term solution.
Longer-term someone needs to decide what SIGWINCH during
tty output means and how to handle it. Maybe ^L + redraw
is the proper thing to do.


Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Tony Finch-2
On Wed, 2 Jan 2008, Mikael Pettersson wrote:
>
> Calling ioctl() from a signal handler should work on any
> proper *NIX, including Linux/Solaris/*BSD/AIX.

No, it isn't listed as async signal safe by POSIX. See the table near the
bottom of
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html

Tony.
--
f.a.n.finch  <dot>  http://dotat.at/
NORTH FITZROY SOLE: SOUTHEAST BECOMING CYCLONIC 5 TO 7, OCCASIONALLY GALE 8 IN
SOLE. VERY ROUGH OR HIGH. RAIN OR SHOWERS. MODERATE OR GOOD, OCCASIONALLY
POOR.


Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Florian Weimer-2
* Tony Finch:

> On Wed, 2 Jan 2008, Mikael Pettersson wrote:
>>
>> Calling ioctl() from a signal handler should work on any
>> proper *NIX, including Linux/Solaris/*BSD/AIX.
>
> No, it isn't listed as async signal safe by POSIX. See the table near the
> bottom of
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html

POSIX only describes the STREAMS ioctl, which is not implemented by
Linux (and probably not by the BSDs, either).


Reply | Threaded
Open this post in threaded view
|

Patch for erl shell (unix/ttsl_drv.c) SIGWINCH handling (R12B-1 31-Dec-2007)

Tony Finch-2
On Thu, 3 Jan 2008, Florian Weimer wrote:
>
> POSIX only describes the STREAMS ioctl, which is not implemented by
> Linux (and probably not by the BSDs, either).

The BSD and Solaris manuals list the syscalls that are async-signal-safe
and ioctl isn't one of them. Linux appears not to be sufficiently
carefully documented to say either way.

Tony.
--
f.a.n.finch  <dot>  http://dotat.at/
VIKING: SOUTHEAST 6 TO GALE 8, PERHAPS SEVERE GALE 9 LATER. ROUGH OR VERY
ROUGH, OCCASIONALLY HIGH IN SOUTHWEST LATER. WINTRY SHOWERS. GOOD.