apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@apache.org>
Subject New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Date Sat, 06 Jul 2002 09:39:18 GMT
On Fri, Jul 05, 2002 at 08:47:32PM -0400, rbb@apache.org wrote:
> This also creates a support library for APR, this is basically just a
> series of functions that APR can use internally to get the job
> done.  Since wait_for_io_or_timeout was identical between files and
> sockets, I have moved that function to the support lib, it also now uses
> apr_poll().

What's the rationale behind a 'support library' for APR?  All of the
content in newpoll.tar.gz seems like it should just stay in APR.  You
have me thoroughly confused here.  Perhaps in a new directory, but
definitely not a separate library.

I'm also not clear what the additional num parameter to apr_poll is
buying us.  Why not just use the *nsds value on input as your num
parameter?  The number of descriptors read is what *nsds is on the
output.  No signature change required.

Aha, it seems that httpd uses a broken apr_poll() semantic.  If you
read the documentation for apr_poll, it says:

 * @param nsds The number of sockets we are polling. 
...
 * The number of sockets signalled is returned in the second argument (nsds)

But, httpd-2.0 doesn't pass in the number of sockets we are polling.
flood does because I read the docs not the code.  I think we should
switch the code to match the docs.

I know I took it to mean that nsds on input is the number of fds we
are polling and on completion, it is the number of signaled fds.  I
believe that the docs have the right API.

So, why not do something like this (very rough):

(Some OS does the low magic number off the stack, but I can't
remember which for the life of me.)

apr_status_t apr_poll(apr_pollfd_t *aprset, apr_int32_t *nsds,
                      apr_interval_time_t timeout)
{
    int i, rv;
    struct pollfd *pollset;
    struct pollfd stack_pollset[SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6];

    if (*nsds < SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6) {
        pollset = &stack_pollset;
    }
    else {
        pollset = apr_palloc(aprset->p,
                             sizeof(struct pollfd) * *nsds);
    }

    for (i = 0; i < *nsds; i++) {
        if (aprset[i].desc_type == APR_POLL_SOCKET) {
            pollset[i].fd = aprset[i].desc.s->socketdes;
        }
        else if (aprset[i].desc_type == APR_POLL_FILE) {
            pollset[i].fd = aprset[i].desc.f->filedes;
        }
        pollset[i].events = get_event(aprset[i].events);
    }

    if (timeout > 0) {
	/* FIXME: need apr conversion macro from apr_interval_time_t
  	 * to milliseconds.
	 */
        timeout /= 1000; /* convert microseconds to milliseconds */
    }

    rv = poll(pollset, *nsds, timeout);

    if (rv < 0) {
        return errno;
    }

    for (i = 0; i < *nsds; i++) {
        aprset[i].revents = get_revent(pollset[i].revents);
    }

    *nsds = rv;
    return APR_SUCCESS;
}

It seems that you might have originally meant to go down this route
because you have:

struct pollfd *pollset = apr_palloc(aprset->p,
                                    sizeof(struct pollfd) * *nsds);

Which means that the pollset is allocated to contain *nsds
pollfds.  And, I think that means you need to have *nsds to be
equivalent to num, or you will segfault.  (Perhaps it hasn't in
your tests by sheer luck.)  -- justin

Mime
View raw message