You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Fritsch <sf...@sfritsch.de> on 2011/08/31 23:08:51 UTC
non-splittable buckets (was: Regression with range fix)
On Wednesday 31 August 2011, Jim Jagielski wrote:
> >> Looking at the patch in 2.2.x; there is a lot of effort expended
> >> deadling with apr_bucket_split() returning ENOTIMPL - that looks
> >> unnecessary; the filter will only handle brigades containing
> >> buckets with known length, and all such buckets "must" be
> >> _split-able.
> >
> > So you think we can rip out the whole if (rv == APR_ENOTIMPL)
> > blocks?
>
> Belt and suspenders?
If we rip it out, we should replace it with ap_assert()s. And maybe
only do it in 2.3?
A different idea I had: If apr_bucket_split() returns ENOTIMPL, we
could call apr_brigade_partition on the copied brigade.
apr_brigade_partition() would then do all the complex handling of
apr_bucket_read(), etc. It is only slightly less efficient because it
has to traverse the brigade again. But the memory usage would still
stay low because the original brigade would remain untouched. And we
would get rid of much of the complexity in copy_brigade_range().
Re: non-splittable buckets (was: Regression with range fix)
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 1, 2011, at 8:59 AM, Joe Orton wrote:
> On Thu, Sep 01, 2011 at 02:47:19PM +0200, "Plüm, Rüdiger, VF-Group" wrote:
>>>> If we rip it out, we should replace it with ap_assert()s. And maybe
>>>> only do it in 2.3?
>>>
>>> It would seem odd to have ENOTIMPL as a "fatal" error but other
>>> "real" errors non-fatal. *No* error should occur here with unless
>>> somebody has cooked up a really whacky bucket type.
>>
>> So you would follow the rip-out-and-replace-with-asserts approach?
>
> No, just remove the special case for ENOTIMPL and don't change anything
> else.
>
+1...
Re: non-splittable buckets (was: Regression with range fix)
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Sep 01, 2011 at 02:47:19PM +0200, "Plüm, Rüdiger, VF-Group" wrote:
> > > If we rip it out, we should replace it with ap_assert()s. And maybe
> > > only do it in 2.3?
> >
> > It would seem odd to have ENOTIMPL as a "fatal" error but other
> > "real" errors non-fatal. *No* error should occur here with unless
> > somebody has cooked up a really whacky bucket type.
>
> So you would follow the rip-out-and-replace-with-asserts approach?
No, just remove the special case for ENOTIMPL and don't change anything
else.
Index: modules/http/byterange_filter.c
===================================================================
--- modules/http/byterange_filter.c (revision 1163995)
+++ modules/http/byterange_filter.c (working copy)
@@ -94,8 +94,6 @@
apr_bucket *first = NULL, *last = NULL, *out_first = NULL, *e;
apr_uint64_t pos = 0, off_first = 0, off_last = 0;
apr_status_t rv;
- const char *s;
- apr_size_t len;
apr_uint64_t start64, end64;
apr_off_t pofft = 0;
@@ -147,44 +145,10 @@
if (e == first) {
if (off_first != start64) {
rv = apr_bucket_split(copy, (apr_size_t)(start64 - off_first));
- if (rv == APR_ENOTIMPL) {
- rv = apr_bucket_read(copy, &s, &len, APR_BLOCK_READ);
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- /*
- * The read above might have morphed copy in a bucket
- * of shorter length. So read and delete until we reached
- * the correct bucket for splitting.
- */
- while (start64 - off_first > (apr_uint64_t)copy->length) {
- apr_bucket *tmp = APR_BUCKET_NEXT(copy);
- off_first += (apr_uint64_t)copy->length;
- APR_BUCKET_REMOVE(copy);
- apr_bucket_destroy(copy);
- copy = tmp;
- rv = apr_bucket_read(copy, &s, &len, APR_BLOCK_READ);
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- }
- if (start64 > off_first) {
- rv = apr_bucket_split(copy, (apr_size_t)(start64 - off_first));
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- }
- else {
- copy = APR_BUCKET_PREV(copy);
- }
+ if (rv != APR_SUCCESS) {
+ apr_brigade_cleanup(bbout);
+ return rv;
}
- else if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
out_first = APR_BUCKET_NEXT(copy);
APR_BUCKET_REMOVE(copy);
apr_bucket_destroy(copy);
@@ -200,38 +164,10 @@
}
if (end64 - off_last != (apr_uint64_t)e->length) {
rv = apr_bucket_split(copy, (apr_size_t)(end64 + 1 - off_last));
- if (rv == APR_ENOTIMPL) {
- rv = apr_bucket_read(copy, &s, &len, APR_BLOCK_READ);
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- /*
- * The read above might have morphed copy in a bucket
- * of shorter length. So read until we reached
- * the correct bucket for splitting.
- */
- while (end64 + 1 - off_last > (apr_uint64_t)copy->length) {
- off_last += (apr_uint64_t)copy->length;
- copy = APR_BUCKET_NEXT(copy);
- rv = apr_bucket_read(copy, &s, &len, APR_BLOCK_READ);
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- }
- if (end64 < off_last + (apr_uint64_t)copy->length - 1) {
- rv = apr_bucket_split(copy, (apr_size_t)(end64 + 1 - off_last));
- if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
- }
+ if (rv != APR_SUCCESS) {
+ apr_brigade_cleanup(bbout);
+ return rv;
}
- else if (rv != APR_SUCCESS) {
- apr_brigade_cleanup(bbout);
- return rv;
- }
copy = APR_BUCKET_NEXT(copy);
if (copy != APR_BRIGADE_SENTINEL(bbout)) {
APR_BUCKET_REMOVE(copy);
RE: non-splittable buckets (was: Regression with range fix)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton [mailto:jorton@redhat.com]
> Sent: Donnerstag, 1. September 2011 14:39
> To: dev@httpd.apache.org
> Subject: Re: non-splittable buckets (was: Regression with range fix)
>
> On Wed, Aug 31, 2011 at 11:08:51PM +0200, Stefan Fritsch wrote:
> > On Wednesday 31 August 2011, Jim Jagielski wrote:
> > > >> Looking at the patch in 2.2.x; there is a lot of
> effort expended
> > > >> deadling with apr_bucket_split() returning ENOTIMPL -
> that looks
> > > >> unnecessary; the filter will only handle brigades containing
> > > >> buckets with known length, and all such buckets "must" be
> > > >> _split-able.
> > > >
> > > > So you think we can rip out the whole if (rv == APR_ENOTIMPL)
> > > > blocks?
>
> Yes. This code could only get exercised with a custom bucket
> type which
> has fixed-length buckets but no ->split implementation.
>
> > > Belt and suspenders?
> >
> > If we rip it out, we should replace it with ap_assert()s. And maybe
> > only do it in 2.3?
>
> It would seem odd to have ENOTIMPL as a "fatal" error but
> other "real"
> errors non-fatal. *No* error should occur here with unless
> somebody has
> cooked up a really whacky bucket type.
So you would follow the rip-out-and-replace-with-asserts approach?
Regards
Rüdiger
Re: non-splittable buckets (was: Regression with range fix)
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Aug 31, 2011 at 11:08:51PM +0200, Stefan Fritsch wrote:
> On Wednesday 31 August 2011, Jim Jagielski wrote:
> > >> Looking at the patch in 2.2.x; there is a lot of effort expended
> > >> deadling with apr_bucket_split() returning ENOTIMPL - that looks
> > >> unnecessary; the filter will only handle brigades containing
> > >> buckets with known length, and all such buckets "must" be
> > >> _split-able.
> > >
> > > So you think we can rip out the whole if (rv == APR_ENOTIMPL)
> > > blocks?
Yes. This code could only get exercised with a custom bucket type which
has fixed-length buckets but no ->split implementation.
> > Belt and suspenders?
>
> If we rip it out, we should replace it with ap_assert()s. And maybe
> only do it in 2.3?
It would seem odd to have ENOTIMPL as a "fatal" error but other "real"
errors non-fatal. *No* error should occur here with unless somebody has
cooked up a really whacky bucket type.
Regards, Joe
RE: non-splittable buckets (was: Regression with range fix)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Stefan Fritsch [mailto:sf@sfritsch.de]
> Sent: Mittwoch, 31. August 2011 23:09
> To: dev@httpd.apache.org
> Subject: non-splittable buckets (was: Regression with range fix)
>
> On Wednesday 31 August 2011, Jim Jagielski wrote:
> > >> Looking at the patch in 2.2.x; there is a lot of effort expended
> > >> deadling with apr_bucket_split() returning ENOTIMPL - that looks
> > >> unnecessary; the filter will only handle brigades containing
> > >> buckets with known length, and all such buckets "must" be
> > >> _split-able.
> > >
> > > So you think we can rip out the whole if (rv == APR_ENOTIMPL)
> > > blocks?
> >
> > Belt and suspenders?
>
> If we rip it out, we should replace it with ap_assert()s. And maybe
> only do it in 2.3?
I guess asserts are fine. IMHO we should do it in 2.3 and 2.2 if we do it.
Nothing is different between 2.2 and 2.3 in this respect and we keep the
diff between 2.2 and 2.3 small.
>From what I can see Joe is correct with his assumption that this case
cannot happen there.
>
> A different idea I had: If apr_bucket_split() returns ENOTIMPL, we
> could call apr_brigade_partition on the copied brigade.
> apr_brigade_partition() would then do all the complex handling of
> apr_bucket_read(), etc. It is only slightly less efficient because it
> has to traverse the brigade again. But the memory usage would still
> stay low because the original brigade would remain untouched. And we
> would get rid of much of the complexity in copy_brigade_range().
>
Would be an approach, but I would like to see Joe's feedback on this
as I don't see value in reconstructing this part if ripping out is fine
as well.
Regards
Rüdiger