Re: More detailed review of Greg's filtering patch

["Roy T. Fielding" <fi...@kiwi.ICS.UCI.EDU>]

>>  API_EXPORT(int) ap_rputc(int c, request_rec *r)
>>  {
>>      ap_bucket_t bucket = ap_bucket_ptrlen_init(&c, 1);
>
>nit: &c may not point to the character :-)

:p That's why I asked about ap_rputc passing an unsigned char instead
of an int.

>ap_fc_*() pass the content to the *next* filter. If you pass r->filter to
>those guys, then you will end up skipping the first filter.
>
>Obviously, this portion needs commenting :-) ... I create a "dummy" filter
>that I can pass to ap_fc_*. The ->next field is set to r->filters so that
>you inject the content into the proper filter.
>
>The dummy filter record is the cleanest way to reduce code duplication in
>the "inject" portion of the patches. (without the dummy, you end up with
>code dup between ap_r* and ap_fc_*)

Ummm, no, because those ap_fc_* functions aren't needed if you set up the
bucket brigade in the ap_r* function (see above).

>>...
>> 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.
>
>Yes. It should be the first filter added. As I described elsewhere, I wanted
>to minimize the patch size and the possible impact. I created
>BUFF_filter_callback to be equivalent to what *would* be the first filter.
>But I didn't want to include its addition to the chain with the patches
>necessary to create the basic filtering system. A second round would have
>all the necessary bits/tweaks to move BUFF_filter_callback to wherever it
>needed to go and to get it inserted properly, on the proper requests, in the
>right order, etc.
>
>"minimize perturbation" was my rule here :-)

I'd prefer to minimize my brain cell wastage.

>>...
>> I don't think we need AP_BUCKET_STRINGS.  In general, we should avoid
>> null-terminated strings except at the ap_r* level.
>
>AP_BUCKET_STRINGS was created so that we could defer copying the strings off
>the stack until we finally determined it was necessary to make that copy.
>
>[ the problem originates from ap_rvputs(r, s1, s2, s3, NULL) ]

I was thinking that you could just allocate a list of buckets at that
point, though I guess a special bucket type is more efficient. *shrug*

>>...
>> 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.
>
>These last two paragraphs:
>
>That global pool is for holding information *about* the filters. Meta info
>if you will. The name -> filter info mapping.

Sounds like config info to me -- use the config pool.

>When you add a filter to a request output chain, it does not use the global
>pool -- it uses the request pool.

Okey doke.

BTW, filter types aren't needed.  Treat filters like a stack, allow
them to be pushed onto r->filters and popped off of r->filters, and
everything will take care of itself.  Why?  Because the order in which
Apache processes the request causes transport protocols to be placed
on first, transfer protocols second, transfer encodings third,
content encodings fourth, and content generators fifth.  Modules
can't screw with that order because their output wouldn't make
sense if they did (i.e., if they sent content first, and then
decided that it should be gzipped, they have no way to retrieve
their first content -- they have no choice but to arrange the
filter stack in order).  The core doesn't need to care for the same
reason that the core doesn't validate content == mime type.

This would also allow the same filter to be used for different levels
based on the decisions of the "invisible hand" that arranges the stack
rather than any assumptions about the context in which it is used.

....Roy