fix n_tty/pty input/output buffer full and other misc char handling

PATCH In review Joe Peterson 3 participants 16 messages Oct 13, 2008 – Oct 25, 2008 Char and misc drivers

lore

Share & tools

lore

Joe Peterson

author

2008-10-13 06:02 UTC #0 lore raw

PATCH 1/1 fix n_tty/pty input/output buffer full and other misc char handling

Alan,

As promised, here is a patch to apply after the latest combined echo buffer patch I recently sent. It fixes a few things, but most importantly the handling of buffer full tests and cases in which filled buffers in a stopped tty can create a "gridlock" condition in two cases (read patch text for more details). Note that the condition can easily be seen by pasting lots of text into, e.g., "tr 'f' 'F'" after ^S is hit.

One question I have is this: should we also let characters through (so that ^C, ^Q, etc. can have effect) in non-canonical mode? This would allow prevention of the gridlocks that still can be invoked when using stty -icanon in, say, an xterm. For now, I took the conservative route in this patch, but let me know if a more permissive approach is better.

Thanks, Joe

Fix the handling of input characters when the tty buffer is full or nearly full. This includes tests that are done in n_tty_receive_char(), setting of receive_room in n_tty_set_room(), handling of PARMRK, and the logic for the returned value from pty_chars_in_buffer() in specific cases. Also fix process_output_block to detect continuation characters correctly and to handle control characters even when O_OLCUC is enabled.

Problems with the buffer-full tests done in receive_char() caused characters to be lost at times when the buffer(s) filled. Also, these full conditions would often only be detected with echo on, and PARMRK was not accounted for properly in all cases. One symptom of these problems, in addition to lost characters, was early termination from unix commands like tr and cat when ^Q was used to break from a stopped tty with full buffers (note that breaking out was often previously not possible, due to the issues discussed next). Note space is always reserved at the end of the buffer for a newline (or EOF/EOL) in canonical mode.

A major problem also addressed is when a tty receives considerable input and a program produces output after the tty is stopped. In this condition, both input and output buffers will fill (e.g. in the ptys), causing a kind of "gridlock" during which neither buffer can empty. If the user then tries to free the stopped state with ^Q or interrupt the process with ^C, nothing will happen, and there is no way to break out of the condition from within the tty (because the ^C or ^Q character is in a yet-to-be-processed part of the input buffer). This is fixed by expanding the case in which n_tty_set_room() ensures receive_room is not zero, allowing special characters to be handled (before, it only handled erase characters). Note that this patch does not address non-canonical mode, which might be worth considering, depending on whether that would affect other tty types and situations.

Another similar issue addressed is when the tty will stop polling for input due to the buffer hitting the size specified by WAKEUP_CHARS. Again, a ^C or ^Q will be stuck in a future part of the buffer. This gridlock will be eventually broken by enough single input characters to flush blocks through (if that ever does happen), but adjusting the pty_chars_in_buffer() logic similarly to what is done in n_tty_set_room() fixes this.

In addition, this patch causes "bell" (^G) characters (invoked when the input buffer is full) to be immediately output rather than filling the echo buffer This is especially a problem when the tty is stopped and buffers fill, since the bells would not achieve their purpose, and they would flush all at once when the tty was restarted.

Signed-off-by Joe Peterson <[email protected]>

---

drivers/char/n_tty.c +55 ~~-34~~

diff -Nurp a/drivers/char/n_tty.c b/drivers/char/n_tty.c
@@ -113,13 +113,15 @@ static void n_tty_set_room(struct tty_st
 	int	left = N_TTY_BUF_SIZE - tty->read_cnt - 1;
 
 	/*
-	 * 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 we are doing input canonicalization and the tty needs
+	 * to handle signals, xon/off, or erase characters (i.e. no
+	 * pending newlines), let characters through without limit,
+	 * so that these special characters will be handled.
+	 * Other excess characters will be beeped.
 	 */
 	if (left <= 0)
-		left = tty->icanon && !tty->canon_data;
+		left = tty->icanon &&
+		       (L_ISIG(tty) || I_IXON(tty) || !tty->canon_data) ? 1 : 0;
 	tty->receive_room = left;
 }

