> 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.
+1
> 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.
+1
> 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.
I disagree about how to fix this. The simple solution is to just abstract
on level, such as:
bucket -> bucket -> bucket
| | |
shared shared shared
| | |
-----------------------------
|
pool_bucket
Same works for file_buckets, and it is why simple_buckets were invented
IIRC. Of course if this is done, then heap buckets need to have the same
general setup. This keeps us from having to traverse a list of buckets
whenever we convert from one type to another.
> 5) apr_bucket_destroy() should allow the type-specific destructor to free
> the bucket itself, too (needed to solve problem #3).
No. This was done on purpose so that we can cache the buckets themselves.
If the type-specific destructor frees the bucket itself, then we can't
cache the freed buckets. It also makes it very difficult to convert
buckets from one type to the next.
> 6) Should mmap_destroy() call apr_mmap_delete(m->mmap) after when the last
> reference to the mmap bucket is destroyed?
No. If you do that, then caching MMAP's becomes impossible. The
apr_mmap_delete should be left up to the pool cleanup.
> 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.
Didn't we decide that buckets should return EOF? I could be wrong, but I
was under the impression that buckets needed to return EOF in order for
things to work.
> 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.
+1
> 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.
+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.
+1
> PHEW! I think that's it for now. Comments, please?
Each of these should be a separate patch, and preferably with a few hours
in between each patch.
Ryan
_______________________________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
