[PATCH] serf pollset confusion

Looking at serf_context_run() I see

    while (num--) {
        serf_connection_t *conn = desc->client_data;

        status = serf_event_trigger(ctx, conn, desc);
        if (status) {
            return status;
        }

        desc++;
    }

When I look at conn[0] gdb it is junk. Looking at serf_event_trigger()
it becomes obvious that desc->client_data is not a serf_connection_t* it
is a serf_io_baton_t* which contains a serf_connection_t* at an non-zero
offset. If I look at ((serf_io_baton_t*)conn)[0].u.conn then I see a
valid data. The code is wrong but it doesn't cause any runtime problem
since the "wrong" pointer is just passed through and gets cast to the
right type. We should fix it, the void* client_data could be passed
directly to serf_event_trigger, or the type could be corrected as:

Index: context.c
===================================================================
--- context.c (revision 2445)
+++ context.c (working copy)
@@ -295,9 +295,9 @@ apr_status_t serf_context_run(
     }
 
     while (num--) {
- serf_connection_t *conn = desc->client_data;
+ serf_io_baton_t *io = desc->client_data;
 
- status = serf_event_trigger(ctx, conn, desc);
+ status = serf_event_trigger(ctx, io, desc);
         if (status) {
             return status;
         }

The confusion goes further. Looking at context.c:pollset_add and
context.c:pollset_rm I see that calls to pollset_add always pass a
serf_io_baton_t* while calls to pollset_rm always pass a
serf_connection_t*. For example:

Breakpoint 1, pollset_add (user_baton=0x45a2c0, pfd=0x7fffffffd8a0,
    serf_baton=0x45bf20) at context.c:84
84 serf_pollset_t *s = (serf_pollset_t*)user_baton;
(gdb)
Continuing.

Breakpoint 2, pollset_rm (user_baton=0x45a2c0, pfd=0x7fffffffd8a0,
    serf_baton=0x45bf10) at context.c:93
93 serf_pollset_t *s = (serf_pollset_t*)user_baton;
(gdb)
Continuing.

Breakpoint 1, pollset_add (user_baton=0x45a2c0, pfd=0x7fffffffd8a0,
    serf_baton=0x45bf20) at context.c:84
84 serf_pollset_t *s = (serf_pollset_t*)user_baton;
(gdb)
Continuing.

Breakpoint 2, pollset_rm (user_baton=0x45a2c0, pfd=0x7fffffffd8b0,
    serf_baton=0x45bf10) at context.c:93
93 serf_pollset_t *s = (serf_pollset_t*)user_baton;
(gdb)

The difference between 0x45bf10 and 0x45bf20 corresponds to the offset
of the serf_connection_t* in a serf_io_baton_t*. I was amazed this
worked at all until I discovered that the underlying implementation of
apr_pollset_remove doesn't use the serf_baton passed to pollset_rm so it
doesn't matter that add and rm don't match. Still, we should fix it:

Index: context.c
===================================================================
--- context.c (revision 2445)
+++ context.c (working copy)
@@ -212,7 +212,7 @@ apr_status_t serf_event_trigger(
             tdesc.desc.s = conn->skt;
             tdesc.reqevents = conn->reqevents;
             ctx->pollset_rm(ctx->pollset_baton,
- &tdesc, conn);
+ &tdesc, &conn->baton);
             return conn->status;
         }
         /* apr_pollset_poll() can return a conn multiple times... */
@@ -233,7 +233,7 @@ apr_status_t serf_event_trigger(
                 tdesc.desc.s = conn->skt;
                 tdesc.reqevents = conn->reqevents;
                 ctx->pollset_rm(ctx->pollset_baton,
- &tdesc, conn);
+ &tdesc, &conn->baton);
             }
             return conn->status;
         }
Index: outgoing.c
===================================================================
--- outgoing.c (revision 2445)
+++ outgoing.c (working copy)
@@ -142,7 +142,7 @@ apr_status_t serf__conn_update_pollset(serf_connec
     desc.reqevents = conn->reqevents;
 
     status = ctx->pollset_rm(ctx->pollset_baton,
- &desc, conn);
+ &desc, &conn->baton);
     if (status && !APR_STATUS_IS_NOTFOUND(status))
         return status;
 
@@ -547,7 +547,7 @@ static apr_status_t remove_connection(serf_context
     desc.reqevents = conn->reqevents;
 
     return ctx->pollset_rm(ctx->pollset_baton,
- &desc, conn);
+ &desc, &conn->baton);
 }
 
 /* A socket was closed, inform the application. */

-- 
Philip