httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
Date Tue, 13 Nov 2001 09:49:03 GMT
On Mon, Nov 12, 2001 at 11:49:08PM -0000, rbb@apache.org wrote:
>...
>   --- httpd.h	2001/10/23 17:26:57	1.168
>   +++ httpd.h	2001/11/12 23:49:06	1.169
>...
>   +typedef struct core_output_filter_ctx {
>   +    apr_bucket_brigade *b;
>   +    apr_pool_t *subpool; /* subpool of c->pool used for data saved after a
>   +                          * request is finished
>   +                          */
>   +    int subpool_has_stuff; /* anything in the subpool? */
>   +} core_output_filter_ctx_t;
>   + 
>   +typedef struct core_filter_ctx {
>   +    apr_bucket_brigade *b;
>   +    int first_line;
>   +} core_ctx_t;
>   + 
>   +typedef struct core_net_rec {
>   +    /** Connection to the client */
>   +    apr_socket_t *client_socket;
>   +
>   +    /** connection record */
>   +    conn_rec *c;
>   + 
>   +    core_output_filter_ctx_t *out_ctx;
>   +    core_ctx_t *in_ctx;
>   +} core_net_rec;

The distinction between core.c and connection.c is currently too nebulous to
try and keep code in one module or another. The whole premise of this change
was to *hide* the above structures. You can/should do so by moving them into
core.c. Pull functions from connection.c into core.c to enable that change.

Honestly, I can never find functions any more. If it has something to do
with serving http, then I have to look in *SEVEN* files:

    server/connection.c
    server/core.c
    server/protocol.c
    server/request.c
    modules/http/http_core.c
    modules/http/http_protocol.c
    modules/http/http_request.c

The boundaries and purposes of each of these are so flimsy that it is not
obvious where things are found. I'd rather just cat them into one big file
and stop worrying about where to find the darned stuff.

>...
>   --- core.c	2001/11/08 14:29:36	1.88
>   +++ core.c	2001/11/12 23:49:06	1.89
>...
>    static int core_input_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t
mode, apr_off_t *readbytes)
>    {
>...
>   +    if (ctx->first_line) {
>   +        apr_setsocketopt(net->client_socket, APR_SO_TIMEOUT,
>   +                         (int)(keptalive
>   +                         ? f->c->base_server->keep_alive_timeout * APR_USEC_PER_SEC
>   +                         : f->c->base_server->timeout * APR_USEC_PER_SEC));
>   +        ctx->first_line = 0;
>   +    }
>   +    else {
>   +        if (keptalive) {
>   +            apr_setsocketopt(net->client_socket, APR_SO_TIMEOUT,
>   +                             (int)(f->c->base_server->timeout * APR_USEC_PER_SEC));
>   +        }
>   +    }

This logic is incorrect. first_line is only going to be set when the filter
is first installed. Thus, the keep_alive_timeout will never be used.

The old code would execute the "first_line" branch of logic on each entry to
ap_read_request().

>...
>   +static conn_rec *core_create_conn(apr_pool_t *ptrans, apr_socket_t *csd,
>   +                                  int my_child_num)
>   +{
>   +    core_net_rec *net = apr_palloc(ptrans, sizeof(*net));
>   +
>   +    net->in_ctx = NULL;
>   +    net->out_ctx = NULL;
>   +    net->c = ap_core_new_connection(ptrans, ap_server_conf, csd,
>   +                                    net, my_child_num);

ARGH!!

ap_server_conf is a global. That used to be passed to the connection
creation function. It should continue to be passed. *Please* don't continue
propagating global variables :-(

>...
>   --- perchild.c	2001/11/10 18:26:29	1.82
>   +++ perchild.c	2001/11/12 23:49:07	1.83
>   @@ -502,7 +502,7 @@
>            ap_sock_disable_nagle(sock);
>        }
>    
>   -    current_conn = ap_new_connection(p, ap_server_conf, sock, conn_id);
>   +    current_conn = ap_run_create_connection(p, ap_server_conf, sock, conn_id);

There is an extra parameter there, but I think it should stay and the rest
should change :-)


Otherwise... a good move. Can we toss the pre_connection stuff?

Oh, and I clipped out the quoted text. The core_create_conn should be
APR_HOOK_MIDDLE. There is no reason for it to be "last." The MIDDLE is the
standard one to use when you just need to pick something and have no
particular concerns about where it falls. The core_create_conn is "just
another" hook.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message