Re: [Bug #14015] pty regressed again, breaking expect and gcc's testsuite

[OGAWA Hirofumi]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/