Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

From: Martin Storsjö <martin@martin.st>
Date: Dec 2, 2009, 13:21 UTC
Message-ID: <alpine.DEB.2.00.0912021511310.5582@cone.home.martin.st>
In-Reply-To: <40aa078e0912020501v9378c37l106e1e23b5e7b43d@mail.gmail.com>

On Wed, 2 Dec 2009, Erik Faye-Lund wrote:

Show 10 quoted lines

> I'm not very familiar with poll(), but if I understand the man-pages
> correctly it's waiting for events on file descriptors, and is in our
> case used to check for incoming connections, right? If so, I see three
> possible ways forward: (1) extending our poll()-emulation to handle
> multiple sockets, (2) change daemon.c to check one socket at the time,
> and (3) using select() instead of poll().
> 
> (1) seems like the "correct" but tricky thing to do, (2) like the
> "easy" but nasty thing to do. However, (3) strikes me as the least
> dangerous thing to do ;)

Generally, poll is a better API than select, since select is limited to fd values up to (about) 1000, depending on the implementation. (This is due to the fact that fd_set is a fixed size bit set with a bit for each possible fd.)

But since we're only doing select on the handful of sockets that were opened initially in the process (so these should receive low numbers), this should only be a theoretical concern.

Show 6 quoted lines

> For (1), there's also a WSAPoll() function in Windows, but I'm not
> sure how to figure out if an fd is a socket or a pipe. There's also
> WaitForMultipleObjects.
> 
> Here's a quick stab at (3). It seems to work fine under Windows. Not
> tested on other platforms, though.

A few comments below, just by reading through this, didn't test it yet:

Show 34 quoted lines

> --->8---
> --- a/daemon.c
> +++ b/daemon.c
> @@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i
> nt **socklist_p)
> 
>  static int service_loop(int socknum, int *socklist)
>  {
> -       struct pollfd *pfd;
> -       int i;
> -
> -       pfd = xcalloc(socknum, sizeof(struct pollfd));
> -
> -       for (i = 0; i < socknum; i++) {
> -               pfd[i].fd = socklist[i];
> -               pfd[i].events = POLLIN;
> -       }
> -
>         signal(SIGCHLD, child_handler);
> 
>         for (;;) {
>                 int i;
> +               fd_set fds;
> +               struct timeval timeout = { 0, 0 };
> 
>                 check_dead_children();
> 
> -               if (poll(pfd, socknum, -1) < 0) {
> +               FD_ZERO(&fds);
> +               for (i = 0; i < socknum; i++)
> +                       FD_SET(socklist[i], &fds);
> +
> +               if (select(0, &fds, NULL, NULL, &timeout) > 0) {
>                         if (errno != EINTR) {

The first parameter to select should be max(all fds set in the fd_set) + 1, this should be trivial enough to determine in the loop above where the fd:s are added with FD_SET.

You're calling select() with a zero timeout - I'd guess this chews quite a bit of cpu just looping around doing nothing? If the last parameter would be set to NULL, it would wait indefinitely, just like the previous poll loop did.

Show 6 quoted lines

> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
>                                         }
>                                 }
>                                 handle(incoming, (struct sockaddr *)&ss, sslen);
> 
> +                               break;

What's this good for?

Other than that, this looks quite good to me, at a first glance.

// Martin