@@ -339,10 +341,12 @@ static int do_output_char(unsigned char 
 			tty->column--;
 		break;
 	default:
-		if (O_OLCUC(tty))
-			c = toupper(c);
-		if (!iscntrl(c) && !is_continuation(c, tty))
-			tty->column++;
+		if (!iscntrl(c)) {
+			if (O_OLCUC(tty))
+				c = toupper(c);
+			if (!is_continuation(c, tty))
+				tty->column++;
+		}
 		break;
 	}

@@ -414,7 +418,9 @@ static ssize_t process_output_block(stru
 		nr = space;
 
 	for (i = 0, cp = buf; i < nr; i++, cp++) {
-		switch (*cp) {
+		unsigned char c = *cp;
+		
+		switch (c) {
 		case '\n':
 			if (O_ONLRET(tty))
 				tty->column = 0;

@@ -436,10 +442,12 @@ static ssize_t process_output_block(stru
 				tty->column--;
 			break;
 		default:
-			if (O_OLCUC(tty))
-				goto break_out;
-			if (!iscntrl(*cp))
-				tty->column++;
+			if (!iscntrl(c)) {
+				if (O_OLCUC(tty))
+					goto break_out;
+				if (!is_continuation(c, tty))
+					tty->column++;
+			}
 			break;
 		}
 	}

@@ -848,7 +856,7 @@ static void eraser(unsigned char c, stru
 	unsigned long flags;
 
 	if (tty->read_head == tty->canon_head) {
-		/* echo_char_raw('\a', tty); */ /* what do you think? */
+		/* process_output('\a', tty); */ /* what do you think? */
 		return;
 	}
 	if (c == ERASE_CHAR(tty))

@@ -1081,6 +1089,7 @@ static inline void n_tty_receive_parity_
 static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 {
 	unsigned long flags;
+	int parmrk;
 
 	if (tty->raw) {
 		put_tty_queue(c, tty);

@@ -1119,21 +1128,22 @@ static inline void n_tty_receive_char(st
 	 */
 	if (!test_bit(c, tty->process_char_map) || tty->lnext) {
 		tty->lnext = 0;
+		parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
+		if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
+			/* beep if no space */
+			if (L_ECHO(tty))
+				process_output('\a', tty);
+			return;
+		}
 		if (L_ECHO(tty)) {
 			finish_erasing(tty);
-			if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-				/* beep if no space */
-				echo_char_raw('\a', tty);
-				process_echoes(tty);
-				return;
-			}
 			/* Record the column of first canon char. */
 			if (tty->canon_head == tty->read_head)
 				echo_set_canon_col(tty);
 			echo_char(c, tty);
 			process_echoes(tty);
 		}
-		if (I_PARMRK(tty) && c == (unsigned char) '\377')
+		if (parmrk)
 			put_tty_queue(c, tty);
 		put_tty_queue(c, tty);
 		return;

@@ -1225,15 +1235,20 @@ send_signal:
 			return;
 		}
 		if (c == '\n') {
+			if (tty->read_cnt >= N_TTY_BUF_SIZE) {
+				if (L_ECHO(tty))
+					process_output('\a', tty);
+				return;
+			}
 			if (L_ECHO(tty) || L_ECHONL(tty)) {
-				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
-					echo_char_raw('\a', tty);
 				echo_char_raw('\n', tty);
 				process_echoes(tty);
 			}
 			goto handle_newline;
 		}
 		if (c == EOF_CHAR(tty)) {
+			if (tty->read_cnt >= N_TTY_BUF_SIZE)
+				return;
 			if (tty->canon_head != tty->read_head)
 				set_bit(TTY_PUSH, &tty->flags);
 			c = __DISABLED_CHAR;

@@ -1241,12 +1256,17 @@ send_signal:
 		}
 		if ((c == EOL_CHAR(tty)) ||
 		    (c == EOL2_CHAR(tty) && L_IEXTEN(tty))) {
+			parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty))
+				 ? 1 : 0;
+			if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk)) {
+				if (L_ECHO(tty))
+					process_output('\a', tty);
+				return;
+			}
 			/*
 			 * XXX are EOL_CHAR and EOL2_CHAR echoed?!?
 			 */
 			if (L_ECHO(tty)) {
-				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
-					echo_char_raw('\a', tty);
 				/* Record the column of first canon char. */
 				if (tty->canon_head == tty->read_head)
 					echo_set_canon_col(tty);

@@ -1257,7 +1277,7 @@ send_signal:
 			 * XXX does PARMRK doubling happen for
 			 * EOL_CHAR and EOL2_CHAR?
 			 */
-			if (I_PARMRK(tty) && c == (unsigned char) '\377')
+			if (parmrk)
 				put_tty_queue(c, tty);
 
 handle_newline:

@@ -1274,14 +1294,15 @@ handle_newline:
 		}
 	}
 
+	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
+	if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
+		/* beep if no space */
+		if (L_ECHO(tty))
+			process_output('\a', tty);
+		return;
+	}
 	if (L_ECHO(tty)) {
 		finish_erasing(tty);
-		if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-			/* beep if no space */
-			echo_char_raw('\a', tty);
-			process_echoes(tty);
-			return;
-		}
 		if (c == '\n')
 			echo_char_raw('\n', tty);
 		else {

@@ -1293,7 +1314,7 @@ handle_newline:
 		process_echoes(tty);
 	}
 
