On Wed, 19 May 2004, Cliff Woolley wrote:
> Note: ap_save_brigade() handles the setaside and brigade concatenation for
> you. I suspect there are a number of places in the code that we are
> incorrectly calling APR_BRIGADE_CONCAT() instead of ap_save_brigade().
> That's bug city right there.
Worse, there are probably places in the code where we don't even use
APR_BRIGADE_CONCAT() and simply append each bucket one at a time from the
source brigade to the "saved" brigade using APR_BRIGADE_INSERT_TAIL() (or
even APR_BUCKET_INSERT_AFTER(), though it's hard for me to imagine a case
where doing it that way would have made sense to the programmer).
Furthermore, I just found a bug in ap_save_brigade(). On line 534 of
util_filter.c:
rv = apr_bucket_setaside(e, p);
if (rv != APR_SUCCESS
/* ### this ENOTIMPL will go away once we implement setaside
### for all bucket types. */
&& rv != APR_ENOTIMPL) {
return rv;
}
Note that ### comment? Yeah it denotes what at this point is a bug. You
can't just ignore APR_ENOTIMPL for bucket types, because pipe and socket
buckets never had setaside implemented on them. I thought I remembered
that that was because Greg and Ryan and I had some huge debate about it
and it was decided for some reason that setting aside a pipe or socket
didn't make sense. But now I can't find where we talked about that in the
archives. Anyway, when Greg originally wrote the lines in question (see
http://marc.theaimsgroup.com/?l=apr-dev&m=99190977519715&w=2), he
obviously did so assuming that setaside would be implemented on all bucket
types. It was not.
I recommend that the interested reader go back and take a look at the
following thread (this kind of starts somewhere in the middle but this one
message and its followups are particularly relevant):
http://marc.theaimsgroup.com/?l=apr-dev&m=99178798619777&w=2
As it is, given the current State Of The Buckets: If rv == APR_ENOTIMPL
when you call apr_bucket_setaside(), then you're supposed to call
apr_bucket_read(), get the data out of the bucket, and stick it into a
heap bucket. ap_save_brigade() could possibly do a little optimizing here
in the special cases of socket and pipe buckets, and grab the actual APR
socket or pipe out of the bucket's internal data and compare the
socket/pipe's pool to pool p. If they already match, you don't have to do
the read and make-into-heap-bucket process; the reason being is that the
point of setaside is to tell the buckets code that you want it to
guarantee that the data storage behind bucket e will live at least as long
as pool p.
I thought I could just refer for some of this back to either of my sets of
ApacheCon slides, but I can see I discussed setaside() insufficiently well
at both ApacheCons. :(
--Cliff
