Review Request 130117: Fix detection of EOF

[Martin Tobias Holmedahl Sandsmark]

> On May 8, 2017, 7:40 a.m., Oswald Buddenhagen wrote:
> > this looks familiar ... https://phabricator.kde.org/D4975
> > 
> > so assuming the condition is valid, this requires an _extensive_ comment.
> > also, shouldn't this just be unified with the solaris code path?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     No, Solaris has a very different behavior that is described by the already existing comment (needing to "drain" the empty bytes with read(..., 0); etc.). It won't work on Linux.
>     
>     What stuff do you want in the comment? I described how detecting an EOF is supposed to be done (by actually trying to read).
> 
> Oswald Buddenhagen wrote:
>     the part i find interesting is the explanation of the confirmed legit situations where this can occur. judging by the phab diff, there is a change of behavior in the kernel involved. otoh, the bugzilla entry suggests that there is a behavior change in kpty, which doesn't ring a bell in to me. i want that cleared up, because it _smells_.
> 
> Peter Wu wrote:
>     I agree with Oswald, while the fix might hide the issue, the real question is why it happens.
>     
>     My patch tried to explain why it possibly happened, but it was still guesswork and not fully confirmed. This patch does not even explain why the assumption should be removed.
>     
>     If you have time, please try to reach the kernel developers (ideally with a minimal testcase, one which I failed to create), maybe they have an idea.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     I don't have time to spend on this, and to me just getting konsole to work is more important than finding out when the kernel behavior changed. To me the old way to check for EOF seems like something that worked by accident, this patch is the correct way I would implement it with the way the kernel works now (e. g. look at https://github.com/torvalds/linux/blob/v4.11/drivers/tty/n_tty.c#L2179-L2182).

According to the latest comment in bugzilla (https://bugs.kde.org/show_bug.cgi?id=372991#c19) a fix/workaround has been landed in the kernel: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=77dae6134440420bac334581a3ccee94cee1c054

I still think that my patch here is the correct way to detect an EOF however, and it definitely should be more robust in the future, so I'm not going to close this.


- Martin Tobias Holmedahl


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130117/#review103203
-----------------------------------------------------------


On May 6, 2017, 6:09 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130117/
> -----------------------------------------------------------
> 
> (Updated May 6, 2017, 6:09 p.m.)
> 
> 
> Review request for Konsole, Kurt Hindenburg, Oswald Buddenhagen, and Peter Wu.
> 
> 
> Bugs: 372991
>     https://bugs.kde.org/show_bug.cgi?id=372991
> 
> 
> Repository: kpty
> 
> 
> Description
> -------
> 
> Don't just assume that 0 bytes read means EOF.
> 
> 
> Diffs
> -----
> 
>   src/kptydevice.cpp 22233a5 
> 
> Diff: https://git.reviewboard.kde.org/r/130117/diff/
> 
> 
> Testing
> -------
> 
> Fixes the konsole issue from https://phabricator.kde.org/D4975 and https://bugs.kde.org/show_bug.cgi?id=372991 and the autotest works.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20170617/ec844e95/attachment.html>