Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <473B6625.7000001@apache.org>
Date: Wed, 14 Nov 2007 22:18:29 +0100
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.9) Gecko/20071030 SeaMonkey/1.1.6
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r595028 -
 /httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c
References: <20071114195907.0B0CE1A9832@eris.apache.org>
In-Reply-To: <20071114195907.0B0CE1A9832@eris.apache.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit


On 11/14/2007 08:59 PM, jerenkrantz@apache.org wrote:
> Author: jerenkrantz
> Date: Wed Nov 14 11:59:05 2007
> New Revision: 595028
> 
> URL: http://svn.apache.org/viewvc?rev=595028&view=rev
> Log:
> Amsterdam sandbox: add serf input/output filters that replace the core filters.
> 
> Modified:
>     httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c
> 
> Modified: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c
> URL: http://svn.apache.org/viewvc/httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?rev=595028&r1=595027&r2=595028&view=diff
> ==============================================================================
> --- httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c (original)
> +++ httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c Wed Nov 14 11:59:05 2007

> @@ -370,6 +372,207 @@
>      return baton.rstatus;
>  }
>  
> +typedef struct {
> +    serf_context_t *serf_ctx;
> +    serf_bucket_alloc_t *serf_bkt_alloc;
> +    serf_bucket_t *serf_in_bucket;
> +    serf_bucket_t *serf_out_bucket;
> +    apr_bucket_brigade *out_brigade;
> +    apr_bucket_brigade *tmp_brigade;
> +    apr_status_t serf_bucket_status;
> +} serf_core_ctx_t;
> +
> +typedef struct {
> +    apr_pool_t *pool;
> +    serf_bucket_alloc_t *allocator;
> +    serf_core_ctx_t *core_ctx;
> +    apr_bucket_brigade *bb;
> +    apr_bucket_brigade *tmp_bb;
> +} brigade_bucket_ctx_t;
> +
> +/* Forward-declare */
> +const serf_bucket_type_t serf_bucket_type_brigade;
> +
> +static serf_bucket_t * brigade_create(ap_filter_t *f, serf_core_ctx_t *core_ctx)
> +{
> +    brigade_bucket_ctx_t *ctx;
> +
> +    ctx = serf_bucket_mem_alloc(core_ctx->serf_bkt_alloc, sizeof(*ctx));
> +    ctx->allocator = core_ctx->serf_bkt_alloc;
> +    ctx->pool = serf_bucket_allocator_get_pool(ctx->allocator);
> +    ctx->core_ctx = core_ctx;
> +    ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
> +    ctx->tmp_bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
> +
> +    return serf_bucket_create(&serf_bucket_type_brigade, ctx->allocator, ctx);
> +}
> +
> +static apr_status_t brigade_read(serf_bucket_t *bucket,
> +                                 apr_size_t requested,
> +                                 const char **data, apr_size_t *len)
> +{
> +    brigade_bucket_ctx_t *ctx = bucket->data;
> +    apr_status_t status;
> +    apr_bucket *b, *end, *f;
> +
> +    b = APR_BRIGADE_FIRST(ctx->bb);
> +    status = apr_bucket_read(b, data, len, APR_BLOCK_READ);

Isn't it dangerous that we do not copy *data here?
Doesn't this data get lost when we delete the bucket in the while loop below?

> +
> +    if (requested < *len) {
> +        *len = requested;
> +    }
> +    status = apr_brigade_partition(ctx->bb, *len, &end);
> +    f = APR_BRIGADE_FIRST(ctx->bb);
> +    while (f != end && f != APR_BRIGADE_SENTINEL(ctx->bb)) {
> +        apr_bucket_delete(f);
> +        f = APR_BRIGADE_FIRST(ctx->bb);
> +    }
> +    return status;
> +}
> +
> +static apr_status_t brigade_readline(serf_bucket_t *bucket,
> +                                     int acceptable, int *found,
> +                                     const char **data, apr_size_t *len)
> +{
> +    brigade_bucket_ctx_t *ctx = bucket->data;
> +    apr_status_t status;
> +
> +    status = apr_brigade_split_line(ctx->tmp_bb, ctx->bb,
> +                                    APR_BLOCK_READ, HUGE_STRING_LEN);
> +    if (APR_STATUS_IS_EAGAIN(status)) {
> +        if (found) {
> +            *found = SERF_NEWLINE_NONE;
> +        }

How do we set *found if the status is not EAGAIN?

> +        status = APR_SUCCESS;
> +    }
> +    apr_brigade_pflatten(ctx->bb, data, len, ctx->pool);

Shouldn't this be ctx->tmp_bb?
Shouldn't we call apr_brigade_cleanup(ctx->tmp_bb) here?

Regards

Rüdiger