-	if (I_PARMRK(tty) && c == (unsigned char) '\377')
+	if (parmrk)
 		put_tty_queue(c, tty);
 
 	put_tty_queue(c, tty);

drivers/char/pty.c +29 ~~-17~~

60 lines not on this page · raw message

Alan Cox

2008-10-13 08:41 UTC #1 lore raw

Quoted 4 lines · On Mon, 13 Oct 2008 00:01:52 -0600 Joe Peterson <[email protected]> wrote:

Alan,
As promised, here is a patch to apply after the latest combined echo buffer patch I recently sent. It fixes a few things, but most

Thanks - queued. Will start looking at tty stuff once the current pile is merged.

Alan

Alan Cox

2008-10-22 09:33 UTC #2 lore raw

In reply to Joe Peterson

Quoted 4 lines

that ^C, ^Q, etc. can have effect) in non-canonical mode? This would allow prevention of the gridlocks that still can be invoked when using stty -icanon in, say, an xterm. For now, I took the conservative route in this patch, but let me know if a more permissive approach is better.

I suspect having thought about this a bit more that the proper logic is in fact

	if (special case a)
	if (special case b)
	if (....)

	/* An ordinary character for the queue */
	if (queue_full) { ....}

and we should process everything that may have a special effect (flow control, delete line, etc) before worrying about having room to store the character whatever the tty ldisc state

Joe Peterson

author

2008-10-22 19:35 UTC #3 lore raw

Quoted 15 lines · Alan Cox

Quoted 4 lines
that ^C, ^Q, etc. can have effect) in non-canonical mode? This would allow prevention of the gridlocks that still can be invoked when using stty -icanon in, say, an xterm. For now, I took the conservative route in this patch, but let me know if a more permissive approach is better.
I suspect having thought about this a bit more that the proper logic is in fact
	if (special case a)
	if (special case b)
	if (....)

	/* An ordinary character for the queue */
	if (queue_full) { ....}

Quoted 3 lines

and we should process everything that may have a special effect (flow control, delete line, etc) before worrying about having room to store the character whatever the tty ldisc state

Yeah, I had thought about this a little before too. The tricky part is knowing that such a character exists and is waiting when we have already stopped accepting characters. In the current way things work, a ^C, for example, will not make it into the line discipline until the stuff that came before has been processed (which is now wedged waiting for space to free up), so it's hard to check in the ldisc for it. I'm guessing that is why the orignal logic in set_room was designed to lets stuff flow in in that case when we're looking for an erase char...

Now, in the case of a ^C or ^Q (e.g.), if we knew one was back in the queue waiting to get to the ldisc, we could put it through out of sequence, but for an erase (e.g.), it needs to wait for what real chars come before it (i.e. it cannot jump to the front)...

I'll think more on this, too - let me know if I am not hearing what you are saying exactly.

-Joe

Joe Peterson

author

2008-10-23 01:33 UTC #4 lore raw

In reply to Alan Cox

Alan,

I have separated this patch into its separate components, since I know we are still thinking about how to address the pty "gridlock" issue, and at least two of the other issues addressed can probably be fixed now.

I will shortly send out 4 new patches that replace the original combined "tty-fix-buffer-full-handling.patch" patch (these all will apply, in order, on top of your current tty git tree):

1) n-tty-fix-cont-and-ctrl-output.patch
2) n-tty-fix-buffer-full-checks.patch

