On Mon, Mar 11, 2002 at 03:03:02PM +0100, Sander Striker wrote:
> Hi,
>
> server/protocol.c:136
> if (ap_strcasestr(type, "charset=") != NULL) {
> /* already has parameter, do nothing */
> /* XXX we don't check the validity */
> ;
> }
>
> Validity checking seems like a good idea, someone
> want to grab this one?
Seems like you volunteered. =)
> server/protocol.c:658
> #if 0
> /* XXX If we want to keep track of the Method, the protocol module should do
> * it. That support isn't in the scoreboard yet. Hopefully next week
> * sometime. rbb */
> ap_update_connection_status(AP_CHILD_THREAD_FROM_ID(conn->id), "Method",
> r->method);
> #endif
>
> Can this block go? 'next week' has been over 6 months now :)
Ah, toss it. I don't think the scoreboard needs to record the
method.
>
> server/protocol.c:823
> r->request_config = ap_create_request_config(r->pool);
> /* Must be set before we run create request hook */
>
> r->proto_output_filters = conn->output_filters;
> r->output_filters = r->proto_output_filters;
> r->proto_input_filters = conn->input_filters;
> r->input_filters = r->proto_input_filters;
> ap_run_create_request(r);
>
> To what code does the comment refer? The line above it, or
> the block under it?
The line below. The filters must be set before we run that hook.
> server/protocol.c:1133
> /* Humm, is this check the best it can be?
> * - protocol >= HTTP/1.1 implies support for chunking
> * - non-keepalive implies the end of byte stream will be signaled
> * by a connection close
> * In both cases, we can send bytes to the client w/o needing to
> * compute content-length.
> * Todo:
> * We should be able to force connection close from this filter
> * when we see we are buffering too much.
> */
>
> The comment says it all.
Um, I think the conditional is right. The todo may be wrong as I
think we could just send "flush" down, but I'm not really clear
what its context is.
> server/protocol.c:1290
> AP_DECLARE(size_t) ap_send_mmap(apr_mmap_t *mm, request_rec *r, size_t offset,
> size_t length)
>
> I reckon the size_t's are left here intentional, weren't forgotten when
> switching to apr_size_t?
Does anything use this? I can't fathom any situation where this should
be called - this screams an MMAP bucket to be passed down. size_t
should refer to anything in memory, while off_t refers to position in
a file. We have some mismatches in that. If anyone would like a
nice project, cleaning up our bucket/brigade types to remove the
dependency on apr_size_t/apr_off_t would be real nice before GA
(create apr_bucket_size_t and apr_brigade_size_t). But, it'll be
painful.
> server/protocol.c:1338
> /* future optimization: record some flags in the request_rec to
> * say whether we've added our filter, and whether it is first.
> */
>
> Still valid?
I think so. Boy, this looks like it could get real expensive.
Ouch.
> server/protocol.c:1501
> /* ### TODO: if the total output is large, put all the strings
> * ### into a single brigade, rather than flushing each time we
> * ### fill the buffer
> */
>
> And that's another one for our performance freaks ;)
Definitely. That todo seems the right thing to do
here. -- justin
