From konsole-devel Mon May 08 20:02:58 2017 From: Oswald Buddenhagen Date: Mon, 08 May 2017 20:02:58 +0000 To: konsole-devel Subject: Re: Review Request 130117: Fix detection of EOF Message-Id: <20170508200258.25551.35744 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=konsole-devel&m=149427379728464 --===============7931389087189267196== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On May 8, 2017, 9: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). 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_. - Oswald ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130117/#review103203 ----------------------------------------------------------- On May 6, 2017, 8: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, 8: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 > > --===============7931389087189267196== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130117/

On May 8th, 2017, 9:40 a.m. CEST, 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?

On May 8th, 2017, 1:03 p.m. CEST, 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).
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.

- Oswald


On May 6th, 2017, 8:09 p.m. CEST, Martin Tobias Holmedahl Sandsmark wrote:

Review request for Konsole, Kurt Hindenburg, Oswald Buddenhagen, and Peter Wu.
By Martin Tobias Holmedahl Sandsmark.

Updated May 6, 2017, 8:09 p.m.

Bugs: 372991
Repository: kpty

Description 

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

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.

Diffs

  • src/kptydevice.cpp (22233a5)

View Diff
--===============7931389087189267196==--