On Sun, Jun 22, 2014 at 9:20 AM, John-Mark Gurney <j...@funkthat.com> wrote:
> sven falempin wrote this message on Tue, Jun 17, 2014 at 07:42 -0400: > > On Mon, Jun 16, 2014 at 10:57 PM, Ted Unangst <t...@tedunangst.com> > wrote: > > > > > On Sat, Jun 14, 2014 at 10:55, Ted Unangst wrote: > > > > On Fri, Jun 13, 2014 at 10:40, sven falempin wrote: > > > >>>> > > > >>>> Now soliciting diffs to change readwrite to a loop with two > buffers > > > >>>> that poll()s in all four directions. :) > > > >>>> > > > >>>> > > > >>> Is using kqueue ok ? > > > >>> > > > >>> like : http://pastebin.com/F1c5Hswi > > > > > > > > uh, maybe. the kqueue changes make it harder to review at first, but > > > > I'll look closer soonish. > > > > > > Turns out pastebin isn't a very good place for storing diffs. It's > > > gone now, making it very hard to review. Can you send the diff to the > > > list? Thanks. > > > > > > > > > Index: netcat.c > > =================================================================== > > RCS file: /cvs/src/usr.bin/nc/netcat.c,v > > retrieving revision 1.121 > > diff -u -p -r1.121 netcat.c > > --- netcat.c 10 Jun 2014 16:35:42 -0000 1.121 > > +++ netcat.c 14 Jun 2014 17:46:31 -0000 > > @@ -36,6 +36,7 @@ > > #include <sys/time.h> > > #include <sys/uio.h> > > #include <sys/un.h> > > +#include <sys/event.h> > > > > #include <netinet/in.h> > > #include <netinet/in_systm.h> > > @@ -734,58 +735,124 @@ readwrite(int nfd) > > { > > struct pollfd pfd[2]; > > pfd can be removed, I don't think you're using it... > > > unsigned char buf[16 * 1024]; > > + struct kevent keventlist[4]; > > + quad_t write_space_network; > > + quad_t write_space_stdout; > > + quad_t available; > > int n, wfd = fileno(stdin); > > int lfd = fileno(stdout); > > int plen; > > + int kq; > > + int evcount; > > + int evid; > > > > plen = sizeof(buf); > > > > - /* Setup Network FD */ > > - pfd[0].fd = nfd; > > - pfd[0].events = POLLIN; > > + /* Set up kqueue*/ > > + kq = kqueue(); > > + if (kq == -1) { > > + close(nfd); > > + err(1, "Kqueue Error"); > > + } > > + > > + /* Set up STDOUT FD. */ > > + EV_SET(&keventlist[0], nfd, EVFILT_WRITE, EV_ADD, 0, 0, NULL); > > + > > + /* Set up Network FD */ > > + EV_SET(&keventlist[1], lfd, EVFILT_WRITE, EV_ADD, 0, 0, NULL); > > + EV_SET(&keventlist[2], nfd, EVFILT_READ, EV_ADD, 0, 0, NULL); > > > > /* Set up STDIN FD. */ > > - pfd[1].fd = wfd; > > - pfd[1].events = POLLIN; > > + EV_SET(&keventlist[3], wfd, EVFILT_READ, EV_ADD, 0, 0, NULL); > > > > - while (pfd[0].fd != -1) { > > + /* Registering events */ > > + evcount = kevent(kq, keventlist, 4, NULL, 0, NULL); > > You should pass the keventlist in to see if any erros happened while > registering.. it'd be bad if there was, but we did tell the user that > IO isn't going to happen... > > > + if (evcount == -1) { > > + close(nfd); > > We should probably close kq here... All the places later in the > function that close nfd, should close kq... > > > + err(1, "Kevent Error"); > > + } > > + > > + while (evcount != -1) { > > if (iflag) > > sleep(iflag); > > - > > - if ((n = poll(pfd, 2 - dflag, timeout)) < 0) { > > + evcount = kevent(kq, NULL, 0, keventlist, 4, NULL); > > + if (evcount == -1) { > > close(nfd); > > - err(1, "Polling Error"); > > + err(1, "Kevent Error"); > > } > > + for (evid = 0; evid < evcount; evid++) { > > + struct kevent event = keventlist[evid]; > > + if (event.flags & EV_ERROR) { > > + close(nfd); > > + err(1, "Kevent Error"); > > + } > > > > - if (n == 0) > > - return; > > - > > - if (pfd[0].revents & POLLIN) { > > - if ((n = read(nfd, buf, plen)) < 0) > > - return; > > - else if (n == 0) { > > - shutdown(nfd, SHUT_RD); > > - pfd[0].fd = -1; > > - pfd[0].events = 0; > > - } else { > > - if (tflag) > > - atelnet(nfd, buf, n); > > - if (atomicio(vwrite, lfd, buf, n) != n) > > + if (event.ident == nfd) { > > + if (event.flags & EV_EOF) { > > You (at least on FreeBSD) can have data along w/ the EOF flag: > It is > possible for EOF to be returned (indicating the con- > nection is gone) while there is still data pending > in the socket buffer. > > So, you should check for data first.. > > Also, you don't check which direction got the EOF, so you might be > shutting down the wrong direction.. > > > + shutdown(nfd, SHUT_RD); > > return; > > Are you sure you want to return here? What if there is more data in > the other direction? > > > + } else { > > + if (event.filter == > EVFILT_WRITE) { > > + write_space_network = > > event.data; > > + } > > + else if (event.filter == > > EVFILT_READ) { > > + available = > MIN(event.data, > > write_space_stdout); > > + available = > MIN(available, > > plen); > > + if (available > 0) { > > + n = read(nfd, > buf, > > available); > > + if (n != > available) > > { > > You should probably raise this as an error like the other cases... > > > + return; > > + } else { > > + n = > > write(lfd, buf, available); > > + if (n != > > available) { > > + > > return; > > + } > > + } > > + > write_space_stdout > > -= available; > > + /* Break to > ignore > > EVFILT_WRITE that could > > + * be in current > > keventlist, after this event > > + * and which is > now > > invalid, because we just > > + * write in the > fd. > > + */ > > + break; > > + } > > + } > > + } > > } > > - } > > - > > - if (!dflag && pfd[1].revents & POLLIN) { > > - if ((n = read(wfd, buf, plen)) < 0) > > - return; > > - else if (n == 0) { > > - if (Nflag) > > - shutdown(nfd, SHUT_WR); > > - pfd[1].fd = -1; > > - pfd[1].events = 0; > > - } else { > > - if (atomicio(vwrite, nfd, buf, n) != n) > > + else if (event.ident == lfd) { > > + if (event.filter == EVFILT_WRITE) { > > + write_space_stdout = event.data; > > + } > > + } > > + else if (event.ident == wfd) { > > + if (event.flags & EV_EOF) { > > + if (Nflag) > > + shutdown(nfd, SHUT_WR); > > return; > > + } else { > > + if (event.filter == EVFILT_READ) > { > > + available = > MIN(event.data, > > write_space_network); > > + available = > MIN(available, > > plen); > > + if (available > 0) { > > + n = read(wfd, > buf, > > available); > > + if (n != > available) > > { > > + return; > > + } else { > > + n = > > write(nfd, buf, available); > > + if (n != > > available) { > > + > > return; > > + } > > + } > > + > write_space_network > > -= available; > > + /* Break to > ignore > > EVFILT_WRITE that could > > + * be in current > > keventlist, after this event > > + * and which is > now > > invalid, because we just > > + * write in the > fd. > > + */ > > + break; > > + } > > + } > > + } > > As this second half of the path is the same as the first, my previous > comments also apply... > > -- > John-Mark Gurney Voice: +1 415 225 5579 > > "All that I will do, has been done, All that I have, has not." > Thank you for the review :-) -- --------------------------------------------------------------------------------------------------------------------- () ascii ribbon campaign - against html e-mail /\
