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, [EMAIL PROTECTED] 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/