These two can probably be applied and tested now, since I believe these are straightforward fixes. #1 simply fixes what looks like an oversight in the output block processing WRT continuation chars and ctrl chars. #2 fixes what appear to be problems that cause loss of characters because of errors in handling situations when the input buffer is full or nearly so.

3) n-tty-output-bells-immediately.patch
4) n-tty-fix-buffer-full-gridlock.patch

These are the ones that we are still discussing.

Hope this splitting up of the patch helps!

-Joe

Joe Peterson

author

2008-10-23 01:34 UTC #5 lore raw

PATCH 1/1 n-tty-fix-cont-and-ctrl-output

In reply to Alan Cox

Fix process_output_block to detect continuation characters correctly and to handle control characters even when O_OLCUC is enabled. Make similar change to do_output_char().

Signed-off-by Joe Peterson <[email protected]>

---

drivers/char/n_tty.c +15 -9

diff -Nurp a/drivers/char/n_tty.c b/drivers/char/n_tty.c
@@ -351,10 +351,12 @@ static int do_output_char(unsigned char 
 			tty->column--;
 		break;
 	default:
-		if (O_OLCUC(tty))
-			c = toupper(c);
-		if (!iscntrl(c) && !is_continuation(c, tty))
-			tty->column++;
+		if (!iscntrl(c)) {
+			if (O_OLCUC(tty))
+				c = toupper(c);
+			if (!is_continuation(c, tty))
+				tty->column++;
+		}
 		break;
 	}

