Fix detection of EOF

Review Request #130117 - Created May 6, 2017 and updated

Information
Martin Tobias Holmedahl Sandsmark
kpty
master
372991
Reviewers
konsole
hindenburg, ossi, peterwu

Don't just assume that 0 bytes read means EOF.

Fixes the konsole issue from https://phabricator.kde.org/D4975 and https://bugs.kde.org/show_bug.cgi?id=372991 and the autotest works.

Oswald Buddenhagen

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?

  1. 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).

  2. 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.

  3. 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.

  4. 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).

  5. 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.

Loading...