Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <47FC8F61.5090505@apache.org>
Date: Wed, 09 Apr 2008 11:41:53 +0200
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r646206 - in /httpd/sandbox/amsterdam/d:
 build/build-modules-c.awk
 include/http_core.h server/Makefile.in server/serf_filters.c
References: <20080409072119.A58521A9832@eris.apache.org>
In-Reply-To: <20080409072119.A58521A9832@eris.apache.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

On 09.04.2008 09:21, pquerna@apache.org wrote:
> Author: pquerna
> Date: Wed Apr  9 00:21:01 2008
> New Revision: 646206
> 
> URL: http://svn.apache.org/viewvc?rev=646206&view=rev
> Log:
> Move the core serf filters to the core.
> 
> Added:
>     httpd/sandbox/amsterdam/d/server/serf_filters.c
> Modified:
>     httpd/sandbox/amsterdam/d/build/build-modules-c.awk
>     httpd/sandbox/amsterdam/d/include/http_core.h
>     httpd/sandbox/amsterdam/d/server/Makefile.in
> 

> Added: httpd/sandbox/amsterdam/d/server/serf_filters.c
> URL: http://svn.apache.org/viewvc/httpd/sandbox/amsterdam/d/server/serf_filters.c?rev=646206&view=auto
> ==============================================================================
> --- httpd/sandbox/amsterdam/d/server/serf_filters.c (added)
> +++ httpd/sandbox/amsterdam/d/server/serf_filters.c Wed Apr  9 00:21:01 2008

> +
> +static int serf_input_filter(ap_filter_t *f, apr_bucket_brigade *bb,
> +                             ap_input_mode_t mode, apr_read_type_e block,
> +                             apr_off_t readbytes)
> +{
> +    apr_status_t status;
> +    core_net_rec *net = f->ctx;
> +    serf_core_ctx_t *ctx = (serf_core_ctx_t*)net->in_ctx;
> +
> +    if (mode == AP_MODE_INIT) {
> +        return APR_SUCCESS;
> +    }
> +    if (!ctx)
> +    {
> +        ctx = init_ctx(f, net->client_socket);
> +        net->in_ctx = (void*)ctx;
> +    }
> +
> +    if (mode == AP_MODE_GETLINE) {
> +        const char *data;
> +        apr_size_t len;
> +        int found;
> +        apr_bucket *b;
> +
> +        ctx->serf_bucket_status = serf_bucket_readline(ctx->serf_in_bucket,
> +                                                       SERF_NEWLINE_ANY,
> +                                                       &found, &data, &len);
> +        b = apr_bucket_transient_create(data, len, f->c->bucket_alloc);
> +        APR_BRIGADE_INSERT_TAIL(bb, b);
> +        return APR_SUCCESS;

Why returning APR_SUCCESS here in all cases? What if the connection to the 
client got broken?

> +    }
> +    if (mode == AP_MODE_READBYTES) {
> +        const char *data;
> +        apr_size_t len;
> +        apr_bucket *b;
> +
> +        ctx->serf_bucket_status = serf_bucket_read(ctx->serf_in_bucket,
> +                                                   readbytes, &data, &len);
> +        b = apr_bucket_transient_create(data, len, f->c->bucket_alloc);
> +        APR_BRIGADE_INSERT_TAIL(bb, b);
> +        return APR_SUCCESS;

Same as above.

> +    }
> +
> +    if (mode == AP_MODE_SPECULATIVE) {
> +        const char *data;
> +        apr_size_t len;
> +        apr_bucket *b;
> +        serf_bucket_t *sb;
> +
> +        ctx->serf_bucket_status = serf_bucket_peek(ctx->serf_in_bucket,
> +                                                   &data, &len);
> +
> +        b = apr_bucket_transient_create(data, len, f->c->bucket_alloc);
> +        APR_BRIGADE_INSERT_TAIL(bb, b);
> +        return APR_SUCCESS;

Same as above.

> +    }
> +
> +    if (mode == AP_MODE_EATCRLF || mode == AP_MODE_EXHAUSTIVE) {
> +        abort();

Looks like a TODO here :-).

> +    }
> +}
> +
> +static apr_status_t serf_output_filter(ap_filter_t *f,
> +                                       apr_bucket_brigade *new_bb)
> +{
> +    apr_status_t rv;
> +    serf_bucket_t *b;
> +    conn_rec *c = f->c;
> +    core_net_rec *net = f->ctx;
> +    serf_core_ctx_t *ctx = (serf_core_ctx_t*)net->in_ctx;
> +    if (!ctx) {
> +        ctx = init_ctx(f, net->client_socket);
> +        net->in_ctx = (void*)ctx;
> +    }
> +
> +    if (new_bb) {
> +        b = brigade_create(f, new_bb, ctx);
> +        serf_bucket_aggregate_append(ctx->serf_out_bucket, b);
> +        c->data_in_output_filters = 1;
> +    }
> +
> +    if (c->data_in_output_filters && new_bb == NULL) {

How do we get in the situation where new_bb is NULL?

> +        do {
> +            apr_status_t srv;
> +            const char *buf;
> +            apr_size_t len = 0;
> +
> +            srv = serf_bucket_read(ctx->serf_out_bucket, SERF_READ_ALL_AVAIL,
> +                                   &buf, &len);
> +
> +            if (SERF_BUCKET_READ_ERROR(srv)) {
> +                /* bad, logme */
> +                return srv;
> +            }
> +
> +            /* write data to network here. */
> +            if (len > 0) {
> +                apr_size_t blen = len;
> +                rv = apr_socket_send(net->client_socket, buf, &blen);
> +                if (blen != len) {
> +                    b = serf_bucket_simple_create(buf+blen, len - blen, NULL, NULL, ctx->serf_bkt_alloc);
> +                    serf_bucket_aggregate_prepend(ctx->serf_out_bucket, b);
> +                    srv = APR_SUCCESS;
> +                }
> +            }
> +
> +            if (APR_STATUS_IS_EOF(srv)) {
> +                c->data_in_output_filters = 0;
> +                break;
> +            }
> +            if (APR_STATUS_IS_EAGAIN(srv)) {
> +                break;
> +            }
> +        } while (rv == APR_SUCCESS);
> +    }
> +
> +    return APR_SUCCESS;

Is it really ok to return APR_SUCCESS in all cases?

Regards

Rüdiger