httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, VF-Group <ruediger.pl...@vodafone.com>
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 11:07:54 GMT
 

> -----Ursprüngliche Nachricht-----
> Von: Graham Leggett 
> Gesendet: Freitag, 2. Mai 2008 12:40
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r647263 - in /httpd/httpd/trunk: ./ 
> docs/manual/mod/ include/ modules/aaa/ modules/filters/ 
> modules/http/ server/
> 
> 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?

No, it does not.

> 
> >> +    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.

But to deliver it more then once you would need to reset the filter context, right?

> 
> >> +                    c = low ^ hi;
> > 
> > Shouldn't this be c = low + hi ?
> 
> In theory either should work, which is faster?

I think there is not much difference with respect to speed but using
'+' seems to be easier to read.

> 
> >> +            /* 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.

Yes, but what worries me is that other input filters aren't setup as well
that might be needed. Couldn't there be a case where we need to have the inflate
input filter in place?
Maybe it is needed to ensure that the input filter stack is already setup
before we read from it.

Regards

Rüdiger


Mime
View raw message