I've been thinking about this all night, so now I have a bunch of
questions. :-)
> The attached patch adds a capabilities flag field to the
> apr_bucket_type_t. Flags currently included indicate whether the
> "optional" functions (setaside,copy,split) are implemented or not without
> having to actually call them to find out, whether the bucket is a
What does this provide us? We are talking about improving performance by
a single function call, and either way, we have to handle error
conditions from those functions. The code is much easier to read and
understand if we just use the return code to determine that the function
doesn't exist. Consider the following:
if (bucket->flags & APR_BUCKET_FLAG_SETASIDE) {
if (apr_bucket_setaside(...) != APR_SUCCESS) {
/* handle error */
}
}
When compared to:
if (apr_bucket_setaside(...) != APR_SUCCESS) {
/* handle error */
}
If we are going to put the check inside of apr_bucket_setaside, we can
already check to see if the function is supported (see below).
> "metadata" bucket, and whether the bucket has a file descriptor in it for
> the use by apr_sendfile().
metadata buckets might be useful, but I am having a hard time seeing it.
Do we really expect more modules to implement their own metadata buckets?
Does it matter if they do? By definition, all buckets implement read
functions. A metadata bucket is simply a bucket that always returns no
data from read. If a module implements their own metadata bucket, then
that module needs to be the thing to handle that bucket type, so I am
missing the utility of the metadata flag.
The sendfile flag I ask about below.
> The main reason I need this right now is that I've implemented and am
> about to commit a patch to Apache's mod_file_cache that uses a custom
> bucket type ("ap_bucket_cachedfile") to denote a file descriptor that's
> shared among multiple threads. Such a file descriptor can be sendfile'd,
> but cannot be read by other means without causing thread-safety problems.
> Apache's core_output_filter currently is hardcoded to assume that a bucket
> can be sendfile'd ONLY if it matches APR_BUCKET_IS_FILE(), though the
> above is clearly a case where an APR_BUCKET_SUPPORTS_SENDFILE() test would
> be much more useful and extensible. The flags field gives us that
> ability.
I am severly missing how this file works. If it can only be sendfile'd,
then we are basically saying that no files in the file cache can be sent
through a filter. There is no garauntee that a filter will not try to
mmap or read from a file. Support for limiting what can be done with a
file descriptor, should be a part of the apr_file_t structure. So, if you
are opening a file to be shared across multiple threads, then that
information should be stored in the apr_file_t, and then the read/mmap
functions should look at the apr_file_t and realize that they can't
operate on that file. Once that work is done, I don't understand the need
for the sendfile flag. By definition, a file bucket can be used in
sendfile if the platform has sendfile. By allowing the APR file functions
to determine if the file can be read/mmap'ed, the cachedfile bucket can go
away completely, and it can become just a standard file bucket again.
> I can easily see future situations arising where similar strategies would
> be useful, so adding a flags field now would allow us to easily handle
> those situations.
I am having a hard time seeing these situations. The number of functions
the bucket implements is already a part of the structure, so we know at
what point the bucket type was added. We should add checks to the
apr_bucket_(setaside, copy, etc) functions to ensure that the bucket does
support those functions, but that is a minor patch.
Can you please post some other situations where these flags are useful.
In general, I like the idea of having more information in the bucket
structure, but I want to be sure that when we add this information, we do
so with very good reasons.
> If nobody objects, I'll commit this tomorrow, followed closely by the
> patch to mod_file_cache and the core_output_filter in Apache.
Please give more time than one day when posting such large changes. This
really deserves two or three days so that people who aren't reading e-mail
everyday can try to keep up.
Ryan
_______________________________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
