Re: svn commit: r647540 - in /apr/apr/trunk: CHANGES include/apr_poll.h poll/unix/epoll.c poll/unix/kqueue.c poll/unix/poll.c poll/unix/port.c poll/unix/select.c test/testpoll.c

[Ruediger Pluem <rp...@apache.org>]

On 04/13/2008 10:31 AM, mturk@apache.org wrote:
> Author: mturk
> Date: Sun Apr 13 01:31:03 2008
> New Revision: 647540
> 
> URL: http://svn.apache.org/viewvc?rev=647540&view=rev
> Log:
> Introduce apr_pollset_wakeup()
> 
> Modified:
>     apr/apr/trunk/CHANGES
>     apr/apr/trunk/include/apr_poll.h
>     apr/apr/trunk/poll/unix/epoll.c
>     apr/apr/trunk/poll/unix/kqueue.c
>     apr/apr/trunk/poll/unix/poll.c
>     apr/apr/trunk/poll/unix/port.c
>     apr/apr/trunk/poll/unix/select.c
>     apr/apr/trunk/test/testpoll.c
> 
> Modified: apr/apr/trunk/CHANGES

> Modified: apr/apr/trunk/poll/unix/poll.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/poll/unix/poll.c?rev=647540&r1=647539&r2=647540&view=diff
> ==============================================================================
> --- apr/apr/trunk/poll/unix/poll.c (original)
> +++ apr/apr/trunk/poll/unix/poll.c Sun Apr 13 01:31:03 2008

> @@ -242,32 +328,69 @@
>                                             apr_int32_t *num,
>                                             const apr_pollfd_t **descriptors)
>  {
> -    int rv;
> +    int ret;
> +    apr_status_t rv = APR_SUCCESS;
>      apr_uint32_t i, j;
>  
>      if (timeout > 0) {
>          timeout /= 1000;
>      }
> -    rv = poll(pollset->pollset, pollset->nelts, timeout);
> -    (*num) = rv;
> -    if (rv < 0) {
> +    ret = poll(pollset->pollset, pollset->nelts, timeout);
> +    (*num) = ret;
> +    if (ret < 0) {
>          return apr_get_netos_error();
>      }
> -    if (rv == 0) {
> +    else if (res == 0) {

Shouldn't this be

  else if (ret == 0) {

instead?
And why do we need else? We have a return above.



>          return APR_TIMEUP;
>      }
> -    j = 0;
> -    for (i = 0; i < pollset->nelts; i++) {
> -        if (pollset->pollset[i].revents != 0) {
> -            pollset->result_set[j] = pollset->query_set[i];
> -            pollset->result_set[j].rtnevents =
> -                get_revent(pollset->pollset[i].revents);
> -            j++;
> +    else {

Why do we need else here? We do a return above.

> +        for (i = 0, j = 0; i < pollset->nelts; i++) {
> +            if (pollset->pollset[i].revents != 0) {
> +#if APR_HAS_THREADS
> +                /* Check if the polled descriptor is our
> +                 * wakeup pipe. In that case do not put it result set.
> +                 */
> +                if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
> +                    pollset->pollset[i].desc_type == APR_POLL_FILE &&
> +                    pollset->pollset[i].desc.f == pollset->wakeup_pipe[0]) {
> +                        drain_wakeup_pipe(pollset);
> +                        /* XXX: Is this a correct return value ?
> +                         * We might simply return APR_SUCEESS.
> +                         */
> +                        rv = APR_EINTR;

What is if we have at least two events:

Some from "regular" fd(s) and one from our wakeup pipe?

Shouldn't we ignore the fact that the wakeup pipe produced an event in this
case and just proceed as this never happened and just return the results
of the regular fd(s)?
Of course the same applies to the epoll implementation.


For sake of completeness, don't you need to adjust the OS/2 poll routines
as well?

Regards

Rüdiger