httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <jwool...@virginia.edu>
Subject Re: is it possible to mark buckets to be copied only when to be set-aside?
Date Wed, 19 May 2004 14:20:57 GMT
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

Mime
View raw message