On Wed, 2 Dec 2009, Erik Faye-Lund wrote:
> 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.
> 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:
> --->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.
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html