On Thu, 2019-02-07 at 08:58 +0700, Robert Elz wrote:
> Date: Wed, 06 Feb 2019 20:29:32 +0100
> From: =?UTF-8?Q?Micha=C5=82_G=C3=B3rny?= <mgorny%gentoo.org@localhost>
> Message-ID: <1549481372.983.14.camel%gentoo.org@localhost>
>
>
> | + } else if (!ISSET(tp->t_state, TS_ISOPEN) &&
> | + !ISSET(tp->t_state, TS_CARR_ON)) {
> | + kn->kn_flags |= EV_EOF;
> | + canread = 1;
> | }
>
> The logic there looks incorrect, though I'd expect it to work
> for your test case. It might always work, and yet still not
> really be correct (lead to readers of the code wondering why,
> and perhaps copying this, elsewhere.)
>
> TS_ISOPEN and TS_CARR_ON are (in this scenario) essentially
> clones of each other. Testing both should be superfluous.
>
> They're both cleared when the slave pts is closed, and as
> long as the master ptc remains open (which it does for your
> test) they're both set again when the pts opens again.
>
> The one time they differ, is when the pts opens with the ptc
> closed (TS_ISOPEN is set, TS_CARR_ON is clear) but that should
> be irrelevant/impossible when you're testing using an open ptc
> fd (ie: we know the ptc is open.)
>
> In ptcread() the read will return EOF (0) if !TS_CARR_ON
> regardless of TS_ISOPEN (unless there is actually data to
> be read). The same is true in ptcpoll()
> where
> revents |= POLLHUP;
> if !TS_CARR_ON, again regardless of TS_ISOPEN.
>
> It will take someone with far more kqueue clue than I have to
> be sure (some pty and tty clues would help too) but I suspect
> that all that might be needed is:
>
> if (!ISSET(tp->t_state, TS_CARR_ON)) {
> kn->kn_flags |= EV_EOF;
> canread = 1;
> }
>
> (no "else" and no TS_ISOPEN test). Testing TS_CARR_ON rather
> that TS_ISOPEN merely for consistency with the other code.
>
> Omitting the "else" is OK only if kqueue can handle the EOF
> return, along with "this much data can be read" in the same
> event. Otherwise the "else" before the "if" should remain.
Thank you. I will update the code as requested.
> ps: please indent the code in the body of the "if" to the same
> level as the code in the "if" that precedes it (the "if (canread)"
> not the one inside its block) (even if the "else" remains).
I'm sorry about this, looks like my vim misfired. My mistake for not
looking at the diff close enough.
--
Best regards,
Michał Górny
Attachment:
signature.asc
Description: This is a digitally signed message part