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,
