httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: svn commit: r647263 - in /httpd/httpd/trunk: ./ docs/manual/mod/ include/ modules/aaa/ modules/filters/ modules/http/ server/
Date Fri, 02 May 2008 10:40:01 GMT
Ruediger Pluem wrote:

>> +    /* all is well, set aside the buckets */
>> +    for (bucket = APR_BRIGADE_FIRST(b);
>> +         bucket != APR_BRIGADE_SENTINEL(b);
>> +         bucket = APR_BUCKET_NEXT(bucket))
>> +    {
>> +        apr_bucket_copy(bucket, &e);
> 
> What about transient buckets? Don't we need to set them aside?

I don't follow - does the apr_bucket_copy not do that for us already?

>> +    ctx->remaining -= readbytes;
>> +    ctx->offset += readbytes;
>> +    return APR_SUCCESS;
> 
> Why using ctx->offset at all and not just taking all data from the 
> kept_body brigade until readbytes,
> copy it over to b and remove it afterwards from the kept_body brigade. 
> This would save one call
> to apr_brigade_partition.

In theory, that would mean you could only read the kept_body once. The 
kept body could be delivered to multiple requests embedded within 
mod_include for example, and would be needed to be read more than once.

>> +                    c = low ^ hi;
> 
> Shouldn't this be c = low + hi ?

In theory either should work, which is faster?

>> +            /* If we have been asked to, keep the data up until the
>> +             * configured limit. If the limit is exceeded, we return an
>> +             * HTTP_REQUEST_ENTITY_TOO_LARGE response so the caller is
>> +             * clear the server couldn't handle their request.
>> +             */
>> +            if (kept_body) {
>> +                if (len <= left) {
>> +                    apr_bucket_copy(bucket, &e);
>> +                    APR_BRIGADE_INSERT_TAIL(kept_body, e);
>> +                    left -= len;
>> +                }
>> +                else {
>> +                    apr_brigade_destroy(bb);
>> +                    apr_brigade_destroy(kept_body);
>> +                    return HTTP_REQUEST_ENTITY_TOO_LARGE;
>> +                }
> 
> Why is this needed? Should this job be performed by the 
> ap_keep_body_filter that should
> be in our input filter chain if we want to keep the body?
> Of course this depends when we call ap_parse_request_form. If we call it 
> during the
> authn/z phase the filter chain hasn't been setup. So maybe we should 
> ensure that
> this is the case.

I think the reason it is there was from when the kept body was being 
captured by ap_discard_request_body, which wouldn't be run if this code 
kicked in.

However we do call it in the authn/z phase, so if the keep body filter 
isn't set up yet then it does still need to be here.

> Why not using the insert_filter hook?

Good question, let me look.

>> @@ -1648,8 +1649,8 @@
>>       * Add the KEPT_BODY filter, which will insert any body marked to be
>>       * kept for the use of a subrequest, into the subrequest.
>>       */
>> -    ap_add_input_filter_handle(ap_kept_body_input_filter_handle,
>> -                               NULL, rnew, rnew->connection);
>> +    ap_add_input_filter(KEPT_BODY_FILTER,
>> +                        NULL, rnew, rnew->connection);
>>  
> 
> This creates an error message on each subrequest if mod_request is not 
> loaded, because
> in this case the KEPT_BODY_FILTER is not registered.

You're right, let me look at this.

Regards,
Graham
--

Mime
View raw message