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."
