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?= <[email protected]> > Message-ID: <[email protected]> > > > | + } 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
signature.asc
Description: This is a digitally signed message part
