Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes: > On Sun, 6 Sep 2009, OGAWA Hirofumi wrote: >> >> This is not meaning to object to your patch though, I think we would be >> good to fix pty_space(), not leaving as wrong. With fix it, I guess we >> don't get strange behavior in the near of buffer limit. > > I'd actually rather not make that function any more complicated. > > Just make the rules be very simple: > > - the pty layer has ~64kB buffering, and if you just blindly do a > ->write() op, you can see how many characters you were able to write. > > - before doing a ->write() op, you can ask how many characters you are > guaranteed to be able to write by doing a "->write_room()" call. > > ..and then the bug literally was just that "pty_write()" was confused, and > thought that it should do that "write_room()" thing, which it really > shouldn't ever have done. > > So I really think that the true fix is to just remove the code from > pty_write(), and not do anything more complicated. I'll also commit the > change to write '\r\n' as one single string, because quite frankly, it's > just stupid to do it as two characters, but at that point it's just a > cleanup. But, current write_room() returns almost all wrong value. For example, if we have the 4kb preallocated buffer in some state and used it, ->memory_used will be 4kb even if we are using only a byte actually. I thought it's strange/wrong, even if we removed the pty_space() in pty_write(). >> Also, it seems the non-n_tty path doesn't use tty_write_room() check, >> and instead it just try to write and check written bytes which returned >> by tty->ops->write(). > > .. and I think that's fine. I think write_room() should be used sparingly, > and only by code that cares about being able to fit at least 'n' > characters in the tty buffers. In fact, I think even n_tty would likely in > general be better off without it (and just check the return value), but > because of the stateful character translation (that doesn't actually keep > any state around, it just wants to expand things as it goes along), and > because of historical reasons, we'll just keep it using write_room. As a bit long term solution, I agree. Current code seems to have fragile buffer handling about echoes, \n etc. And yes, perhaps, to avoid write_room() is clean way. But, I felt 64kb (pty_write) vs 8kb (pty_write_room) sounds strange currently. Thanks. -- OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
