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

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Wed, Dec 2, 2009 at 2:21 PM, Martin Storsjö <martin@martin.st> wrote:
> 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.
>

Yeah. In our case, I guess it's a maximum of two times the number of
network adapters installed, so I think we should be relatively safe.

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

Yeah, thanks for pointing out. I did a little bit of searching, and it
seems like "most other people passes zero, and it just works".
However, we should be nice to systems where it doesn't "just work", so
I'll fix this before sending it out.

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

Ah, yes. That's much nicer!

>> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
>>                                         }
>>                                 }
>>                                 handle(incoming, (struct sockaddr *)&ss, sslen);
>>
>> +                               break;
>
> What's this good for?
>

When I clone git://localhost/some-repo, select() returns a fd-set with
both the IPv4 and IPv6 fds. After accept()'ing the first one, the
second call to accept() hangs. I solved this by accepting only the
first connection I got; the second one should be accepted in the next
round of the service loop (if still available).

-- 
Erik "kusma" Faye-Lund