httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@ebuilt.com>
Subject Re: Code questions (server/protocol.c)
Date Tue, 12 Mar 2002 08:25:12 GMT
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


Mime
View raw message