You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2008/09/06 10:38:43 UTC
Re: svn commit: r692567 - /httpd/httpd/trunk/modules/filters/mod_charset_lite.c
On 09/06/2008 12:21 AM, gregames@apache.org wrote:
> Author: gregames
> Date: Fri Sep 5 15:21:36 2008
> New Revision: 692567
>
> URL: http://svn.apache.org/viewvc?rev=692567&view=rev
> Log:
> PR 45687: Detect and pass along error buckets
>
> Submitted by: Dan Poirier <poirier pobox.org>
> Reviewed by: trawick
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_charset_lite.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_charset_lite.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_charset_lite.c?rev=692567&r1=692566&r2=692567&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_charset_lite.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_charset_lite.c Fri Sep 5 15:21:36 2008
> @@ -414,6 +414,23 @@
> return rv;
> }
>
> +static apr_status_t send_bucket_downstream(ap_filter_t *f, apr_bucket *b)
> +{
> + request_rec *r = f->r;
> + conn_rec *c = r->connection;
> + apr_bucket_brigade *bb;
> + charset_filter_ctx_t *ctx = f->ctx;
> + apr_status_t rv;
> +
> + bb = apr_brigade_create(r->pool, c->bucket_alloc);
This is bad. Please store the brigade in the filter context and reuse it,
by cleaning it. See also http://httpd.apache.org/docs/trunk/en/developer/output-filters.html#filtering
The same error is in send_downstream and IMHO they can share the same brigade.
> + APR_BRIGADE_INSERT_TAIL(bb, b);
> + rv = ap_pass_brigade(f->next, bb);
> + if (rv != APR_SUCCESS) {
> + ctx->ees = EES_DOWNSTREAM;
> + }
> + return rv;
> +}
> +
> static apr_status_t set_aside_partial_char(charset_filter_ctx_t *ctx,
> const char *partial,
> apr_size_t partial_len)
> @@ -673,7 +690,7 @@
> }
> b = APR_BRIGADE_FIRST(bb);
> if (b == APR_BRIGADE_SENTINEL(bb) ||
> - APR_BUCKET_IS_EOS(b)) {
> + APR_BUCKET_IS_METADATA(b)) {
> break;
> }
> rv = apr_bucket_read(b, &bucket, &bytes_in_bucket, APR_BLOCK_READ);
> @@ -892,6 +909,17 @@
> }
> break;
> }
> + if (APR_BUCKET_IS_METADATA(dptr)) {
Don't we need to handle flush buckets separately and flush our buffer downstream
as far as we can in this case?
> + apr_bucket *metadata_bucket;
> + metadata_bucket = dptr;
> + dptr = APR_BUCKET_NEXT(dptr);
> + APR_BUCKET_REMOVE(metadata_bucket);
> + rv = send_bucket_downstream(f, metadata_bucket);
> + if (rv != APR_SUCCESS) {
> + done = 1;
> + }
> + continue;
> + }
> rv = apr_bucket_read(dptr, &cur_str, &cur_len, APR_BLOCK_READ);
> if (rv != APR_SUCCESS) {
> done = 1;
Regards
Rüdiger
Re: svn commit: r692567 - /httpd/httpd/trunk/modules/filters/mod_charset_lite.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/06/2008 01:30 PM, Eric Covener wrote:
> On Sat, Sep 6, 2008 at 4:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>>> @@ -673,7 +690,7 @@
>>> }
>>> b = APR_BRIGADE_FIRST(bb);
>>> if (b == APR_BRIGADE_SENTINEL(bb) ||
>>> - APR_BUCKET_IS_EOS(b)) {
>>> + APR_BUCKET_IS_METADATA(b)) {
>>> break;
>>> }
>>> rv = apr_bucket_read(b, &bucket, &bytes_in_bucket,
>>> APR_BLOCK_READ);
>> Don't we need to handle flush buckets separately and flush our buffer
>> downstream as far as we can in this case?
>
> I think this part is correct in the patch. This path is used on input
> only, and the break-on-metadata will causes us to:
>
> move remaining input into our filter state
> create a heap bucket containing all the bytes we've xlated so far
> (collected in a char*)
> add the heap bucket onto the (emptied) bb being returned
> add any metadata at the head of the input/filterstate to the bb being returned
>
> The only thing held onto past the flush would be leftover bytes from
> (apr_xlate() == APR_INCOMPLETE), for which it seems better to send
> what we had (i.e. multibyte sequence with a flush bucket separating
> the bytes doesn't get flushed)
>
You are corerect. Thanks for explaining. I missed that this only happens
in the input filter. Apart from the fact that the current code flow does
it correct as you have explained, flush buckets do not seem to make a lot of
sense in an input brigade anyway.
Regards
Rüdiger
Re: svn commit: r692567 - /httpd/httpd/trunk/modules/filters/mod_charset_lite.c
Posted by Eric Covener <co...@gmail.com>.
On Sat, Sep 6, 2008 at 4:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>> @@ -673,7 +690,7 @@
>> }
>> b = APR_BRIGADE_FIRST(bb);
>> if (b == APR_BRIGADE_SENTINEL(bb) ||
>> - APR_BUCKET_IS_EOS(b)) {
>> + APR_BUCKET_IS_METADATA(b)) {
>> break;
>> }
>> rv = apr_bucket_read(b, &bucket, &bytes_in_bucket,
>> APR_BLOCK_READ);
>
> Don't we need to handle flush buckets separately and flush our buffer
> downstream as far as we can in this case?
I think this part is correct in the patch. This path is used on input
only, and the break-on-metadata will causes us to:
move remaining input into our filter state
create a heap bucket containing all the bytes we've xlated so far
(collected in a char*)
add the heap bucket onto the (emptied) bb being returned
add any metadata at the head of the input/filterstate to the bb being returned
The only thing held onto past the flush would be leftover bytes from
(apr_xlate() == APR_INCOMPLETE), for which it seems better to send
what we had (i.e. multibyte sequence with a flush bucket separating
the bytes doesn't get flushed)
--
Eric Covener
covener@gmail.com
Re: svn commit: r692567 - /httpd/httpd/trunk/modules/filters/mod_charset_lite.c
Posted by Greg Ames <am...@gmail.com>.
On Sat, Sep 6, 2008 at 4:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>> + bb = apr_brigade_create(r->pool, c->bucket_alloc);
>>
>
> This is bad. Please store the brigade in the filter context and reuse it,
> by cleaning it. See also
> http://httpd.apache.org/docs/trunk/en/developer/output-filters.html#filtering
I committed Dan's latest patch as r693564 which reuses the brigade as
suggested.
<http://httpd.apache.org/docs/trunk/en/developer/output-filters.html#filtering>
> The same error is in send_downstream and IMHO they can share the same
> brigade.
Dan plans to merge code to take care of that, but not in this patch for ease
of review.
Greg