@@ -425,7 +427,9 @@ static ssize_t process_output_block(stru
 		nr = space;
 
 	for (i = 0, cp = buf; i < nr; i++, cp++) {
-		switch (*cp) {
+		unsigned char c = *cp;
+		
+		switch (c) {
 		case '\n':
 			if (O_ONLRET(tty))
 				tty->column = 0;

@@ -447,10 +451,12 @@ static ssize_t process_output_block(stru
 				tty->column--;
 			break;
 		default:
-			if (O_OLCUC(tty))
-				goto break_out;
-			if (!iscntrl(*cp))
-				tty->column++;
+			if (!iscntrl(c)) {
+				if (O_OLCUC(tty))
+					goto break_out;
+				if (!is_continuation(c, tty))
+					tty->column++;
+			}
 			break;
 		}
 	}

Alan Cox

2008-10-24 08:58 UTC #6 lore raw

Re: [PATCH] n-tty-fix-cont-and-ctrl-output

Quoted 3 lines · On Wed, 22 Oct 2008 19:34:32 -0600 Joe Peterson <[email protected]> wrote:

Fix process_output_block to detect continuation characters correctly and to handle control characters even when O_OLCUC is enabled. Make similar change to do_output_char().

If I have a upper case character as my control code then OLCUC should upper case the printed result..

Joe Peterson

author

2008-10-23 01:35 UTC #7 lore raw

PATCH 1/1 n-tty-fix-buffer-full-checks

In reply to Alan Cox

Fix the handling of input characters when the tty buffer is full or nearly full. This includes tests that are done in n_tty_receive_char() and handling of PARMRK.

Problems with the buffer-full tests done in receive_char() caused characters to be lost at times when the buffer(s) filled. Also, these full conditions would often only be detected with echo on, and PARMRK was not accounted for properly in all cases. One symptom of these problems, in addition to lost characters, was early termination from unix commands like tr and cat when ^Q was used to break from a stopped tty with full buffers (note that breaking out was often previously not possible, due to the pty getting in "gridlock", which will be addressed in another patch). Note space is always reserved at the end of the buffer for a newline (or EOF/EOL) in canonical mode.

Signed-off-by Joe Peterson <[email protected]>

---

drivers/char/n_tty.c +38 ~~-17~~

diff -Nurp a/drivers/char/n_tty.c b/drivers/char/n_tty.c
@@ -1107,6 +1107,7 @@ static inline void n_tty_receive_parity_
 static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 {
 	unsigned long flags;
+	int parmrk;
 
 	if (tty->raw) {
 		put_tty_queue(c, tty);

@@ -1144,21 +1145,24 @@ static inline void n_tty_receive_char(st
 	 */
 	if (!test_bit(c, tty->process_char_map) || tty->lnext) {
 		tty->lnext = 0;
-		if (L_ECHO(tty)) {
-			finish_erasing(tty);
-			if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-				/* beep if no space */
+		parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
+		if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
+			/* beep if no space */
+			if (L_ECHO(tty)) {
 				echo_char_raw('\a', tty);
 				process_echoes(tty);
-				return;
 			}
+			return;
+		}
+		if (L_ECHO(tty)) {
+			finish_erasing(tty);
 			/* Record the column of first canon char. */
 			if (tty->canon_head == tty->read_head)
 				echo_set_canon_col(tty);
 			echo_char(c, tty);
 			process_echoes(tty);
 		}
-		if (I_PARMRK(tty) && c == (unsigned char) '\377')
+		if (parmrk)
 			put_tty_queue(c, tty);
 		put_tty_queue(c, tty);
 		return;

@@ -1250,15 +1254,22 @@ send_signal:
 			return;
 		}
 		if (c == '\n') {
-			if (L_ECHO(tty) || L_ECHONL(tty)) {
-				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
+			if (tty->read_cnt >= N_TTY_BUF_SIZE) {
+				if (L_ECHO(tty)) {
 					echo_char_raw('\a', tty);
+					process_echoes(tty);
+				}
+				return;
+			}
+			if (L_ECHO(tty) || L_ECHONL(tty)) {
 				echo_char_raw('\n', tty);
 				process_echoes(tty);
 			}
 			goto handle_newline;
 		}
 		if (c == EOF_CHAR(tty)) {
+			if (tty->read_cnt >= N_TTY_BUF_SIZE)
+				return;
 			if (tty->canon_head != tty->read_head)
 				set_bit(TTY_PUSH, &tty->flags);
 			c = __DISABLED_CHAR;

@@ -1266,12 +1277,19 @@ send_signal:
 		}
 		if ((c == EOL_CHAR(tty)) ||
 		    (c == EOL2_CHAR(tty) && L_IEXTEN(tty))) {
+			parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty))
+				 ? 1 : 0;
+			if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk)) {
+				if (L_ECHO(tty)) {
+					echo_char_raw('\a', tty);
+					process_echoes(tty);
+				}
+				return;
+			}
 			/*
 			 * XXX are EOL_CHAR and EOL2_CHAR echoed?!?
 			 */
 			if (L_ECHO(tty)) {
-				if (tty->read_cnt >= N_TTY_BUF_SIZE-1)
-					echo_char_raw('\a', tty);
 				/* Record the column of first canon char. */
 				if (tty->canon_head == tty->read_head)
 					echo_set_canon_col(tty);

@@ -1282,7 +1300,7 @@ send_signal:
 			 * XXX does PARMRK doubling happen for
 			 * EOL_CHAR and EOL2_CHAR?
 			 */
-			if (I_PARMRK(tty) && c == (unsigned char) '\377')
+			if (parmrk)
 				put_tty_queue(c, tty);
 
 handle_newline:

@@ -1299,14 +1317,17 @@ handle_newline:
 		}
 	}
 
-	if (L_ECHO(tty)) {
-		finish_erasing(tty);
-		if (tty->read_cnt >= N_TTY_BUF_SIZE-1) {
-			/* beep if no space */
+	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
+	if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
+		/* beep if no space */
+		if (L_ECHO(tty)) {
 			echo_char_raw('\a', tty);
 			process_echoes(tty);
-			return;
 		}
+		return;
+	}
+	if (L_ECHO(tty)) {
+		finish_erasing(tty);
 		if (c == '\n')
 			echo_char_raw('\n', tty);
 		else {

@@ -1318,7 +1339,7 @@ handle_newline:
 		process_echoes(tty);
 	}
 
-	if (I_PARMRK(tty) && c == (unsigned char) '\377')
+	if (parmrk)
 		put_tty_queue(c, tty);
 
 	put_tty_queue(c, tty);

Joe Peterson

author

2008-10-23 01:36 UTC #8 lore raw

PATCH 1/1 n-tty-output-bells-immediately

In reply to Alan Cox

This patch causes "bell" (^G) characters (invoked when the input buffer is full) to be immediately output rather than filling the echo buffer.

This is especially a problem when the tty is stopped and buffers fill, since the bells do not serve their purpose of immediate notification that the buffer cannot take further input, and they will flush all at once when the tty is restarted.

Signed-off-by Joe Peterson <[email protected]>

---

drivers/char/n_tty.c +9 ~~-17~~

diff -Nurp b/drivers/char/n_tty.c c/drivers/char/n_tty.c
@@ -872,7 +872,7 @@ static void eraser(unsigned char c, stru
 
 	/* FIXME: locking needed ? */
 	if (tty->read_head == tty->canon_head) {
-		/* echo_char_raw('\a', tty); */ /* what do you think? */
+		/* process_output('\a', tty); */ /* what do you think? */
 		return;
 	}
 	if (c == ERASE_CHAR(tty))

@@ -1148,10 +1148,8 @@ static inline void n_tty_receive_char(st
 		parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
 		if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
 			/* beep if no space */
-			if (L_ECHO(tty)) {
-				echo_char_raw('\a', tty);
-				process_echoes(tty);
-			}
+			if (L_ECHO(tty))
+				process_output('\a', tty);
 			return;
 		}
 		if (L_ECHO(tty)) {

@@ -1255,10 +1253,8 @@ send_signal:
 		}
 		if (c == '\n') {
 			if (tty->read_cnt >= N_TTY_BUF_SIZE) {
-				if (L_ECHO(tty)) {
-					echo_char_raw('\a', tty);
-					process_echoes(tty);
-				}
+				if (L_ECHO(tty))
+					process_output('\a', tty);
 				return;
 			}
 			if (L_ECHO(tty) || L_ECHONL(tty)) {

@@ -1280,10 +1276,8 @@ send_signal:
 			parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty))
 				 ? 1 : 0;
 			if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk)) {
-				if (L_ECHO(tty)) {
-					echo_char_raw('\a', tty);
-					process_echoes(tty);
-				}
+				if (L_ECHO(tty))
+					process_output('\a', tty);
 				return;
 			}
 			/*

@@ -1320,10 +1314,8 @@ handle_newline:
 	parmrk = (c == (unsigned char) '\377' && I_PARMRK(tty)) ? 1 : 0;
 	if (tty->read_cnt >= (N_TTY_BUF_SIZE - parmrk - 1)) {
 		/* beep if no space */
-		if (L_ECHO(tty)) {
-			echo_char_raw('\a', tty);
-			process_echoes(tty);
-		}
+		if (L_ECHO(tty))
+			process_output('\a', tty);
 		return;
 	}
 	if (L_ECHO(tty)) {

Alan Cox

2008-10-24 09:10 UTC #9 lore raw

Re: [PATCH] n-tty-output-bells-immediately

Ok without randomly hitting send this time...

LC/UC and control characters looks right. (except the added white space) Buffer sizing looks fine
Bell definitely a good idea

Those three applied with better subject lines and queued for .29

The last one I'm rather less happy with but want to ponder a bit more the best way to handle.

Alan

Joe Peterson

author

2008-10-25 15:41 UTC #10 lore raw

Re: [PATCH] n-tty-output-bells-immediately

Quoted 10 lines · Alan Cox

Ok without randomly hitting send this time...
LC/UC and control characters looks right. (except the added white space) Buffer sizing looks fine
Bell definitely a good idea
Those three applied with better subject lines and queued for .29
The last one I'm rather less happy with but want to ponder a bit more the best way to handle.

All sounds good - thanks. Yes, let's think a bit more about the last one. The problem, as I see it now, is that only the ldisc knows how to treat certain chars, but the input buffers get clogged before they get to the ldisc. Perhaps devising a way for upstream input code to call the ldisc to inform it that there is a "signal pending/queued" (i.e. SIGINT, SIGQUIT, SIGSTP) or a tty restart (e.g. ^Q) queued...

BTW, where are the patches sitting at this point? I saw an email about "linux-next" that lists just the first of the 3 (the drop bkl one); and your tty git is now empty. Just curious.

Thanks! Joe

Alan Cox

2008-10-25 15:48 UTC #11 lore raw

Re: [PATCH] n-tty-output-bells-immediately

Quoted 3 lines

BTW, where are the patches sitting at this point? I saw an email about "linux-next" that lists just the first of the 3 (the drop bkl one); and your tty git is now empty. Just curious.

Stephen asked that there was nothing being picked up for next that wasn't .28 material for the moment.

Alan

Joe Peterson

author

2008-10-25 15:53 UTC #12 lore raw

Re: [PATCH] n-tty-output-bells-immediately

Quoted 6 lines · Alan Cox

Quoted 3 lines
BTW, where are the patches sitting at this point? I saw an email about "linux-next" that lists just the first of the 3 (the drop bkl one); and your tty git is now empty. Just curious.
Stephen asked that there was nothing being picked up for next that wasn't .28 material for the moment.

Oh, ok, so the echo/bkl-drop one is in .28, and the other 2 in .29? Sounds good. Do you just queue them yourself, or are they in some other git now?

-Joe

Joe Peterson

author

2008-10-25 21:13 UTC #13 lore raw

Re: [PATCH] n-tty-output-bells-immediately

Quoted 1 line · Joe Peterson

Oh, ok, so the echo/bkl-drop one is in .28, and the other 2 in .29?

                                                            |
       ------------------------------------------------------

Ugh.

I must not have had enough coffee when I wrote this email. Never mind! :)

-Joe

Joe Peterson

author

2008-10-23 01:36 UTC #14 lore raw

PATCH 1/1 n-tty-fix-buffer-full-gridlock

In reply to Alan Cox

Fix possible gridlocks in ptys/ttys when input and output happens in stopped state.

One major problem addressed here is when a tty receives considerable input and a program produces output after the tty is stopped. In this condition, both input and output buffers will fill (e.g. in the ptys), causing a kind of "gridlock" during which neither buffer can empty. If the user then tries to free the stopped state with ^Q or interrupt the process with ^C, nothing will happen, and there is no way to break out of the condition from within the tty (because the ^C or ^Q character is in a yet-to-be-processed part of the input buffer). This is fixed by expanding the case in which n_tty_set_room() ensures receive_room is not zero, allowing special characters to be handled (before, it only handled erase characters). Note that this patch does not address non-canonical mode, which might be worth considering, depending on whether that would affect other tty types and situations.

Another similar issue addressed is when the tty will stop polling for input due to the buffer hitting the size specified by WAKEUP_CHARS. Again, a ^C or ^Q will be stuck in a future part of the buffer. This gridlock will be eventually broken by enough single input characters to flush blocks through (if that ever does happen), but this is fixed by adjusting the n_tty_poll() logic similarly to what is done in n_tty_set_room(), except we are on the master side of a pty in this case (and we check the slave).

Signed-off-by Joe Peterson <[email protected]>

---

drivers/char/n_tty.c +15 -6

diff -Nurp a/drivers/char/n_tty.c b/drivers/char/n_tty.c
@@ -114,13 +114,15 @@ static void n_tty_set_room(struct tty_st
 	int	left = N_TTY_BUF_SIZE - tty->read_cnt - 1;
 
 	/*
-	 * 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 we are doing input canonicalization and the tty needs
+	 * to handle signals, xon/off, or erase characters (i.e. no
+	 * pending newlines), let characters through without limit,
+	 * so that these special characters will be handled.
+	 * Other excess characters will be beeped.
 	 */
 	if (left <= 0)
-		left = tty->icanon && !tty->canon_data;
+		left = tty->icanon &&
+		       (L_ISIG(tty) || I_IXON(tty) || !tty->canon_data) ? 1 : 0;
 	tty->receive_room = left;
 }

@@ -2053,7 +2055,14 @@ static unsigned int n_tty_poll(struct tt
 			tty->minimum_to_wake = 1;
 	}
 	if (tty->ops->write && !tty_is_writelocked(tty) &&
-			tty_chars_in_buffer(tty) < WAKEUP_CHARS &&
+			((tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+			  tty->driver->subtype == PTY_TYPE_MASTER &&
+			  tty->link &&
+			  tty->link->icanon &&
+			  (L_ISIG(tty->link) ||
+			   I_IXON(tty->link) ||
+			   !tty->link->canon_data)) ||
+			 tty_chars_in_buffer(tty) < WAKEUP_CHARS) &&
 			tty_write_room(tty) > 0)
 		mask |= POLLOUT | POLLWRNORM;
 	return mask;

Andrew Morton

2008-10-23 01:56 UTC #15 lore raw

Re: [PATCH] n-tty-fix-buffer-full-gridlock

Quoted 1 line · On Wed, 22 Oct 2008 19:36:46 -0600 Joe Peterson <[email protected]> wrote:

Subject: [PATCH] n-tty-fix-buffer-full-gridlock

Can we stop doing this please? Documentation/SubmittingPatches section 15 has help.