Created attachment 96691 [details] the fix If icanon is enabled while the data written in raw mode is still pending, currently it won't be read. The attached patch fixes the problem.
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.