Hi,
The problem:
Large pastes (5k or more) into a readline enabled program fail when
running kernels larger than v2.6.31-rc5. "Fail" means that some lines
are incomplete. From v2.6.39-rc1 onwards, "some lines" become "almost
all lines after the first 4k". This turns up at least in Fedora,
Debian, Ubuntu and Gentoo. From our findings, it should happen in any
readline enabled program on a system running kernels later than the
mentioned ones.
The problematic commits in the kernel tree:
1 - 2009-07-27 (never shipped) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a54297478e6578f96fd54bf4daa1751130aca86
After this commit, pastes start breaking. For a 35k file, about 50%
of the times one or two lines are partially incomplete.
2 - 2009-07-29 (v2.6.31-rc5) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e043e42bdb66885b3ac10d27a01ccb9972e2b0a3
This commit reverts the previous one, but adds one extra call to
flush_to_ldisc. Pastes still break, commenting out the function call
prevents breakage *up to 2.6.39-rc1*.
3 - 2011-03-22 (v2.6.39-rc1) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f23eb2b2b28547fc70df82dd5049eb39bec5ba12
This commit changes many schedule/flush/cancel_delayed_work calls into
schedule/flush/cancel_work. After this commit, the big breakage
starts: for the 35k example file, it starts breaking at aprox. 4k and
then every line is partially incomplete or directly not there.
Still after this commit, commenting out the tty_flush_to_ldisc(tty)
call added by e043e42bdb66885b3ac10d27a01ccb9972e2b0a3 prevents the
breakage.
4 - 2011-04-04 (v2.6.39-rc2) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5660b41af6a28f8004e70eb261e1202ad55c5e3
This commit modifies the behaviour of how the ttys are polled. After
this commit, commenting out the tty_flush_to_ldisc(tty) call doesn't
prevent breakage anymore.
But then re-adding the call to schedule_work(&tty->buf.work) that was
removed in this commit, prevents the breakage even up to 3.11-rc2.
I'm attaching a diff of the patch that we applied, just to show what
had to be done, this is not a proposed fix, because this does cause a
busy loop that is particularly noticeable in VMs.
We haven't yet found a good fix for this issue, but we believe that
pasting into readline enabled programs shouldn't cause characters to
get lost, and it should be possible to do that properly without the
busy loop.
***
This was originally reported as a bug in readline, but it was found
that going back to very old kernels prevented the issue, regardless of
the version of readline.
Original Report (2012-06-25):
http://lists.gnu.org/archive/html/bug-readline/2012-06/msg00006.html
Follow Up thread (2013-07-22):
http://lists.gnu.org/archive/html/bug-readline/2013-07/msg00006.html
I'm attaching here a very simple readline enabled program that helps
with performing tests. Compile, run, then copy and paste a large
enough file into it, close and diff.
Looking at the code in readline, the issue is triggered by these lines
in rltty.c, while preparing the input:
tiop->c_lflag &= ~(ICANON | ECHO);
(...)
tiop->c_iflag &= ~(ICRNL | INLCR);
If those two lines are replaced by:
tiop->c_lflag &= ~(ECHO);
(...)
tiop->c_iflag &= ~(INLCR);
Then the pastes work fine: no lines are missing. Of course, this
means that readline doesn't work properly, but this is just to note
that those are the terminal settings that cause the issue to pop-up.
Credit: this investigation was done together with Maximiliano Curia.
--
Regards,
Margarita Manterola
On 07/25/2013 07:29 AM, Margarita Manterola wrote:
> Hi,
>
> The problem:
> Large pastes (5k or more) into a readline enabled program fail when
> running kernels larger than v2.6.31-rc5. "Fail" means that some lines
> are incomplete. From v2.6.39-rc1 onwards, "some lines" become "almost
> all lines after the first 4k". This turns up at least in Fedora,
> Debian, Ubuntu and Gentoo. From our findings, it should happen in any
> readline enabled program on a system running kernels later than the
> mentioned ones.
>
> The problematic commits in the kernel tree:
> 1 - 2009-07-27 (never shipped) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a54297478e6578f96fd54bf4daa1751130aca86
>
> After this commit, pastes start breaking. For a 35k file, about 50%
> of the times one or two lines are partially incomplete.
>
> 2 - 2009-07-29 (v2.6.31-rc5) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e043e42bdb66885b3ac10d27a01ccb9972e2b0a3
>
> This commit reverts the previous one, but adds one extra call to
> flush_to_ldisc. Pastes still break, commenting out the function call
> prevents breakage *up to 2.6.39-rc1*.
>
> 3 - 2011-03-22 (v2.6.39-rc1) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f23eb2b2b28547fc70df82dd5049eb39bec5ba12
>
> This commit changes many schedule/flush/cancel_delayed_work calls into
> schedule/flush/cancel_work. After this commit, the big breakage
> starts: for the 35k example file, it starts breaking at aprox. 4k and
> then every line is partially incomplete or directly not there.
>
> Still after this commit, commenting out the tty_flush_to_ldisc(tty)
> call added by e043e42bdb66885b3ac10d27a01ccb9972e2b0a3 prevents the
> breakage.
>
> 4 - 2011-04-04 (v2.6.39-rc2) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5660b41af6a28f8004e70eb261e1202ad55c5e3
>
> This commit modifies the behaviour of how the ttys are polled. After
> this commit, commenting out the tty_flush_to_ldisc(tty) call doesn't
> prevent breakage anymore.
>
> But then re-adding the call to schedule_work(&tty->buf.work) that was
> removed in this commit, prevents the breakage even up to 3.11-rc2.
> I'm attaching a diff of the patch that we applied, just to show what
> had to be done, this is not a proposed fix, because this does cause a
> busy loop that is particularly noticeable in VMs.
>
> We haven't yet found a good fix for this issue, but we believe that
> pasting into readline enabled programs shouldn't cause characters to
> get lost, and it should be possible to do that properly without the
> busy loop.
>
> ***
> This was originally reported as a bug in readline, but it was found
> that going back to very old kernels prevented the issue, regardless of
> the version of readline.
>
> Original Report (2012-06-25):
> http://lists.gnu.org/archive/html/bug-readline/2012-06/msg00006.html
> Follow Up thread (2013-07-22):
> http://lists.gnu.org/archive/html/bug-readline/2013-07/msg00006.html
>
> I'm attaching here a very simple readline enabled program that helps
> with performing tests. Compile, run, then copy and paste a large
> enough file into it, close and diff.
>
> Looking at the code in readline, the issue is triggered by these lines
> in rltty.c, while preparing the input:
>
> tiop->c_lflag &= ~(ICANON | ECHO);
> (...)
> tiop->c_iflag &= ~(ICRNL | INLCR);
>
> If those two lines are replaced by:
>
> tiop->c_lflag &= ~(ECHO);
> (...)
> tiop->c_iflag &= ~(INLCR);
>
> Then the pastes work fine: no lines are missing. Of course, this
> means that readline doesn't work properly, but this is just to note
> that those are the terminal settings that cause the issue to pop-up.
>
> Credit: this investigation was done together with Maximiliano Curia.
>
readline is fundamentally incompatible with an active writer.
readline() saves and restores the termios settings for each input
line it reads. However, tty i/o is asynchronous in the kernel.
This means that when readline() restores the original termios
settings, any new data received by the tty will be interpreted
with the current, original termios settings.
When a large paste happens, the tty/line discipline read buffer
quickly fills up (to 4k). When full, further input is forced to
wait. After readline() reads an input line, more space becomes
available in the read buffer. Unfortunately, this event roughly
coincides with restoring the original termios settings, and
thus increases the probability that more paste data will be
received with the wrong termios settings.
That's why the patches that involve scheduling the receive
buffer work seem to have some effect on the outcome.
As you've already noted, readline() termios settings are
substantially different than the default termios settings.
Below I've included a simple test jig that
1) sets termios to the same settings as readline()
2) uses the same read() method as readline()
3) outputs what it reads to stdout
4) restores the original termios
Note that the test jig differs from readline() in that
it reads _all_ the available input without changing
termios.
The test jig will identically duplicate a large paste. Feel
free to modify the test jig to change the termios settings
per-line.
Regards,
Peter Hurley
[ NB: In the test jig, I didn't bother with ensuring eof is
the first input of a new line.
I converted CR -> NL because that's what your test
program does, in conjunction with readline(). Pasted lines
are CR-terminated; readline() returns the input line less the
CR; your test program prints the line followed by NL.
]
--- >% ---
#include <stdio.h>
#include <termios.h>
#include <unistd.h>
#include <stdlib.h>
int main(int argc, char* argv[]) {
int c, eof;
ssize_t actual;
struct termios termios, save;
int err;
err = tcgetattr(STDIN_FILENO, &termios);
if (err < 0)
exit(EXIT_FAILURE);
save = termios;
termios.c_lflag &= ~(ICANON | ECHO | ISIG);
termios.c_iflag &= ~(IXON | IXOFF);
if ((termios.c_cflag & CSIZE) == CS8)
termios.c_iflag &= ~(ISTRIP | INPCK);
termios.c_iflag &= ~(ICRNL | INLCR);
termios.c_cc[VMIN] = 1;
termios.c_cc[VTIME] = 0;
termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
eof = termios.c_cc[VEOF];
err = tcsetattr(STDIN_FILENO, TCSANOW, &termios);
if (err < 0)
exit(EXIT_FAILURE);
while (1) {
actual = read( fileno(stdin), &c, sizeof(unsigned char));
if (actual <= 0)
break;
if (actual == sizeof(unsigned char)) {
if (c == eof)
break;
if (c == '\r')
c = '\n';
fputc(c, stdout);
}
fflush(stdout);
}
err = tcsetattr(STDIN_FILENO, TCSAFLUSH, &save);
if (err < 0)
exit(EXIT_FAILURE);
return 0;
}
Peter Hurley wrote:
> readline is fundamentally incompatible with an active writer.
This wasn't the case with older kernel versions. I don't see any POSIX
reference that claims user input could be lost setting termios so I think
this is a serious regression.
Also, consider the readline use cases. bash, for instance, uses readline to
process the command lines entered, but needs to change the termios to a
canonical mode for each entered command. I would expect that pasting a
sequence of commands (of 4K, which is not even 'a lot') to work.
The same is true for psql, where users might paste several KB of queries, or
almost every readline enabled "shell".
> readline() saves and restores the termios settings for each input
> line it reads. However, tty i/o is asynchronous in the kernel.
> This means that when readline() restores the original termios
> settings, any new data received by the tty will be interpreted
> with the current, original termios settings.
> When a large paste happens, the tty/line discipline read buffer
> quickly fills up (to 4k). When full, further input is forced to
> wait. After readline() reads an input line, more space becomes
> available in the read buffer. Unfortunately, this event roughly
> coincides with restoring the original termios settings, and
> thus increases the probability that more paste data will be
> received with the wrong termios settings.
> That's why the patches that involve scheduling the receive
> buffer work seem to have some effect on the outcome.
It's not totally clear to me why receiving characters with the wrong termios
settings might lead to this characters being dropped when reading them with
different settings.
I took a deep look into the code, trying to find where was the code that ended
up dropping characters, but could not find it.
Could you maybe point me to it?
> As you've already noted, readline() termios settings are
> substantially different than the default termios settings.
>
> Below I've included a simple test jig that
> 1) sets termios to the same settings as readline()
> 2) uses the same read() method as readline()
> 3) outputs what it reads to stdout
> 4) restores the original termios
I've updated your code the be closer to the readline behaviour. readline
calls tcsetattr with TCSADRAIN, and not TCSAFLUSH which explictly claims to
discard the input. I've also reordered the call to process lines, and
initialized the int c.
--- >% ---
#include <stdio.h>
#include <termios.h>
#include <unistd.h>
#include <stdlib.h>
void init(int *eof, struct termios* save)
{
int err;
static struct termios termios;
err = tcgetattr(STDIN_FILENO, &termios);
if (err < 0)
exit(EXIT_FAILURE);
*save = termios;
termios.c_lflag &= ~(ICANON | ECHO | ISIG);
termios.c_iflag &= ~(IXON | IXOFF);
if ((termios.c_cflag & CSIZE) == CS8)
termios.c_iflag &= ~(ISTRIP | INPCK);
termios.c_iflag &= ~(ICRNL | INLCR);
termios.c_cc[VMIN] = 1;
termios.c_cc[VTIME] = 0;
termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
*eof = termios.c_cc[VEOF];
err = tcsetattr(STDIN_FILENO, TCSADRAIN, &termios);
if (err < 0)
exit(EXIT_FAILURE);
}
void deinit(struct termios* termios)
{
int err;
err = tcsetattr(STDIN_FILENO, TCSADRAIN, termios);
if (err < 0)
exit(EXIT_FAILURE);
}
int main(int argc, char* argv[])
{
int c=0, eof;
ssize_t actual;
struct termios save;
while (1) {
init(&eof, &save);
while (1) {
actual = read(fileno(stdin), &c, sizeof(unsigned char));
if (actual <= 0)
break;
if (actual == sizeof(unsigned char)) {
if (c == eof)
break;
if (c == '\r') {
c = '\n';
}
fputc(c, stdout);
fflush(stdout);
if (c == '\n') break;
}
}
deinit(&save);
if (c == eof) break;
}
return 0;
}
--- >% ---
--
"Seek simplicity, and distrust it." -- Whitehead's Rule
Saludos /\/\ /\ >< `/
Please don't drop CCs.
[ +cc Margarita Manterola, Greg Kroah-Hartman, Jiri Slaby, linux-kernel ]
On 07/30/2013 08:41 AM, Maximiliano Curia wrote:
> Peter Hurley wrote:
>
>> readline is fundamentally incompatible with an active writer.
>
> This wasn't the case with older kernel versions. I don't see any POSIX
> reference that claims user input could be lost setting termios so I think
> this is a serious regression.
I'm only interested in this discussion on the basis of a kernel
regression, because this behavior is fully POSIX compliant.
From POSIX 2008, Section 11. General Terminal Interface, Part 11.1.6.
Canonical Mode Input Processing:
In canonical mode input processing, terminal input is processed in
units of lines. A line is delimited by a <newline> character (NL),
an end-of-file character (EOF), or an end-of-line (EOL) character.
See Special Characters for more information on EOF and EOL. This
means that a read request shall not return until an entire line has
been typed or a signal has been received. Also, no matter how many
bytes are requested in the read() call, at most one line shall be
returned. It is not, however, necessary to read a whole line at once;
any number of bytes, even one, may be requested in a read() without
losing information.
If {MAX_CANON} is defined for this terminal device, it shall be a
limit on the number of bytes in a line. The behavior of the system
when this limit is exceeded is implementation-defined.
In other words, while processing input in canonical mode, the system may
discard input in excess of MAX_CANON until a NL character is received.
Linux defines MAX_CANON as 255.
> Also, consider the readline use cases. bash, for instance, uses readline to
> process the command lines entered, but needs to change the termios to a
> canonical mode for each entered command. I would expect that pasting a
> sequence of commands (of 4K, which is not even 'a lot') to work.
>
> The same is true for psql, where users might paste several KB of queries, or
> almost every readline enabled "shell".
>
>> readline() saves and restores the termios settings for each input
>> line it reads. However, tty i/o is asynchronous in the kernel.
>> This means that when readline() restores the original termios
>> settings, any new data received by the tty will be interpreted
>> with the current, original termios settings.
>
>> When a large paste happens, the tty/line discipline read buffer
>> quickly fills up (to 4k). When full, further input is forced to
>> wait. After readline() reads an input line, more space becomes
>> available in the read buffer. Unfortunately, this event roughly
>> coincides with restoring the original termios settings, and
>> thus increases the probability that more paste data will be
>> received with the wrong termios settings.
>
>> That's why the patches that involve scheduling the receive
>> buffer work seem to have some effect on the outcome.
>
> It's not totally clear to me why receiving characters with the wrong termios
> settings might lead to this characters being dropped when reading them with
> different settings.
Many termios settings are applied when the input is received rather than when
the application reads them.
Here's an example sequence:
1. termios is set to non-canonical mode
2. the input buffer is filled with paste data (although unknown to the kernel
there is more paste data which has not yet been received). The paste data
contains CR as line delimiter.
3. the input buffer is read character-by-character until the first CR is found.
4. termios is set to canonical mode but ICRNL is not set.
5. the writer thread is woken because more space has become available in the
input buffer.
6. more paste data is received in canonical mode but a NL cannot be found
(because there are only CRs in the paste data).
7. the system discards this input to prevent already received data from being
overrun and continues to do so until a NL is found (or termios is set back
to non-canonical mode).
> I took a deep look into the code, trying to find where was the code that ended
> up dropping characters, but could not find it.
> Could you maybe point me to it?
n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
From n_tty_set_room():
/*
* If we are doing input canonicalization, and there are no
* pending newlines, let characters through without limit, so
* that erase characters will be handled. Other excess
* characters will be beeped.
*/
if (left <= 0)
left = ldata->icanon && !ldata->canon_data;
old_left = tty->receive_room;
tty->receive_room = left;
The code means that if the input buffer is full and ICANON is set and
a NL has not been received yet, still report that space for 1 char
is available in the input buffer.
When the character is actually received and interpreted, if there is
no space in the input buffer, the character is dropped. If ECHO is set,
a '\a' (bell) is written to the tty's output.
>> As you've already noted, readline() termios settings are
>> substantially different than the default termios settings.
>>
>> Below I've included a simple test jig that
>> 1) sets termios to the same settings as readline()
>> 2) uses the same read() method as readline()
>> 3) outputs what it reads to stdout
>> 4) restores the original termios
>
> I've updated your code the be closer to the readline behaviour.
That's why I wrote the test jig: to show the paste data was
received error-free with _exactly_ the same termios settings
that readline() uses.
Re-introducing setting and unsetting termios simply confirms
what I already diagnosed.
Regards,
Peter Hurley
> readline calls tcsetattr with TCSADRAIN, and not TCSAFLUSH which explictly
> claims to discard the input. I've also reordered the call to process lines,
> and initialized the int c.
>
> --- >% ---
> #include <stdio.h>
> #include <termios.h>
> #include <unistd.h>
> #include <stdlib.h>
>
> void init(int *eof, struct termios* save)
> {
> int err;
> static struct termios termios;
>
> err = tcgetattr(STDIN_FILENO, &termios);
> if (err < 0)
> exit(EXIT_FAILURE);
> *save = termios;
>
> termios.c_lflag &= ~(ICANON | ECHO | ISIG);
> termios.c_iflag &= ~(IXON | IXOFF);
> if ((termios.c_cflag & CSIZE) == CS8)
> termios.c_iflag &= ~(ISTRIP | INPCK);
> termios.c_iflag &= ~(ICRNL | INLCR);
> termios.c_cc[VMIN] = 1;
> termios.c_cc[VTIME] = 0;
> termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
> *eof = termios.c_cc[VEOF];
>
> err = tcsetattr(STDIN_FILENO, TCSADRAIN, &termios);
> if (err < 0)
> exit(EXIT_FAILURE);
> }
>
> void deinit(struct termios* termios)
> {
> int err;
> err = tcsetattr(STDIN_FILENO, TCSADRAIN, termios);
> if (err < 0)
> exit(EXIT_FAILURE);
> }
>
> int main(int argc, char* argv[])
> {
> int c=0, eof;
> ssize_t actual;
> struct termios save;
>
> while (1) {
> init(&eof, &save);
> while (1) {
> actual = read(fileno(stdin), &c, sizeof(unsigned char));
> if (actual <= 0)
> break;
> if (actual == sizeof(unsigned char)) {
> if (c == eof)
> break;
> if (c == '\r') {
> c = '\n';
> }
> fputc(c, stdout);
> fflush(stdout);
> if (c == '\n') break;
> }
> }
> deinit(&save);
> if (c == eof) break;
> }
>
> return 0;
> }
> --- >% ---
>
Hi,
> n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
> From n_tty_set_room():
> /*
> * If we are doing input canonicalization, and there are no
> * pending newlines, let characters through without limit, so
> * that erase characters will be handled. Other excess
> * characters will be beeped.
> */
> if (left <= 0)
> left = ldata->icanon && !ldata->canon_data;
> old_left = tty->receive_room;
> tty->receive_room = left;
I took a long look at this code and thought about how it could be made to work
for readline's case and also for the canonical readers. I came up with this
simple patch:
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..2ba7f4e 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left <= 0)
- left = ldata->icanon && !ldata->canon_data;
+ if (waitqueue_active(&tty->read_wait))
+ left = ldata->icanon && !ldata->canon_data;
old_left = tty->receive_room;
tty->receive_room = left;
This is of course just an idea, but I tested this and it worked correctly for
the cases I was testing.
The effect of this patch is that when there is a canonical reader waiting for
input, it maintains the previous behavior, but when there's no reader (like
when readline is changing modes), it blocks and doesn't lose any characters.
Another approach would be to recalculate the size of canon_data when the mode
is changed, but this would probably be much more invasive, and awfully less
efficient since it would imply going through the buffer.
What do you think? Is the proposed solution, or something along those lines,
acceptable?
If there are other cases that need to be taken into account and that I
currently don't know about, please let me know.
--
"If you think your users are idiots, only idiots will use it."
-- Linus Torvalds
Saludos /\/\ /\ >< `/
Hi!
> > n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
>
> > From n_tty_set_room():
>
> > /*
> > * If we are doing input canonicalization, and there are no
> > * pending newlines, let characters through without limit, so
> > * that erase characters will be handled. Other excess
> > * characters will be beeped.
> > */
> > if (left <= 0)
> > left = ldata->icanon && !ldata->canon_data;
> > old_left = tty->receive_room;
> > tty->receive_room = left;
>
> I took a long look at this code and thought about how it could be made to work
> for readline's case and also for the canonical readers. I came up with this
> simple patch:
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..2ba7f4e 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> * characters will be beeped.
> */
> if (left <= 0)
> - left = ldata->icanon && !ldata->canon_data;
> + if (waitqueue_active(&tty->read_wait))
> + left = ldata->icanon && !ldata->canon_data;
> old_left = tty->receive_room;
> tty->receive_room = left;
>
> This is of course just an idea, but I tested this and it worked correctly for
> the cases I was testing.
>
> The effect of this patch is that when there is a canonical reader waiting for
> input, it maintains the previous behavior, but when there's no reader (like
> when readline is changing modes), it blocks and doesn't lose any characters.
>
> Another approach would be to recalculate the size of canon_data when the mode
> is changed, but this would probably be much more invasive, and awfully less
> efficient since it would imply going through the buffer.
>
> What do you think? Is the proposed solution, or something along those lines,
> acceptable?
>
> If there are other cases that need to be taken into account and that I
> currently don't know about, please let me know.
Was this applied? You may want to cc rjw... it is a regression, it is
not pretty, and it is something I blieve I hit but thought it was some
kind of "X weirdness".
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi,
On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <[email protected]> wrote:
>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>> index 4bf0fc0..2ba7f4e 100644
>> --- a/drivers/tty/n_tty.c
>> +++ b/drivers/tty/n_tty.c
>> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
>> * characters will be beeped.
>> */
>> if (left <= 0)
>> - left = ldata->icanon && !ldata->canon_data;
>> + if (waitqueue_active(&tty->read_wait))
>> + left = ldata->icanon && !ldata->canon_data;
>> old_left = tty->receive_room;
>> tty->receive_room = left;
> Was this applied? You may want to cc rjw... it is a regression, it is
> not pretty, and it is something I blieve I hit but thought it was some
> kind of "X weirdness".
There were no replies to the previous mail asking for comments, and as
far as I can see this has not been applied. I don't know who rjw is,
could you be a bit more explicit, please?
--
Besos,
Marga
On Sun, Aug 18, 2013 at 12:57 AM, Margarita Manterola
<[email protected]> wrote:
> There were no replies to the previous mail asking for comments, and as
> far as I can see this has not been applied. I don't know who rjw is,
> could you be a bit more explicit, please?
"git grep rjw MAINTAINERS"
Rafael J. Wysocki <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 08/08/2013 01:58 PM, Maximiliano Curia wrote:
> Hi,
>
>> n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
>
>> From n_tty_set_room():
>
>> /*
>> * If we are doing input canonicalization, and there are no
>> * pending newlines, let characters through without limit, so
>> * that erase characters will be handled. Other excess
>> * characters will be beeped.
>> */
>> if (left <= 0)
>> left = ldata->icanon && !ldata->canon_data;
>> old_left = tty->receive_room;
>> tty->receive_room = left;
>
> I took a long look at this code and thought about how it could be made to work
> for readline's case and also for the canonical readers. I came up with this
> simple patch:
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..2ba7f4e 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> * characters will be beeped.
> */
> if (left <= 0)
> - left = ldata->icanon && !ldata->canon_data;
> + if (waitqueue_active(&tty->read_wait))
> + left = ldata->icanon && !ldata->canon_data;
> old_left = tty->receive_room;
> tty->receive_room = left;
>
> This is of course just an idea, but I tested this and it worked correctly for
> the cases I was testing.
>
> The effect of this patch is that when there is a canonical reader waiting for
> input, it maintains the previous behavior, but when there's no reader (like
> when readline is changing modes), it blocks and doesn't lose any characters.
Apologies for taking so long to reply.
My primary concern is canonical readers not become stuck with a full
read buffer, even with bogus input data (IOW, that an error condition will
not prevent a reader from making forward progress). I believe that won't
happen with this change, but what I really need in this case is a detailed
analysis from you of why that won't happen. That analysis should be in
the patch changelog. (Feel free to send me private mail if you need help
preparing a patch.)
And the patch above has a bug that allows a negative 'left' to be
assigned to tty->receive_room which will be interpreted as a very large
positive value.
This approach still has several drawbacks.
1) Since additional state is reset when the termios is changed by
readline(), the canonical line buffer state will be bogus.
This renders the termios change by readline() pointless; the
caller will not be able to retrieve expected input properly.
2) Since the input data is interpreted with the current termios when
data is received, any embedded control characters will not be
interpreted properly; again, the caller will not be able to retrieve
expected input properly.
> Another approach would be to recalculate the size of canon_data when the mode
> is changed, but this would probably be much more invasive, and awfully less
> efficient since it would imply going through the buffer.
This approach is not possible prior to linux-next since the input worker
thread and the reader thread are not locked out of access to the read buffer
while changing the termios.
And while rescanning the read buffer is possible in linux-next (eg, to
compute the read_flags bitmap indicating the position of NLs), this doesn't
address embedded control characters not being reinterpreted. And completely
reinterpreting the read buffer makes interpreting when receiving pointless.
> What do you think? Is the proposed solution, or something along those lines,
> acceptable?
I'm wondering if this problem might be best addressed on the paste side
instead of the read side. Although this wouldn't be a magic bullet, it
would be easier to control when more paste data is added.
Regards,
Peter Hurley
On Sunday 18 of August 2013, Margarita Manterola wrote:
> Hi,
>
> On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <[email protected]> wrote:
> >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> >> index 4bf0fc0..2ba7f4e 100644
> >> --- a/drivers/tty/n_tty.c
> >> +++ b/drivers/tty/n_tty.c
> >> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> >>
> >> * characters will be beeped.
> >> */
> >>
> >> if (left <= 0)
> >>
> >> - left = ldata->icanon && !ldata->canon_data;
> >> + if (waitqueue_active(&tty->read_wait))
> >> + left = ldata->icanon && !ldata->canon_data;
> >>
> >> old_left = tty->receive_room;
> >> tty->receive_room = left;
> >
> > Was this applied? You may want to cc rjw... it is a regression, it is
> > not pretty, and it is something I blieve I hit but thought it was some
> > kind of "X weirdness".
>
> There were no replies to the previous mail asking for comments, and as
> far as I can see this has not been applied. I don't know who rjw is,
> could you be a bit more explicit, please?
Hi.
Was there some kind of continuation of this thread or the thing died
completly?
--
Arkadiusz Miśkiewicz, arekm / maven.pl
?Hola Peter!
El 2013-08-19 a las 08:25 -0400, Peter Hurley escribi?:
> My primary concern is canonical readers not become stuck with a full
> read buffer, even with bogus input data (IOW, that an error condition will
> not prevent a reader from making forward progress). I believe that won't
> happen with this change, but what I really need in this case is a detailed
> analysis from you of why that won't happen. That analysis should be in
> the patch changelog. (Feel free to send me private mail if you need help
> preparing a patch.)
I'm not sure what level of analysis you are looking for. The driver will block
when there are no readers but as soon as there is a read call it unblocks.
I've added this information to the patch description that I'm including below.
> And the patch above has a bug that allows a negative 'left' to be
> assigned to tty->receive_room which will be interpreted as a very large
> positive value.
Ok, fixed with an else clause. It could also use an extra &&, but it looks a
bit confusing.
> This approach still has several drawbacks.
> 1) Since additional state is reset when the termios is changed by
> readline(), the canonical line buffer state will be bogus.
> This renders the termios change by readline() pointless; the
> caller will not be able to retrieve expected input properly.
> 2) Since the input data is interpreted with the current termios when
> data is received, any embedded control characters will not be
> interpreted properly; again, the caller will not be able to retrieve
> expected input properly.
Indeed this is correct, however this is not an issue of this patch but of the
current interaction between the kernel and readline. In order to fix this, the
reading buffer should always be in raw and only when responding to a read call
for canonical mode should it be interpreted. This is a very big change, and
I'm not sure if anybody will be interested in implementing it.
> >What do you think? Is the proposed solution, or something along those lines,
> >acceptable?
> I'm wondering if this problem might be best addressed on the paste side
> instead of the read side. Although this wouldn't be a magic bullet, it
> would be easier to control when more paste data is added.
I don't see how this could work, could you elaborate?
This is the patch proposal, for comments:
From 81afd3b666cbf94bb9923ebf87fb2017a7cd645e Mon Sep 17 00:00:00 2001
From: Maximiliano Curia <[email protected]>
Date: Tue, 3 Sep 2013 22:48:34 +0200
Subject: [PATCH] Only let characters through when there are active readers.
If there is an active reader, previous behavior is in place. When there is no
active reader, input is blocked until the next read call unblocks it.
This fixes a long standing issue with readline when pasting more than 4096
bytes.
---
drivers/tty/n_tty.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..cdc3b19 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
* pending newlines, let characters through without limit, so
* that erase characters will be handled. Other excess
* characters will be beeped.
+ * If there is no reader waiting for the input, block instead of
+ * letting the characters through.
*/
if (left <= 0)
- left = ldata->icanon && !ldata->canon_data;
+ if (waitqueue_active(&tty->read_wait)) {
+ left = ldata->icanon && !ldata->canon_data;
+ } else {
+ left = 0;
+ }
+
old_left = tty->receive_room;
tty->receive_room = left;
--
1.8.4.rc3
--
"Always code as if the person who ends up maintaining your code is a violent
psychopath who knows where you live."
-- John Woods
Saludos /\/\ /\ >< `/
On 09/03/2013 05:12 PM, Maximiliano Curia wrote:
> ¡Hola Peter!
>
> El 2013-08-19 a las 08:25 -0400, Peter Hurley escribió:
>> My primary concern is canonical readers not become stuck with a full
>> read buffer, even with bogus input data (IOW, that an error condition will
>> not prevent a reader from making forward progress). I believe that won't
>> happen with this change, but what I really need in this case is a detailed
>> analysis from you of why that won't happen. That analysis should be in
>> the patch changelog. (Feel free to send me private mail if you need help
>> preparing a patch.)
>
> I'm not sure what level of analysis you are looking for.
For example, why will CPU 0 (the reader) not hang forever under the following
circumstances?
CPU 0 | CPU 1
|
n_tty_read() | n_tty_receive_buf2()
. | receive_room
. | .
. | is waitqueue_active()? no
add_wait_queue() | .
. | .
is input_available_p()? | .
no - schedule_timeout() <-- reader sleeps
| .
| return 0
| exit i/o loop in flush_to_ldisc() work function
|
A complete and detailed analysis would go a long way to getting this patch
accepted. If you show that your patch won't hang readers _and_ fixes the
larger-than-4k-paste bug, then the readline() deficiencies will probably be
overlooked.
> The driver will block
> when there are no readers but as soon as there is a read call it unblocks.
> I've added this information to the patch description that I'm including below.
>
>> And the patch above has a bug that allows a negative 'left' to be
>> assigned to tty->receive_room which will be interpreted as a very large
>> positive value.
>
> Ok, fixed with an else clause. It could also use an extra &&, but it looks a
> bit confusing.
>
>> This approach still has several drawbacks.
>
>> 1) Since additional state is reset when the termios is changed by
>> readline(), the canonical line buffer state will be bogus.
>> This renders the termios change by readline() pointless; the
>> caller will not be able to retrieve expected input properly.
>
>> 2) Since the input data is interpreted with the current termios when
>> data is received, any embedded control characters will not be
>> interpreted properly; again, the caller will not be able to retrieve
>> expected input properly.
>
> Indeed this is correct, however this is not an issue of this patch but of the
> current interaction between the kernel and readline.
My point here was this: the whole point of readline() flipping termios settings
back and forth is to allow readline() to use one setting while the caller
uses a different setting.
If you don't care that the readline() caller doesn't receives its input
properly when pasted, why is the solution to this problem patching the
linux kernel instead of ripping out the termios-flipping that readline()
does?
> In order to fix this, the
> reading buffer should always be in raw and only when responding to a read call
> for canonical mode should it be interpreted. This is a very big change, and
> I'm not sure if anybody will be interested in implementing it.
So the receiver only echoes input once it's been read??? Because if implemented
as you suggest, until the input has been read you wouldn't know what termios
settings to apply for the echoed chars (or even if to echo).
>>> What do you think? Is the proposed solution, or something along those lines,
>>> acceptable?
>
>> I'm wondering if this problem might be best addressed on the paste side
>> instead of the read side. Although this wouldn't be a magic bullet, it
>> would be easier to control when more paste data is added.
>
> I don't see how this could work, could you elaborate?
Well, the vt driver already supports a PASTE_SELECTION ioctl
which bypasses the flip buffers. Something similar might be suitable
for line-oriented pasting, where only one line is written at a time.
I haven't given too much thought to how/what would perform the wake
up of the paster; right now, the vt driver implements unthrottle to
continue pasting.
> This is the patch proposal, for comments:
>
> From 81afd3b666cbf94bb9923ebf87fb2017a7cd645e Mon Sep 17 00:00:00 2001
> From: Maximiliano Curia <[email protected]>
> Date: Tue, 3 Sep 2013 22:48:34 +0200
> Subject: [PATCH] Only let characters through when there are active readers.
>
> If there is an active reader, previous behavior is in place. When there is no
> active reader, input is blocked until the next read call unblocks it.
>
> This fixes a long standing issue with readline when pasting more than 4096
> bytes.
> ---
> drivers/tty/n_tty.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..cdc3b19 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
> * pending newlines, let characters through without limit, so
> * that erase characters will be handled. Other excess
> * characters will be beeped.
> + * If there is no reader waiting for the input, block instead of
> + * letting the characters through.
> */
> if (left <= 0)
> - left = ldata->icanon && !ldata->canon_data;
> + if (waitqueue_active(&tty->read_wait)) {
> + left = ldata->icanon && !ldata->canon_data;
> + } else {
> + left = 0;
> + }
Style nitpick. Single-line if-else shouldn't have braces.
> +
> old_left = tty->receive_room;
> tty->receive_room = left;
>
>
On Tuesday 03 of September 2013, Arkadiusz Miskiewicz wrote:
> On Sunday 18 of August 2013, Margarita Manterola wrote:
> > Hi,
> >
> > On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <[email protected]> wrote:
> > >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> > >> index 4bf0fc0..2ba7f4e 100644
> > >> --- a/drivers/tty/n_tty.c
> > >> +++ b/drivers/tty/n_tty.c
> > >> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> > >>
> > >> * characters will be beeped.
> > >> */
> > >>
> > >> if (left <= 0)
> > >>
> > >> - left = ldata->icanon && !ldata->canon_data;
> > >> + if (waitqueue_active(&tty->read_wait))
> > >> + left = ldata->icanon && !ldata->canon_data;
> > >>
> > >> old_left = tty->receive_room;
> > >> tty->receive_room = left;
> > >
> > > Was this applied? You may want to cc rjw... it is a regression, it is
> > > not pretty, and it is something I blieve I hit but thought it was some
> > > kind of "X weirdness".
> >
> > There were no replies to the previous mail asking for comments, and as
> > far as I can see this has not been applied. I don't know who rjw is,
> > could you be a bit more explicit, please?
>
Hi
Was just going over bug-readline and lkml archives and found no continuation
of this.
There was a patch proposed but didn't get applied.
https://lkml.org/lkml/2013/8/17/31
Maybe it only needs proper patch submission?
Looking from bug-readline archives debugging it took huge amount of work and
would be sad that all that ended up being wasted.
Whole thread if someone lost it https://lkml.org/lkml/2013/7/25/205
--
Arkadiusz Miśkiewicz, arekm / maven.pl
?Hola Arkadiusz!
El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribi?:
> Was just going over bug-readline and lkml archives and found no continuation
> of this.
> There was a patch proposed but didn't get applied.
> https://lkml.org/lkml/2013/8/17/31
> Maybe it only needs proper patch submission?
The latest patch is: https://lkml.org/lkml/2013/9/3/539
And the latest reply is: https://lkml.org/lkml/2013/9/11/825
Where Peter Hurley suggests that the patch needs a complete and detailed
analysis, but sadly I'm still not sure how to provide it. If anybody is up for
the task, by all means, go ahead.
The patch seems to be applied in the ubuntu kernel without any known issues so
far.
> Looking from bug-readline archives debugging it took huge amount of work and
> would be sad that all that ended up being wasted.
True, but I somehow agree with Peter in that a known bug is better than
applying a fix with unknown consequences, even though I think that
this particular patch won't break anything.
Happy hacking,
--
"If you have too many special cases, you are doing it wrong." -- Craig Zarouni
Saludos /\/\ /\ >< `/
On 10/29/2013 09:50 AM, Maximiliano Curia wrote:
> ¡Hola Arkadiusz!
>
> El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
>> Was just going over bug-readline and lkml archives and found no continuation
>> of this.
>
>> There was a patch proposed but didn't get applied.
>> https://lkml.org/lkml/2013/8/17/31
>> Maybe it only needs proper patch submission?
>
> The latest patch is: https://lkml.org/lkml/2013/9/3/539
>
> And the latest reply is: https://lkml.org/lkml/2013/9/11/825
>
> Where Peter Hurley suggests that the patch needs a complete and detailed
> analysis, but sadly I'm still not sure how to provide it. If anybody is up for
> the task, by all means, go ahead.
The analysis is on my TODO list. I'll include it with a proper patch, this or
next weekend.
Regards,
Peter Hurley
Hi!
> >>How is this different from the unpatched kernel?
> >>In the unpatched kernel, if you happen on reader side
> >>to enable icanon while n_tty received all but VEOF (is this
> >>possible at all?),
> >>then the buffer will be flushed, and the remaining VEOF
> >>will get you a nice EOF.
> >>So, in the unpatched kernel you get EOF because the buffer
> >>gets wiped.
> >
> >???
> >
> >Testcase output from 3.12 w/o patch:
> OK, sorry, after a year of rot of my patch in bugzilla, I've
> completely forgot the pre-conditions, which is that the
> buffer is not discarded, just not pushed.
>
> >Consider the total brute-force approach; a shadow read_flags that
> >distinguishes a real EOF receive from the fake EOF push initiated
> >by the patch.
Was this solved somehow?
Given that it is recent regression, maybe right solution is to do the brute-force patch
now, and worry about effectivity later?
Pavel
28.01.2014 16:03, Pavel Machek пишет:
> Hi!
>
>>>> How is this different from the unpatched kernel?
>>>> In the unpatched kernel, if you happen on reader side
>>>> to enable icanon while n_tty received all but VEOF (is this
>>>> possible at all?),
>>>> then the buffer will be flushed, and the remaining VEOF
>>>> will get you a nice EOF.
>>>> So, in the unpatched kernel you get EOF because the buffer
>>>> gets wiped.
>>> ???
>>>
>>> Testcase output from 3.12 w/o patch:
>> OK, sorry, after a year of rot of my patch in bugzilla, I've
>> completely forgot the pre-conditions, which is that the
>> buffer is not discarded, just not pushed.
>>
>>> Consider the total brute-force approach; a shadow read_flags that
>>> distinguishes a real EOF receive from the fake EOF push initiated
>>> by the patch.
> Was this solved somehow?
Wasn't this already applied?
I think you've missed the part of the discussion thread:
https://groups.google.com/forum/#!msg/linux.kernel/05c-vQUDww4/umXJsD_uiskJ
On 01/28/2014 07:17 AM, Stas Sergeev wrote:
> 28.01.2014 16:03, Pavel Machek пишет:
>> Hi!
>>
>>>>> How is this different from the unpatched kernel?
>>>>> In the unpatched kernel, if you happen on reader side
>>>>> to enable icanon while n_tty received all but VEOF (is this
>>>>> possible at all?),
>>>>> then the buffer will be flushed, and the remaining VEOF
>>>>> will get you a nice EOF.
>>>>> So, in the unpatched kernel you get EOF because the buffer
>>>>> gets wiped.
>>>> ???
>>>>
>>>> Testcase output from 3.12 w/o patch:
>>> OK, sorry, after a year of rot of my patch in bugzilla, I've
>>> completely forgot the pre-conditions, which is that the
>>> buffer is not discarded, just not pushed.
>>>
>>>> Consider the total brute-force approach; a shadow read_flags that
>>>> distinguishes a real EOF receive from the fake EOF push initiated
>>>> by the patch.
>> Was this solved somehow?
> Wasn't this already applied?
Yes, this was applied and is on the mainline master branch,
so will appear in 3.14-rc1.
Regards,
Peter Hurley