Bug 55991

Thanks for the proposal.

Unfortunately, simulating an EOF push when switching from non-canonical to canonical mode assumes that the input will actually now be read in canonical mode. However, that may not be true.

For example, an application using readline() for all its input will loop, calling readline(). Since readline() switches to non-canonical to perform char-by-char reads and then restores the original termios settings before
returning to the caller, any typeahead or paste read on the next iteration would be corrupted by the '\0'.

Yes, that's not very good.
The result of allowing the patches to hang
for so long, is that I don't remember the
code very well now, and probably have lost the
test-case...
Looking at the code now, I think the patch can
just be changed as follows:
+ set_bit(ldata->read_head, ldata->read_flags);
should be changed to
+ set_bit(ldata->read_head - 1, ldata->read_flags);

and the
+ put_tty_queue(__DISABLED_CHAR, ldata);
can then just be removed. It seems, the code handles
that case very well and will not be confused by
the lack of __DISABLED_CHAR.

I don't know however when I recover the test-case
to test that proposal...

Created attachment 115941 [details]
testcase

Created attachment 115951 [details]
Proposed patch

Let me know if this works for you and I'll submit it to linux-kernel.

Created attachment 116081 [details]
Proposed patch v2

Hello Peter, thanks for taking that!
So it seems the code have changed a lot...
Why does read_head now need &(N_TTY_BUF_SIZE - 1)
everywhere, instead of just keeping it in
0...N_TTY_BUF_SIZE-1 range, as before? Does
this mean it can eventually overflow around size_t
with some data loss in buffer?

While I was typing about the strange logic in the patch,
you already replaced it with V2. :)
I won't be able to test it any soon though, but it looks good.

Acked-by: Stas Sergeev <stsp@users.sourceforge.net>

By the way, can someone please suggest
me the way to flush the icanon data to
the reader without adding EOL?
Doing that with different EOLs (like VEOL/VEOL2)
is inappropriate as they are being passed
to the reader.
VEOF is not being passed to reader, but it
will produce EOF if no data is available...
That's why I started in my program to switch
between the raw and icanon mode, and so is this
patch. But that's hackish. Any clean way to
flush the icanon data?

(In reply to Stas Sergeev from comment #6)
> Hello Peter, thanks for taking that!
> So it seems the code have changed a lot...
> Why does read_head now need &(N_TTY_BUF_SIZE - 1)
> everywhere, instead of just keeping it in
> 0...N_TTY_BUF_SIZE-1 range, as before? Does
> this mean it can eventually overflow around size_t
> with some data loss in buffer?

The read_buf is now shared locklessly between the input
processing worker thread (the receive_buf() method which is a
single producer) and the reader (single consumer).

The unsigned overflow arithmetic allows the shared buffer
indices to be updated without locks.

> While I was typing about the strange logic in the patch,
> you already replaced it with V2. :)

Yep, discovered the sudo breakage readily enough :)

> I won't be able to test it any soon though, but it looks good.
>
> Acked-by: Stas Sergeev <stsp@users.sourceforge.net>

Thanks. You'll be cc'd a copy of the proposed from the mailing
list shortly.

PS - The patch should apply to 3.12 with 'git am -C1 <patch_file_name>'

(In reply to Stas Sergeev from comment #7)
> By the way, can someone please suggest
> me the way to flush the icanon data to
> the reader without adding EOL?
> Doing that with different EOLs (like VEOL/VEOL2)
> is inappropriate as they are being passed
> to the reader.
> VEOF is not being passed to reader, but it
> will produce EOF if no data is available...
> That's why I started in my program to switch
> between the raw and icanon mode, and so is this
> patch. But that's hackish. Any clean way to
> flush the icanon data?

Not that I know of, sorry.

Thanks for CCing me the e-mail.
It recalled me another part of the solution,
which is the bug 55981.
It solves an opposite case: when icanon is
_disabled_ after some data was written but
no newline yet, the reader should be woken
up so the data to became immediately
available for read() in raw mode.
This is what patch 55981 does.

Can we close these as obsolete now the code has been reworked upstream ?

The patch was modified and applied:
https://groups.google.com/forum/#!msg/linux.kernel/05c-vQUDww4/umXJsD_uiskJ
So this is a CODE_FIX.
Would be nice to get some attention to
other tty patches of mine too.