You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com> on 2007/11/15 13:02:10 UTC
httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
- if (APR_BRIGADE_SENTINEL(ctx->bb)) {
+ if (APR_BRIGADE_EMPTY(ctx->bb)) {
+ *len = 0;
+ return APR_EOF;
+ }
+
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(ctx->bb)) ||
+ AP_BUCKET_IS_EOR(APR_BRIGADE_FIRST(ctx->bb))) {
*len = 0;
return APR_EOF;
Why do we not put this in one if block like?
if (APR_BRIGADE_SENTINEL(ctx->bb) ||
APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(ctx->bb)) ||
AP_BUCKET_IS_EOR(APR_BRIGADE_FIRST(ctx->bb))) {
*len = 0;
return APR_EOF;
}
@@ -567,8 +567,21 @@
return APR_SUCCESS;
}
- if (mode == AP_MODE_EATCRLF || mode == AP_MODE_EXHAUSTIVE ||
- mode == AP_MODE_SPECULATIVE) {
+ 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_read(ctx->serf_in_bucket,
+ readbytes, &data, &len);
+
+ sb = serf_bucket_simple_create(data, len, NULL, NULL, ctx->serf_bkt_alloc);
+ serf_bucket_aggregate_prepend(ctx->serf_in_bucket, sb);
Hm. Don't we need the following lines here?
b = apr_bucket_transient_create(data, len, f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, b);
Otherwise I guess we read the data but do not return it :-).
+ return APR_SUCCESS;
+ }
+
+ if (mode == AP_MODE_EATCRLF || mode == AP_MODE_EXHAUSTIVE) {
abort();
}
}
Regards
Rüdiger
Re: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Nov 15, 2007 12:17 PM, Ruediger Pluem <rp...@apache.org> wrote:
> And in the case above we only read the data from ser_in_bucket, but unlike in the AP_MODE_READBYTES
> case we do not create a transient bucket with data and add it to the bb brigade. So the
> caller gets back an empty brigade with no data.
Yup. My bad. Fixed in r595396. -- justin
Re: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 15, 2007, at 12:17 PM, Ruediger Pluem wrote:
>> I don't follow that, but I'm still not quite fully awake yet...
>
> Well, as far as I understand AP_MODE_SPECULATIVE its purpose is to
> read data and return
> it to the caller, but leave the data in the input filter chain such
> that a following read
> (or better a get_brigade) from the input filter chain returns the
> same data again.
> And in the case above we only read the data from ser_in_bucket, but
> unlike in the AP_MODE_READBYTES
> case we do not create a transient bucket with data and add it to
> the bb brigade. So the
> caller gets back an empty brigade with no data.
>
Ahh yes, I see know. Yep. This appears to be the case that we're
dropping bytes.
Re: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/15/2007 04:26 PM, Jim Jagielski wrote:
>
> On Nov 15, 2007, at 7:02 AM, Plüm, Rüdiger, VF-Group wrote:
>
>>
>> @@ -567,8 +567,21 @@
>> return APR_SUCCESS;
>> }
>>
>> - if (mode == AP_MODE_EATCRLF || mode == AP_MODE_EXHAUSTIVE ||
>> - mode == AP_MODE_SPECULATIVE) {
>> + 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_read(ctx->serf_in_bucket,
>> + readbytes, &data,
>> &len);
>> +
>> + sb = serf_bucket_simple_create(data, len, NULL, NULL,
>> ctx->serf_bkt_alloc);
>> + serf_bucket_aggregate_prepend(ctx->serf_in_bucket, sb);
>>
>> Hm. Don't we need the following lines here?
>>
>> b = apr_bucket_transient_create(data, len, f->c->bucket_alloc);
>> APR_BRIGADE_INSERT_TAIL(bb, b);
>>
>> Otherwise I guess we read the data but do not return it :-).
>>
>>
>
> I don't follow that, but I'm still not quite fully awake yet...
Well, as far as I understand AP_MODE_SPECULATIVE its purpose is to read data and return
it to the caller, but leave the data in the input filter chain such that a following read
(or better a get_brigade) from the input filter chain returns the same data again.
And in the case above we only read the data from ser_in_bucket, but unlike in the AP_MODE_READBYTES
case we do not create a transient bucket with data and add it to the bb brigade. So the
caller gets back an empty brigade with no data.
Regards
Rüdiger
Re: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 15, 2007, at 7:02 AM, Plüm, Rüdiger, VF-Group wrote:
>
> @@ -567,8 +567,21 @@
> return APR_SUCCESS;
> }
>
> - if (mode == AP_MODE_EATCRLF || mode == AP_MODE_EXHAUSTIVE ||
> - mode == AP_MODE_SPECULATIVE) {
> + 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_read(ctx-
> >serf_in_bucket,
> + readbytes,
> &data, &len);
> +
> + sb = serf_bucket_simple_create(data, len, NULL, NULL, ctx-
> >serf_bkt_alloc);
> + serf_bucket_aggregate_prepend(ctx->serf_in_bucket, sb);
>
> Hm. Don't we need the following lines here?
>
> b = apr_bucket_transient_create(data, len, f->c-
> >bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(bb, b);
>
> Otherwise I guess we read the data but do not return it :-).
>
>
I don't follow that, but I'm still not quite fully awake yet...
Re: httpd/sandbox/amsterdam/d/modules/proxy/mod_serf.c?revision=595110&view=markup
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 15, 2007, at 7:02 AM, Plüm, Rüdiger, VF-Group wrote:
>
> - if (APR_BRIGADE_SENTINEL(ctx->bb)) {
> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
> + *len = 0;
> + return APR_EOF;
> + }
> +
> + if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(ctx->bb)) ||
> + AP_BUCKET_IS_EOR(APR_BRIGADE_FIRST(ctx->bb))) {
> *len = 0;
> return APR_EOF;
>
> Why do we not put this in one if block like?
>
LOL... I saw the commit message and thought the same thing and
actually changed it before I actually checked dev@ :) :)