Welcome,
I've observed then very often a X11 terminal app is not getting proper
window sizes afer terminal resize operation. This could be seen with
mc, jed, vim or other curses and not curses aware apps.
I wrote a simple program which just does nothing but uses SIGWINCH
handler so I can observe values reported by ioctl(1,TIOCGWINS,&ws) call
inside my signal handler. What is interesting that from time to time it
obtain unchanged values. It means values which were valid just before
terminal resize.
In drivers/char/vt.c and drivers/char/tty_io.c variables
vc->vc_tty->winsize and tty->winsize , real_tty->winsize are updated
after kill_pgrp(pgrp, SIGWINCH, 1) calls. I am not very familiar with
mutex design and how it corresponds to kill_pgrp() kernel function but
it seems that locking is not working here as we expect. An app can read
tty winsize data through ioctl() call in SIGWINCH handler and obtain
uchanged values.
So as a quick solution I made patches which move mentioned updates
before kill_pgrp() calls. As I tested modified kernel there is no
observed effect now. So I send patchs.
There are some places where kill_pgrp() call is used and some variable
is changed after it. It should be considered if this code is always
working properly or some race scheduler condition exists.
Signed-off-by: Adam Tla/lka <[email protected]>
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
> So as a quick solution I made patches which move mentioned updates
> before kill_pgrp() calls. As I tested modified kernel there is no
> observed effect now. So I send patchs.
NAK - this might happen to make the race miss on your box but it's not a
fix of any kind.
> There are some places where kill_pgrp() call is used and some variable
> is changed after it. It should be considered if this code is always
> working properly or some race scheduler condition exists.
The code was never race free, the scheduler change made the problem show
up more. Later 2.6.27-rc has patches that use the termios lock across
TIOCG/SWINSZ and deal with the problem properly.
Alan
Hi,
Mon, 6 Oct 2008 14:13:06 +0100 - Alan Cox <[email protected]>:
> > So as a quick solution I made patches which move mentioned updates
> > before kill_pgrp() calls. As I tested modified kernel there is no
> > observed effect now. So I send patchs.
>
> NAK - this might happen to make the race miss on your box but it's
> not a fix of any kind.
It depends. If mutexes are not working properly only in case of signal
sending then moving variables modification before kill_pgrp() call
could be a quite good enough solution too. Mutexes seems to be faster
and more efficient then semaphores for example.
> The code was never race free, the scheduler change made the problem
> show up more. Later 2.6.27-rc has patches that use the termios lock
> across TIOCG/SWINSZ and deal with the problem properly.
Maybe but what with older versions. Anyway the problem is if mutexes
are usable here or not.
Regards
--
Adam Tlałka
> > show up more. Later 2.6.27-rc has patches that use the termios lock
> > across TIOCG/SWINSZ and deal with the problem properly.
>
> Maybe but what with older versions. Anyway the problem is if mutexes
> are usable here or not.
Well if mutexes don't work your kernel will eat your computer fairly
rapidly so I think we may safely conclude that mutex locks work
Mon, 6 Oct 2008 23:14:34 +0100 - Alan Cox <[email protected]>:
> > > show up more. Later 2.6.27-rc has patches that use the termios
> > > lock across TIOCG/SWINSZ and deal with the problem properly.
> >
> > Maybe but what with older versions. Anyway the problem is if mutexes
> > are usable here or not.
>
> Well if mutexes don't work your kernel will eat your computer fairly
> rapidly so I think we may safely conclude that mutex locks work
>
OK, so if this race problem raises only while kill_pgrp() is
used the proposed patch eliminates this problem. Of course we should
change code in all places where pgrp_kill() is used in conjuction
with mutexes and some internal variables are modified.
What do you think about it?
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
Welcome,
now we have 2.6.26.6 kernel and still terminal resize leads to
undesired effects. It is very inconvenient to wait for 2.6.27 for
corrections.
As Alan Cox previously said mutexes generally work but as we can
observe in case of kill_pgrp() call inside mutex lock we got
race because of rescheduling so lock is not working here.
Rearanging code so the variable change is placed before kill_pgrp()
call removes mentioned race situaction.
Signed-off-by: Adam Tla/lka <[email protected]>
I strongly suggest to patch actual 2.6.26.x kernel to remove this very
nasty pts behaviour.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
Welcome,
Fri, 10 Oct 2008 03:12:34 +0200 - Adam Tlałka <[email protected]>:
> now we have 2.6.26.6 kernel and still terminal resize leads to
> undesired effects. It is very inconvenient to wait for 2.6.27 for
> corrections.
Very funny, I've posted my patch just before 2.6.27 appeared so now it
seems obsolete. Only argument for it now is the cleaner and more
optimized code. Why?
Now we have two distant places where we use ws and tty->winsize
variables:
(from drivers/char/tty_io.c:tty_do_resize())
mutex_lock(&real_tty->termios_mutex);
=> if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
goto done;
/* Get the PID values and reference them so we can
avoid holding the tty ctrl lock while sending signals */
spin_lock_irqsave(&tty->ctrl_lock, flags);
pgrp = get_pid(tty->pgrp);
rpgrp = get_pid(real_tty->pgrp);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
if (pgrp)
kill_pgrp(pgrp, SIGWINCH, 1);
if (rpgrp != pgrp && rpgrp)
kill_pgrp(rpgrp, SIGWINCH, 1);
put_pid(pgrp);
put_pid(rpgrp);
=> tty->winsize = *ws;
=> real_tty->winsize = *ws;
done:
mutex_unlock(&real_tty->termios_mutex);
Rearanged code
mutex_lock(&real_tty->termios_mutex);
=> if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
goto done;
=> tty->winsize = *ws;
=> real_tty->winsize = *ws;
/* Get the PID values and reference them so we can
avoid holding the tty ctrl lock while sending signals */
spin_lock_irqsave(&tty->ctrl_lock, flags);
pgrp = get_pid(tty->pgrp);
rpgrp = get_pid(real_tty->pgrp);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
if (pgrp)
kill_pgrp(pgrp, SIGWINCH, 1);
if (rpgrp != pgrp && rpgrp)
kill_pgrp(rpgrp, SIGWINCH, 1);
put_pid(pgrp);
put_pid(rpgrp);
done:
mutex_unlock(&real_tty->termios_mutex);
is more logical and grouping acces to the same variable in one place
mean that gcc can better optimize outputed machine code. So it should
be a bit faster.
If some app gets SIGWINCH and will be waiting on this mutex so in case
of rearanged code it will be waiting shorter because variables setting
is done before signal generation.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
On Fri, 10 Oct 2008 03:12:34 +0200
Adam Tlałka <[email protected]> wrote:
> Welcome,
>
> now we have 2.6.26.6 kernel and still terminal resize leads to
> undesired effects. It is very inconvenient to wait for 2.6.27 for
> corrections.
>
> As Alan Cox previously said mutexes generally work but as we can
> observe in case of kill_pgrp() call inside mutex lock we got
> race because of rescheduling so lock is not working here.
> Rearanging code so the variable change is placed before kill_pgrp()
> call removes mentioned race situaction.
>
> Signed-off-by: Adam Tla/lka <[email protected]>
>
> I strongly suggest to patch actual 2.6.26.x kernel to remove this very
> nasty pts behaviour.
NAK again
Moving the copies around simply moves the race, it might be that it fixes
your box and unfixes other peoples.
Fri, 10 Oct 2008 10:29:06 +0100 - Alan Cox <[email protected]>:
> On Fri, 10 Oct 2008 03:12:34 +0200
> Adam Tlałka <[email protected]> wrote:
>
> > Welcome,
> >
> > now we have 2.6.26.6 kernel and still terminal resize leads to
> > undesired effects. It is very inconvenient to wait for 2.6.27 for
> > corrections.
> >
> > As Alan Cox previously said mutexes generally work but as we can
> > observe in case of kill_pgrp() call inside mutex lock we got
> > race because of rescheduling so lock is not working here.
> > Rearanging code so the variable change is placed before kill_pgrp()
> > call removes mentioned race situaction.
> >
> > Signed-off-by: Adam Tla/lka <[email protected]>
> >
> > I strongly suggest to patch actual 2.6.26.x kernel to remove this
> > very nasty pts behaviour.
>
> NAK again
>
> Moving the copies around simply moves the race, it might be that it
> fixes your box and unfixes other peoples.
>
I don't think so. Race appears because of kill_pgrp() call which
generates SIGWINCH so it leads to reschedule and ioctl() which reads
termios sizes before they are updated - from time to time. So if we
update variables before signal generation there will be no race.
Moving the point of variables update eliminates
possibility of reading old values. So even if after kill_pgrp() the
other process will not lock here on this mutex values obtained will be
sane.
Whats more we could protect by mutex variable only test and change
operations and it stil will work correctly.
Because now we have 2.6.27 I tested this kind of code in
tty_io.c(tty_do_resize):
struct pid *pgrp, *rpgrp;
unsigned long flags;
/* For a PTY we need to lock the tty side */
mutex_lock(&real_tty->termios_mutex);
if ((flags = memcmp(ws, &tty->winsize, sizeof(*ws)))){
tty->winsize = *ws;
real_tty->winsize = *ws;
}
mutex_unlock(&real_tty->termios_mutex);
if (flags){
/* Get the PID values and reference them so we can
avoid holding the tty ctrl lock while sending signals */
spin_lock_irqsave(&tty->ctrl_lock, flags);
pgrp = get_pid(tty->pgrp);
rpgrp = get_pid(real_tty->pgrp);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
if (pgrp)
kill_pgrp(pgrp, SIGWINCH, 1);
if (rpgrp != pgrp && rpgrp)
kill_pgrp(rpgrp, SIGWINCH, 1);
put_pid(pgrp);
put_pid(rpgrp);
}
return 0;
So it works, and change of tty->winsize and real_tty->winsize are protected .
Why another process should wait more if winsize is already properly set?
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
Fri, 10 Oct 2008 12:35:17 +0200 - Adam Tlałka <[email protected]>:
> Fri, 10 Oct 2008 10:29:06 +0100 - Alan Cox <[email protected]>:
> >
> > NAK again
> >
> > Moving the copies around simply moves the race, it might be that it
> > fixes your box and unfixes other peoples.
> >
>
> I don't think so. Race appears because of kill_pgrp() call which
> generates SIGWINCH so it leads to reschedule and ioctl() which reads
> termios sizes before they are updated - from time to time. So if we
> update variables before signal generation there will be no race.
> Moving the point of variables update eliminates
> possibility of reading old values. So even if after kill_pgrp() the
> other process will not lock here on this mutex values obtained will be
> sane.
>
> Whats more we could protect by mutex variable only test and change
> operations and it stil will work correctly.
>
> Because now we have 2.6.27 I tested this kind of code in
> tty_io.c(tty_do_resize):
>
> ...
>
> So it works, and change of tty->winsize and real_tty->winsize are
> protected . Why another process should wait more if winsize is
> already properly set?
Next if we want to speed up our code in case of resize we could remove
one of two comparizons so values always be updated in tty_io.c(tty_do_resize):
struct pid *pgrp, *rpgrp;
unsigned long flags;
/* For a PTY we need to lock the tty side */
mutex_lock(&real_tty->termios_mutex);
flags = memcmp(ws, &tty->winsize, sizeof(*ws));
tty->winsize = *ws;
real_tty->winsize = *ws;
mutex_unlock(&real_tty->termios_mutex);
if (flags){
/* Get the PID values and reference them so we can
avoid holding the tty ctrl lock while sending signals */
spin_lock_irqsave(&tty->ctrl_lock, flags);
pgrp = get_pid(tty->pgrp);
rpgrp = get_pid(real_tty->pgrp);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
if (pgrp)
kill_pgrp(pgrp, SIGWINCH, 1);
if (rpgrp != pgrp && rpgrp)
kill_pgrp(rpgrp, SIGWINCH, 1);
put_pid(pgrp);
put_pid(rpgrp);
}
return 0;
We could assume that ioctl which sets the same values is rather rare
so we want faster code in case of changes. Presented above code for
kernel 2.6.27 works quit nicely and I can't observe any bad effect of it.
Anyway we can prove on paper by time diagrams that there will be no races
according to update and reading winsize variables.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
Alan Cox <[email protected]> wrote:
> Adam Tla?ka <[email protected]> wrote:
>> now we have 2.6.26.6 kernel and still terminal resize leads to
>> undesired effects. It is very inconvenient to wait for 2.6.27 for
>> corrections.
You'll have to wait some longer, since it still has this bug.
>> As Alan Cox previously said mutexes generally work but as we can
>> observe in case of kill_pgrp() call inside mutex lock we got
>> race because of rescheduling so lock is not working here.
>> Rearanging code so the variable change is placed before kill_pgrp()
>> call removes mentioned race situaction.
> NAK again
>
> Moving the copies around simply moves the race, it might be that it fixes
> your box and unfixes other peoples.
This patch does not move around any race, but it works around a locking issue
by making sure you are the hedgehog racing the rabbit.
However, you are right in spotting that there must be something wrong with
the resulting code. It does (still) modify both tty and the real_tty while
only holding one lock. Besides that, it depends on tty->mutex to prevent
reading the old values because real_tty->mutex is held.
Adam, since you are working on this issue, I'd suggest you modify the source
to take both locks, one at a time, while setting the new values (lock
tty->mutex, compare tty->ws, possibly set ws, unlock, lock real_tty, ...).
Alan, do you agree? Or is it required to take both locks at the same time?
If it is, in which order?
> Alan, do you agree? Or is it required to take both locks at the same time?
> If it is, in which order?
I don't agree. Please read the code more carefully. In paticular note
that TIOCGWINSZ is driven off the tty side of any tty/pty pair. That we
set the pty size termios bits is really a curiousity of history.
Alan
Welcome,
generally speaking terminal resize could be made from vc or other
terminal device module code, from terminal ioctl on tty master
or tty slave. As we read the code we will see that the problem is to
use a proper mutex. Also we should use a one variable in which we set
terminal sizes. So in case of tty master and slave we have tty and
real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
called on tty and real_tty at the same time. To avoid race condition
we should use a mutex. But this must be the same mutex in all cases
- tty or real_tty. For the best we should also use the same
winsize variable. Proposed previously code change only simplifies
the code and eliminates SIGWINCH signal race but an app could read
terminal sizes at any time so this patch not closes all race
possibilities. So I propose a patch in which we have some code
movements and always use real_tty->termios_mutex and also
real_tty->winsize. In no pty case tty == real_tty so it works properly
as before and in pty situaction we use the same mutex and variable so
it removes all race conditions according to access to winsize now.
Signed-off-by: Adam Tla/lka <[email protected]>
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
O> real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
> called on tty and real_tty at the same time. To avoid race condition
No they can't. Would you please bother to spend five minutes actually
reading the source code and following through your assumptions to see if
they make sense before posting.
tiocgwinsz is never called for the pty side of a pty pair.
Sun, 12 Oct 2008 15:22:00 +0100 - Alan Cox <[email protected]>:
> O> real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
> > called on tty and real_tty at the same time. To avoid race condition
>
> No they can't. Would you please bother to spend five minutes actually
> reading the source code and following through your assumptions to see
> if they make sense before posting.
>
> tiocgwinsz is never called for the pty side of a pty pair.
I've read the code. The race problem with xterm or other pty using
program in 2.6.26 appeared because one app called ioctl(TIOCSWINSZ) on
the master side while other read winsize (TIOCGWINSZ) using client side
(slave). So in one ioctl() call tty == master and in other tty ==
real_tty. Of course we can have the opposite situaction so terminal app
is using ioctl(TIOCSWINSZ) on its side (slave) and xterm is using ioctl
on its side to know to which size resize itself. Not working now as I
tested but possible.
Anyway I think that you miss the point. Why using
real_tty->termios_mutex instead of tty->termios_mutex in tty_do_resize
called from tiocswins() so from ioctl(TIOCSWINSZ) closes the race. If as
you said tiocgwinsz is called on tty and not real_tty then
tty->termios_mutex should be valid here.
Mutexes work and it is not a scheduler problem as I wrongly assumed.
The scheduler just exposed this problem doing an app switch.
It's just wrong mutex used.
Look at the tty_ioctl(struct file *file, unsigned int cmd, unsigned
long arg) in tty_io.c.
tty = (struct tty_struct *)file->private_data;
so if you calling ioctl on master side we have tty = master
and on client side tty = real_tty in ioctl entry;
next
real_tty = tty;
if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
tty->driver->subtype == PTY_TYPE_MASTER)
real_tty = tty->link;
if tty is the master one we set real_tty but in case of client side
tty == real_tty already so real_tty points to the same structure.
So it seems that tty->termios_mutex could point to different
location in different calls but real_tty->termios_mutex always points
to the same location.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
> I've read the code. The race problem with xterm or other pty using
Clearly not.
> program in 2.6.26 appeared because one app called ioctl(TIOCSWINSZ) on
> the master side while other read winsize (TIOCGWINSZ) using client side
> (slave). So in one ioctl() call tty == master and in other tty ==
> real_tty. Of course we can have the opposite situaction so terminal app
Then we end up with both using real_tty.
> Anyway I think that you miss the point. Why using
> real_tty->termios_mutex instead of tty->termios_mutex in tty_do_resize
To avoid deadlocks if you took both as you updated both structures.
> So it seems that tty->termios_mutex could point to different
> location in different calls but real_tty->termios_mutex always points
> to the same location.
You've finally got there - we always work off real_tty. That is why we
can safely use the mutex on the real_tty side.
Alan
Sun, 12 Oct 2008 19:03:12 +0100 - Alan Cox <[email protected]>:
> Then we end up with both using real_tty.
>
> > Anyway I think that you miss the point. Why using
> > real_tty->termios_mutex instead of tty->termios_mutex in
> > tty_do_resize
>
> To avoid deadlocks if you took both as you updated both structures.
Actually to avoid race and not valid winsize reading because
ioctl(TIOCGWINSZ) is done with tty == real_tty so we have to use this
mutex and not the other one.
What is more because we are using real_tty we could safely use only
real_tty->winsize and forget about master tty->winsize.
> > So it seems that tty->termios_mutex could point to different
> > location in different calls but real_tty->termios_mutex always
> > points to the same location.
>
> You've finally got there - we always work off real_tty. That is why we
> can safely use the mutex on the real_tty side.
typically ioctl(,TIOCSWINSZ,) is called on master side but
ioctl(,TIOCGWINSZ,) is called on slave side so tty is not the same in
both calls inside ioctl() in tty_io.c. Only real_tty variable has the
same value for the same master/slave pair. So we must use real_tty
mutex all the time because we not always work from the same side IMHO.
That is why I propose usage of real_tty in ioctl() handling:
case TIOCGWINSZ:
return tiocgwinsz(real_tty, p);
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
> That is why I propose usage of real_tty in ioctl() handling:
>
> case TIOCGWINSZ:
> return tiocgwinsz(real_tty, p);
Congratulations we've finally got there - and that is exactly the code in
the current tty tree.
Alan
On Sun, 12 Oct 2008, Alan Cox wrote:
> > That is why I propose usage of real_tty in ioctl() handling:
> >
> > case TIOCGWINSZ:
> > return tiocgwinsz(real_tty, p);
>
> Congratulations we've finally got there - and that is exactly the code in
> the current tty tree.
These patches are missing from 2.6.27. There you have:
case TIOCGWINSZ:
return tiocgwinsz(tty, p);
--
WAR IS PEACE
FREEDOM IS SLAVERY
IGNORANCE IS STRENGTH
On Mon, 13 Oct 2008 11:59:52 +0200 (CEST)
Bodo Eggert <[email protected]> wrote:
> On Sun, 12 Oct 2008, Alan Cox wrote:
>
> > > That is why I propose usage of real_tty in ioctl() handling:
> > >
> > > case TIOCGWINSZ:
> > > return tiocgwinsz(real_tty, p);
> >
> > Congratulations we've finally got there - and that is exactly the code in
> > the current tty tree.
>
> These patches are missing from 2.6.27. There you have:
>
> case TIOCGWINSZ:
> return tiocgwinsz(tty, p);
Yes I know. The rules for the stable tree are that it has to go upstream
first. It has not gone upstream so it cannot go into the stable tree yet.
As and when Linus takes the relevant changes I will send a set to the
stable tree.
Alan
On Mon, 13 Oct 2008, Alan Cox wrote:
> Bodo Eggert <[email protected]> wrote:
> > On Sun, 12 Oct 2008, Alan Cox wrote:
> > > > That is why I propose usage of real_tty in ioctl() handling:
> > > >
> > > > case TIOCGWINSZ:
> > > > return tiocgwinsz(real_tty, p);
> > >
> > > Congratulations we've finally got there - and that is exactly the code in
> > > the current tty tree.
> >
> > These patches are missing from 2.6.27. There you have:
> >
> > case TIOCGWINSZ:
> > return tiocgwinsz(tty, p);
>
> Yes I know. The rules for the stable tree are that it has to go upstream
> first. It has not gone upstream so it cannot go into the stable tree yet.
> As and when Linus takes the relevant changes I will send a set to the
> stable tree.
Nice to hear. I misunderstood the waiting for 2.6.27 part as in "Will be
fixed there" ... until I saw your patchset.
--
You must be the arithmetic man -- you add trouble, subtract pleasure, divide
attention, and multiply ignorance. You have an inferiority complex -- and
it's fully justified. You used to be arrogant and obnoxious. Now you are
just the opposite. You are obnoxious and arrogant. -- Harry Murphy in a.2.hz
Hello,
I am sending sligtly corrected patch.
In case of ioctl(,TIOCSWINSZ,) on pty side we should send signal to
master side too. I've tested it on modified unix putty version
and it works properly.
Signed-off-by: Adam Tla/lka <[email protected]>
Maybe we could optimize code more and not call
get_pid() and put_pid() if they are not needed - in case
where there is no master/slave pair.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
> Hello,
>
> I am sending sligtly corrected patch.
> In case of ioctl(,TIOCSWINSZ,) on pty side we should send signal to
> master side too. I've tested it on modified unix putty version
> and it works properly.
>
> Signed-off-by: Adam Tla/lka <[email protected]>
>
> Maybe we could optimize code more and not call
> get_pid() and put_pid() if they are not needed - in case
> where there is no master/slave pair.
Not it is after small changes and proper path in patch.
Signed-off-by: Adam Tla/lka <[email protected]>
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
Hello,
actual pty ioctl(,TIOCSWINSZ,) handling is broken IMHO.
I we do that action on normal tty (console for example)
some resize is done or not and resulting size variables are updated
and signal generated. In case of pty ioctl() on slave side it just sets
pty size variables, generates SIGWINCH, but terminal is not changed so
a terminal app will go crazy now. I propose changes which lead to more
consistent handling:
1. set in non pty master/slave case: no changes
2. set in pty master case: we update all sizes and do SIGWINCH on slave
side
3. set in pty slave case: we update only master variable and do
SIGWINCH on master side
4. read: master reads master variable while slave reads slave variable
Now if xterm resizes itself then a program on slave gets its signal
but if this program sets terminal sizes by ioctl then only xterm gets
the SIGWINCH signal and could read desired sizes by ioctl and then
resize itself and set valid sizes on slave side by another ioctl() call.
If it not supports this method then there will be no changes on slave
side. I think that it is more proper so on the slave side we will see
always actual values and if terminal resizes we will get SIGWINCH.
Signed-off-by: Adam Tla/lka <[email protected]>
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
> and signal generated. In case of pty ioctl() on slave side it just sets
> pty size variables, generates SIGWINCH, but terminal is not changed so
> a terminal app will go crazy now. I propose changes which lead to more
> consistent handling:
It sets the tty and pty side variables.
> Now if xterm resizes itself then a program on slave gets its signal
> but if this program sets terminal sizes by ioctl then only xterm gets
> the SIGWINCH signal and could read desired sizes by ioctl and then
> resize itself and set valid sizes on slave side by another ioctl() call.
> If it not supports this method then there will be no changes on slave
> side. I think that it is more proper so on the slave side we will see
> always actual values and if terminal resizes we will get SIGWINCH.
The current and historic behaviour is I believe correct and matches other
Unixes.
Your patch doesn't really seem to make a lot of sense either. You add pty
special cases in places they are not needed and you pass various extra
arguments to functions that don't need them.
I did actually have a glance at the pty signalling question a couple of
days ago while further tidying up the default resize logic - see the
ttydev tree. I'm cautious about changing the signal behaviour however
without having a hard look to see whether any other Unixen has that
behaviour currently as we may risk breaking stuff.
Alan
Welcome,
Thu, 16 Oct 2008 11:52:02 +0100 - Alan Cox <[email protected]>:
> > Now if xterm resizes itself then a program on slave gets its signal
> > but if this program sets terminal sizes by ioctl then only xterm
> > gets the SIGWINCH signal and could read desired sizes by ioctl and
> > then resize itself and set valid sizes on slave side by another
> > ioctl() call. If it not supports this method then there will be no
> > changes on slave side. I think that it is more proper so on the
> > slave side we will see always actual values and if terminal resizes
> > we will get SIGWINCH.
>
> The current and historic behaviour is I believe correct and matches
> other Unixes.
I don't know what it means ,,correct'' here. stty(,TIOCSWINSZ,) on pty
slave in its correct behaviour is unusable and leads to application
confuse and sometimes crash.
> Your patch doesn't really seem to make a lot of sense either. You add
> pty special cases in places they are not needed and you pass various
> extra arguments to functions that don't need them.
I do not agree. Adding real_tty arg to tiocgwinsz() is for purpose
of using always the same mutex but read winsize depending on call
side (master or slave). So master could get what it should set and
slave reads always actual terminal values corresponding to real
terminal sizes.
> I did actually have a glance at the pty signalling question a couple
> of days ago while further tidying up the default resize logic - see
> the ttydev tree. I'm cautious about changing the signal behaviour
> however without having a hard look to see whether any other Unixen
> has that behaviour currently as we may risk breaking stuff.
Other Unixes seem to have the ,,correct'' not quite usable behaviour ;).
If we want to stick to this my previous patch - [PATCH 0/4] seems to be
suitable.
It only corrects situaction with mutex usage and sending signal to the
master side in case of ioctl(,TIOCSWINS,) done on the slave side.
So a teminal emulator can get signal too and react. But 1. an app can
get its signal before terminal emulator while it is not really resized
yet; 2. a terminal emulator does not support SIGWINCH so we have no
resize but variable is updated and signal generated. So it is not usable
anymore till terminal emulator resizes by itself - but if it changes
sizes to values retuned by ioctl(,TIOCGWINSZ,) there will be no
SIGWINCH send to the app. So you have to manually force it to repaint
its screen so it looks correctly.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland
Welcome,
If we want to be more consistent with console behaviour the ptm/pty
behaviour should be similar. In my opinion it could be done
be using some kind of callback function which could be registered by
the program which rules ptm. Then it could be activated through
tty->ops->resize from tiocswinsz in tty_io. The default will be not set
so we get current typical behaviour.
We could also use some default function which calls tty_do_resize() if
called from master side and does nothing if called from slave side
so there is no change because real terminal sizes are not changed.
The terminal emulator program should register its callback
function which leads to proper resize and then signal generating by
ioctl(ptm,TIOCSWINSZ,). It is different from current behaviour but more
usable and sane IMHO.
The current ,,proper'' behaviour is inconsistent. For example you can
do "stty rows 0" on console and there is no change at all and no error
reporting too. With pty we get SIGWINCH signal and variable rows is
set to 0. Of course the real physical terminal is not changed at all.
Regards
--
Adam Tlałka mailto:[email protected] ^v^ ^v^ ^v^
System & Network Administration Group - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland