httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] Take two on the conn_rec change
Date Tue, 17 Apr 2001 22:43:18 GMT
Okay... I think the patch is fine. Just nits at this point.

+1 because I'd like to see more HTTP stuff move back to modules/http/. IMO,
it was a mistake to move them back to server/ in the first place :-)

On Tue, Apr 17, 2001 at 02:39:26PM -0700, rbb@covalent.net wrote:
>...
> +static int read_request_line(request_rec *r)

Were there any changes to this function during the move? It is very
difficult to tell from the patch.

>...
> +static int http_create_request(request_rec *r)
> +{
> +    ap_http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
&http_module);
> +    unsigned keptalive;

"int"

> +
> +    hconn = apr_pcalloc(r->pool, sizeof(*hconn));
> +    ap_set_module_config(r->connection->conn_config, &http_module, hconn);
> +
> +    if (!r->main && !(r->prev || r->next)) {

a bit clearer might be:

  if (!r->main && !r->prev && !r->next)

or (my preferred form):

  if (r->main == NULL && r->prev == NULL && r->next == NULL)

> +        keptalive = r->connection->keepalive == 1;
> +        r->connection->keepalive    = 0;
> +
> +        apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
> +                         (int)(keptalive
> +                         ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
> +                         : r->server->timeout * APR_USEC_PER_SEC));

I'm guessing there is some optimization available w/ setting these timeouts.
No suggestions, other than leaving a comment to that effect?

> +
> +        /* Get the request... */
> +        if (!read_request_line(r)) {
> +            if (r->status == HTTP_REQUEST_URI_TOO_LARGE) {
> +                ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> +                              "request failed: URI too long");
> +                ap_send_error_response(r, 0);
> +                ap_run_log_transaction(r);
> +                return OK;
> +            }
> +            return DONE;
> +        }
> +        if (keptalive) {
> +            apr_setsocketopt(r->connection->client_socket,
> +                             APR_SO_TIMEOUT,
> +                             (int)(r->server->timeout * APR_USEC_PER_SEC));
> +        }
> +
> +        keptalive = 0;

useless assignment.

> +
> +        return OK;
> +    }
> +    return OK;

That first return is useless.

>...
> --- modules/http/mod_core.h	2001/02/28 15:24:07	1.6
> +++ modules/http/mod_core.h	2001/04/17 21:29:28
> @@ -70,6 +70,13 @@
>   * @package mod_core private header file
>   */
> 
> +typedef struct ap_http_conn_rec ap_http_conn_rec;
> +
> +struct ap_http_conn_rec {
> +    /** How many times have we used it? */
> +    int keepalives;
> +};

Is it really worth it to create this structure? Other guys use it, so it
isn't private. Do we envision more stuff going into this from the conn_rec?
(i.e. moving soon; we can always create this structure later)

>...
> @@ -571,7 +458,9 @@
>      r->notes           = apr_table_make(r->pool, 5);
> 
>      r->request_config  = ap_create_request_config(r->pool);
> -    ap_run_create_request(r);
> +    if (ap_run_create_request(r) != 0) {
> +        return NULL;
> +    }

That can return OK, DECLINE, or some kind of error (DONE in your patch). I
believe that if DECLINE is returned, then we ought to error out (i.e. nobody
is handling the request).

I'd suggest changing the 0 to OK for clarity.

Cheers,
-g

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

Mime
View raw message