httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@kiwi.ICS.UCI.EDU>
Subject More detailed review of Greg's filtering patch
Date Sun, 23 Jul 2000 01:53:05 GMT
Greg explained some things that I didn't understand about the filters patch
while we were in Monterey.  For example, since we aren't sharing bucket
structures, it is possible to allocate those on the stack and
not worry about freeing them -- any blocking point on the stream
will just save the bucket data and not the structure.  However, we still
need a way to differentiate between statically allocated data and
dynamically allocated data, since we don't want heap stuff copied when
it doesn't need to be.

Just thought of one issue.  If we are passing down a large chain
of buckets and some bottom part is going to set them aside, is it
best to copy just the contents (effectively merging the buckets)
or set aside a copy of the bucket structure as well?  I guess that
would depend on the type of bucket.

Other comments on greg.patch:

flush_filters is named wrong -- ending the stream is different from flushing
buffered content from the end of the stream.  This really should be
ap_end_content, or simply ap_rclose.  We may also want a flush call,
but that should only impact filters that have set aside processed data
for the purpose of output buffering -- unprocessed data should not be
affected.

I don't like the DECL_FILTER_HEAD macro for a variety of reasons.
The style reason is simply that it is better to leave the type visible
for code readability, which could be accomplished by 

#define AP_INIT_FILTER_HEAD(r)  { NULL, (r), NULL, 0, (r)->filters }

     ap_filter fc = AP_INIT_FILTER_HEAD(r, filter);

but I'm not fond of that either.  Basically, I don't understand why
we need to allocate a filter structure instead of just pointing to
the r->filter.  Instead of

 API_EXPORT(int) ap_rputc(int c, request_rec *r)
 {
     DECL_FILTER_HEAD(r, filter);
 
     if (r->connection->aborted)
         return EOF;

     ap_fc_putc(&filter, c);
 
     SET_BYTES_SENT(r);
     return 1;
 }

I would prefer

 API_EXPORT(int) ap_rputc(int c, request_rec *r)
 {
     ap_bucket_t bucket = ap_bucket_ptrlen_init(&c, 1);

     if (r->connection->aborted)
         return EOF;

     ap_fc_write(r->filter, b);
 
     SET_BYTES_SENT(r);
     return 1;
 }

In order for this to work in Greg's scheme, r->filter would have to be
primed with the BUFF connection.  ap_bucket_ptrlen_init would still be
a macro function.

On a side note, we should rethink SET_BYTES_SENT.  And shouldn't ap_rputc
be writing a const unsigned char?

BUFF_filter_callback confuses me, but that may be just because I don't
think of function pointers as callbacks.  Having a NULL next filter
imply BUFF_filter_callback may hide programming errors.  This should
just be the first filter placed on the filter list.

There are too many ap_fc_putblah functions -- we should be doing the
conversion to bucket brigades in the ap_r* functions.  All of the filter
writes should be returning a status value.  A given filter may need
to know about the request_rec (for the actual filtering logic), but
the generic filter processing should not have access to r.

Eventually, sub requests should be replaced by a URI bucket and
APACHE_XLATE should just be a content filter.

Regarding ap_filter.h:

Buckets are separate from filters -- they should not be in the same file.
Likewise, placing filters on the output chain is separable from filter
processing.  We haven't talked about input filters yet, but we should
keep the concept in mind when choosing symbol names.

I don't think we need AP_BUCKET_STRINGS.  In general, we should avoid
null-terminated strings except at the ap_r* level.

After talking to Greg, I now understand the desire for AP_BUCKET_PRINTF.

The bucket structure should separate itself from the data it carries:

struct ap_bucket_t {
    ap_bucket_type type;
    ap_bucket_t *next;          /* next bucket in list */
    ap_bucket_t *prev;          /* previous bucket in list */

    void *data;                 /* structure for bucket-specific data */
    ap_ssize_t datalen;         /* shortcut for buffer processing */
};

so that each specific bucket type has its own structure.  That is assuming
we don't want to be able to register bucket types, which is needed to
separate the bucket implementation from the Apache-specific request types.
In order for bucket registration to work, the bucket itself needs to
contain function pointers for type conversion (bucket_to_nstr) and
ap_bucket_type would be a struct of function pointers.

Regarding filters.c:

A global pool and registration functions simply won't work.  In a
multithreaded process model, we will have multiple simultaneous streams
of output filters, and there should be one pool for each stream that
is a subpool of the connection pool.  Each filter should inherit the
pool from the next filter on the chain.

Summary:

I understand this design much better after having talked to Greg
about the properties of allocating bucket structures off the stack.
I would still change a lot of this, but the only thing I am -1 about
is the filter registration and global pool.  The rest are design
differences.

....Roy

Mime
View raw message