<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/130117/">https://git.reviewboard.kde.org/r/130117/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 8th, 2017, 7:40 a.m. UTC, <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this looks familiar ... https://phabricator.kde.org/D4975</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so assuming the condition is valid, this requires an <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">extensive</em> comment.
also, shouldn't this just be unified with the solaris code path?</p></pre>
</blockquote>
<p>On May 8th, 2017, 11:03 a.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What stuff do you want in the comment? I described how detecting an EOF is supposed to be done (by actually trying to read).</p></pre>
</blockquote>
<p>On May 8th, 2017, 8:02 p.m. UTC, <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">smells</em>.</p></pre>
</blockquote>
<p>On May 8th, 2017, 8:33 p.m. UTC, <b>Peter Wu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree with Oswald, while the fix might hide the issue, the real question is why it happens.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On June 3rd, 2017, 6:22 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
<br />
<p>- Martin Tobias Holmedahl</p>
<br />
<p>On May 6th, 2017, 6:09 p.m. UTC, Martin Tobias Holmedahl Sandsmark wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Konsole, Kurt Hindenburg, Oswald Buddenhagen, and Peter Wu.</div>
<div>By Martin Tobias Holmedahl Sandsmark.</div>
<p style="color: grey;"><i>Updated May 6, 2017, 6:09 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=372991">372991</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kpty
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Don't just assume that 0 bytes read means EOF.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Fixes the konsole issue from https://phabricator.kde.org/D4975 and https://bugs.kde.org/show_bug.cgi?id=372991 and the autotest works.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/kptydevice.cpp <span style="color: grey">(22233a5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130117/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>