apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <cliffwool...@yahoo.com>
Subject Bucket API cleanup issues
Date Tue, 27 Feb 2001 04:57:35 GMT

Okay, I've got a series of pretty major bucket API cleanups I'd like to do
(most of which are mostly transparent outside apr-util, BTW).  Before I
go off and commit, though, this was major enough to warrant seeking group
consent.  Here are the issues I'm addressing, in roughly descending order
of majority:

1) The apr_bucket_shared and apr_bucket_simple structs are completely
useless, and a needless time/resource drain.  If we just add an apr_off_t
start into the apr_bucket struct, apr_bucket_shared and apr_bucket_simple
can go away completely, saving massive amounts of work in all of the
bucket code and making the code vastly simpler and easier to
read/understand.  That simplicity can easily be seen in that
apr_bucket_shared_split is reduced to simple_split plus a refcount++ (same
for shared_copy).  I've renamed simple_split and simple_copy giving them
the apr_bucket_ prefix and making them public functions called by their
shared counterparts.  I have this patched, tested, and ready to go.  If
anyone wants to see the patch, tell me and I'll post it.

2) The parameter 'w' (returned length) to apr_bucket_heap_make() and
apr_bucket_heap_create() is useless.  The returned length is invariant
from the passed-in length, assuming malloc didn't fail, which we can
easily determine from the function's return value.  There's no way to get
*part* of the data made into a heap bucket... malloc just doesn't do
"partial" allocations.  You've either got enough memory or you dont.
Those callers that are currently using the parameter generally discard its
function anyway, seeming to think that they have to use it just because
it's there.  This patch is done for apr-util but not for Apache.

3) pool_bucket_cleanup() is completely bogus AFAICT.  I've added this
comment to the code, which describes the problems pretty well:
     * XXX: this is fubar... only the *first* apr_bucket to reference
     * the bucket is changed to a heap bucket... ALL references must
     * be changed.  So instead of h->b, we need an array of b's in h.
     * pool_destroy() should remove that bucket from the array.  But
     * pool_destroy doesn't know where the apr_bucket itself is.
     * ARRGGGH.  Only solution: apr_bucket_destroy() should let the
     * e->type->destroy() function destroy the apr_bucket itself, too.
     * pool_destroy() can remove destroyed apr_bucket referrers from
     * the array.  Any referrers that remain when this function is
     * called get converted to heap buckets by adding a loop over the
     * array here (only the first call to apr_bucket_heap_make() gets
     * the copy flag set, of course).  Another gotcha, though this one
     * is easily worked around: apr_bucket_heap_make() calls
     * apr_bucket_shared_make() which invalidates the reference count
     * on the refcounted bucket. --jcw

4) The same problem applies to file buckets that have been split/copied
when APR_HAS_MMAP: when one of them gets read it gets made into an MMAP.
ALL of the file buckets should be converted at the same time to reference
the same MMAP.

5) apr_bucket_destroy() should allow the type-specific destructor to free
the bucket itself, too (needed to solve problem #3).

6) Should mmap_destroy() call apr_mmap_delete(m->mmap) after when the last
reference to the mmap bucket is destroyed?

7) socket_read() and pipe_read() should return APR_SUCCESS even in an EOF
condition, as discussed on new-httpd a week or so ago.  I haven't made
this change yet, though I have put an XXX comment in the code about it,
because it requres fixing the input filters in Apache to figure out EOS on
their own.  Looking for suggestions on how to do that.

8) Can I make apr_bucket_shared_destroy() just return a boolean value
indicating whether or not this bucket is the last reference to the shared
resource?  It's silly to return a pointer to that resource, since the
caller must already know where the resource is anyway.

9) file_read in the non-mmap case was allocating HUGE_STRING_LEN even if
the file was smaller than that. Fixed and thrown in with patch #1.

10) socket_read and pipe_read should modify the heap bucket they create to
have alloc_len == HUGE_STRING_LEN to reflect the actual size of buf. Fixed
and thrown in with patch #1.

PHEW!  I think that's it for now.  Comments, please?


View raw message