You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2004/10/01 14:00:16 UTC
Re: ap_save_brigade vs ENOTIMPL setaside
On Thu, Sep 23, 2004 at 09:36:30PM +0100, Joe Orton wrote:
> So what's the proper fix? If ap_save_brigade did:
>
> if bkt->setaside() == ENOTIMPL
> bkt->read()
> if bkt->setaside() != SUCCESS
> give up
There is a problem with this new logic: the mod_perl SV bucket type
happens to be a case where ->setaside() is ENOTIMPL (currently) and
->read() does not morph the bucket type, so then ap_save_brigade() will
abort without doing any work which is quite nasty.
I think the logic needs to be refined to copy the read data into a HEAP
or POOL bucket if the second setaside() call still gives ENOTIMPL, as
below. I'm not sure whether POOL or HEAP is better, but if it does use
a POOL the data may get copied again if it is later turned into a HEAP,
which seems inefficient. Comments?
--- server/util_filter.c 26 Sep 2004 15:52:51 -0000 1.101
+++ server/util_filter.c 1 Oct 2004 11:33:27 -0000
@@ -557,6 +557,18 @@
rv = apr_bucket_read(e, &s, &n, APR_BLOCK_READ);
if (rv == APR_SUCCESS) {
rv = apr_bucket_setaside(e, p);
+ if (rv == APR_ENOTIMPL) {
+ apr_bucket *h;
+
+ /* No choice but to manually move it into a heap bucket */
+ h = apr_bucket_heap_create(s, n, NULL, f->c->bucket_alloc);
+
+ APR_BUCKET_INSERT_AFTER(e, h);
+ apr_bucket_delete(e);
+
+ rv = APR_SUCCESS;
+ e = h;
+ }
}
}
Re: ap_save_brigade vs ENOTIMPL setaside
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 1 Oct 2004, Joe Orton wrote:
> current mod_perl installations. The mod_perl guys know that the lack of
> ->setaside() is a bug and are working on that for future releases.
>
> So a better proposal is this: if setaside() == ENOTIMPL, read() == OK,
> and then setaside() still == ENOTIMPL, then still carry on and setaside
> the rest of buckets in the brigade, but do return the error.
>
> Is this OK?
+1, sounds like a good compromise.
--Cliff
Re: ap_save_brigade vs ENOTIMPL setaside
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Oct 01, 2004 at 11:16:13AM -0400, Cliff Woolley wrote:
> On Fri, 1 Oct 2004, Joe Orton wrote:
>
> > There is a problem with this new logic: the mod_perl SV bucket type
> > happens to be a case where ->setaside() is ENOTIMPL (currently) and
> > ->read() does not morph the bucket type, so then ap_save_brigade() will
> > abort without doing any work which is quite nasty.
>
> That sounds like a bug in the mod_perl SV bucket type. ->read() is
> supposed to be defined such that the result of the read is a bucket that
> *can* be set aside.
>
> >From apr_buckets.h, starting at line 76:
>
> * read returns the address and size of the data in the bucket. If the
> * data isn't in memory then it is read in and the bucket changes type
> * so that it can refer to the new location of the data. If all the
> * data doesn't fit in the bucket then a new bucket is inserted into
> * the brigade to hold the rest of it.
Yes, the ->read() function meets those requirements fine, the bucket
type just wraps a Perl variable in memory so ->read() just gives you
back that memory. But the problem is that ->setaside() is ENOTIMPL.
> In other words, if the data is not in memory, ->read() should put it there
> and morph the bucket type. If it is already in memory, then ->setaside()
> should be implemented.
>
> The logic to deal with all this is already complicated enough from the
> caller's perspective... I'd rather not make it even *more* complicated
> just to deal with broken bucket types.
OK, that makes sense. I just want to be able to backport the
ap_save_brigade() changes without triggering unnecessary regressions in
current mod_perl installations. The mod_perl guys know that the lack of
->setaside() is a bug and are working on that for future releases.
So a better proposal is this: if setaside() == ENOTIMPL, read() == OK,
and then setaside() still == ENOTIMPL, then still carry on and setaside
the rest of buckets in the brigade, but do return the error.
This is equivalent to the old logic except that it will return an error
for a bad bucket type which can't be setaside, and it will correctly
setaside more good bucket types e.g. CGI buckets. (all
ap_save_brigade() callers in 2.0 ignore the return value so the former
change really makes little difference)
Is this OK?
--- server/util_filter.c 26 Sep 2004 15:52:51 -0000 1.101
+++ server/util_filter.c 1 Oct 2004 15:33:34 -0000
@@ -532,7 +532,7 @@
apr_bucket_brigade **b, apr_pool_t *p)
{
apr_bucket *e;
- apr_status_t rv;
+ apr_status_t rv, srv = APR_SUCCESS;
/* If have never stored any data in the filter, then we had better
* create an empty bucket brigade so that we can concat.
@@ -561,11 +561,16 @@
}
if (rv != APR_SUCCESS) {
- return rv;
+ srv = rv;
+ /* Return an error but still save the brigade if
+ * ->setaside() is really not implemented. */
+ if (rv != APR_ENOTIMPL) {
+ return rv;
+ }
}
}
APR_BRIGADE_CONCAT(*saveto, *b);
- return APR_SUCCESS;
+ return srv;
}
AP_DECLARE_NONSTD(apr_status_t) ap_filter_flush(apr_bucket_brigade *bb,
Re: ap_save_brigade vs ENOTIMPL setaside
Posted by Cliff Woolley <jw...@virginia.edu>.
On Fri, 1 Oct 2004, Joe Orton wrote:
> There is a problem with this new logic: the mod_perl SV bucket type
> happens to be a case where ->setaside() is ENOTIMPL (currently) and
> ->read() does not morph the bucket type, so then ap_save_brigade() will
> abort without doing any work which is quite nasty.
That sounds like a bug in the mod_perl SV bucket type. ->read() is
supposed to be defined such that the result of the read is a bucket that
*can* be set aside.
>From apr_buckets.h, starting at line 76:
* read returns the address and size of the data in the bucket. If the
* data isn't in memory then it is read in and the bucket changes type
* so that it can refer to the new location of the data. If all the
* data doesn't fit in the bucket then a new bucket is inserted into
* the brigade to hold the rest of it.
In other words, if the data is not in memory, ->read() should put it there
and morph the bucket type. If it is already in memory, then ->setaside()
should be implemented.
The logic to deal with all this is already complicated enough from the
caller's perspective... I'd rather not make it even *more* complicated
just to deal with broken bucket types.
--Cliff