You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/04/17 23:39:26 UTC
[PATCH] Take two on the conn_rec change
Here is the second version, which handles the bug Greg noticed. Namely,
our timeout is correct with this one.
Ryan
? build.log
? build.err
? server/mpm/prefork/cd
Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.146
diff -u -d -b -w -u -r1.146 httpd.h
--- include/httpd.h 2001/04/16 20:33:16 1.146
+++ include/httpd.h 2001/04/17 21:28:52
@@ -860,14 +860,10 @@
unsigned aborted:1;
/** Are we using HTTP Keep-Alive? -1 fatal error, 0 undecided, 1 yes */
signed int keepalive:2;
- /** Did we use HTTP Keep-Alive? */
- unsigned keptalive:1;
/** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
* 1 yes/success */
signed int double_reverse:2;
- /** How many times have we used it? */
- int keepalives;
/** server IP address */
char *local_ip;
/** used for ap_get_server_name when UseCanonicalName is set to DNS
Index: modules/http/config.m4
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/config.m4,v
retrieving revision 1.4
diff -u -d -b -w -u -r1.4 config.m4
--- modules/http/config.m4 2001/03/04 06:27:27 1.4
+++ modules/http/config.m4 2001/04/17 21:29:27
@@ -7,5 +7,8 @@
APACHE_MODULE(http, HTTP protocol handling, $http_objects, , yes)
APACHE_MODULE(mime, mapping of file-extension to MIME, , , yes)
+if test "$enable_http" = "yes"; then
+ AC_DEFINE(AP_HTTP_ENABLED, 1, [HTTP is enabled on this server])
+fi
APACHE_MODPATH_FINISH
Index: modules/http/http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.270
diff -u -d -b -w -u -r1.270 http_core.c
--- modules/http/http_core.c 2001/04/11 23:37:16 1.270
+++ modules/http/http_core.c 2001/04/17 21:29:27
@@ -58,6 +58,7 @@
#include "apr_strings.h"
#include "apr_thread_proc.h" /* for RLIMIT stuff */
+#include "apr_lib.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -65,6 +66,7 @@
#define CORE_PRIVATE
#include "httpd.h"
#include "http_config.h"
+#include "http_log.h"
#include "http_connection.h"
#include "http_protocol.h" /* For index_of_response(). Grump. */
#include "http_request.h"
@@ -295,6 +297,154 @@
return OK;
}
+static int read_request_line(request_rec *r)
+{
+ char l[DEFAULT_LIMIT_REQUEST_LINE + 2]; /* getline's two extra for \n\0 */
+ const char *ll = l;
+ const char *uri;
+ const char *pro;
+
+ int major = 1, minor = 0; /* Assume HTTP/1.0 if non-"HTTP" protocol */
+ int len;
+
+ /* Read past empty lines until we get a real request line,
+ * a read error, the connection closes (EOF), or we timeout.
+ *
+ * We skip empty lines because browsers have to tack a CRLF on to the end
+ * of POSTs to support old CERN webservers. But note that we may not
+ * have flushed any previous response completely to the client yet.
+ * We delay the flush as long as possible so that we can improve
+ * performance for clients that are pipelining requests. If a request
+ * is pipelined then we won't block during the (implicit) read() below.
+ * If the requests aren't pipelined, then the client is still waiting
+ * for the final buffer flush from us, and we will block in the implicit
+ * read(). B_SAFEREAD ensures that the BUFF layer flushes if it will
+ * have to block during a read.
+ */
+
+ while ((len = ap_getline(l, sizeof(l), r, 0)) <= 0) {
+ if (len < 0) { /* includes EOF */
+ /* this is a hack to make sure that request time is set,
+ * it's not perfect, but it's better than nothing
+ */
+ r->request_time = apr_time_now();
+ return 0;
+ }
+ }
+ /* we've probably got something to do, ignore graceful restart requests */
+
+ /* XXX - sigwait doesn't work if the signal has been SIG_IGNed (under
+ * linux 2.0 w/ glibc 2.0, anyway), and this step isn't necessary when
+ * we're running a sigwait thread anyway. If/when unthreaded mode is
+ * put back in, we should make sure to ignore this signal iff a sigwait
+ * thread isn't used. - mvsk
+
+#ifdef SIGWINCH
+ apr_signal(SIGWINCH, SIG_IGN);
+#endif
+ */
+
+ r->request_time = apr_time_now();
+ r->the_request = apr_pstrdup(r->pool, l);
+ r->method = ap_getword_white(r->pool, &ll);
+
+#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
+ uri = ap_getword_white(r->pool, &ll);
+
+ /* Provide quick information about the request method as soon as known */
+
+ r->method_number = ap_method_number_of(r->method);
+ if (r->method_number == M_GET && r->method[0] == 'H') {
+ r->header_only = 1;
+ }
+
+ ap_parse_uri(r, uri);
+
+ /* ap_getline returns (size of max buffer - 1) if it fills up the
+ * buffer before finding the end-of-line. This is only going to
+ * happen if it exceeds the configured limit for a request-line.
+ */
+ if (len > r->server->limit_req_line) {
+ r->status = HTTP_REQUEST_URI_TOO_LARGE;
+ r->proto_num = HTTP_VERSION(1,0);
+ r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
+ return 0;
+ }
+
+ if (ll[0]) {
+ r->assbackwards = 0;
+ pro = ll;
+ len = strlen(ll);
+ } else {
+ r->assbackwards = 1;
+ pro = "HTTP/0.9";
+ len = 8;
+ }
+ r->protocol = apr_pstrndup(r->pool, pro, len);
+
+ /* XXX ap_update_connection_status(conn->id, "Protocol", r->protocol); */
+
+ /* Avoid sscanf in the common case */
+ if (len == 8 &&
+ pro[0] == 'H' && pro[1] == 'T' && pro[2] == 'T' && pro[3] == 'P' &&
+ pro[4] == '/' && apr_isdigit(pro[5]) && pro[6] == '.' &&
+ apr_isdigit(pro[7])) {
+ r->proto_num = HTTP_VERSION(pro[5] - '0', pro[7] - '0');
+ } else if (2 == sscanf(r->protocol, "HTTP/%u.%u", &major, &minor)
+ && minor < HTTP_VERSION(1,0)) /* don't allow HTTP/0.1000 */
+ r->proto_num = HTTP_VERSION(major, minor);
+ else
+ r->proto_num = HTTP_VERSION(1,0);
+
+ return 1;
+}
+
+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;
+
+ 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)) {
+ 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));
+
+ /* 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;
+
+ return OK;
+ }
+ return OK;
+}
+
static void register_hooks(apr_pool_t *p)
{
ap_hook_pre_connection(ap_pre_http_connection,NULL,NULL,
@@ -303,6 +453,7 @@
APR_HOOK_REALLY_LAST);
ap_hook_http_method(http_method,NULL,NULL,APR_HOOK_REALLY_LAST);
ap_hook_default_port(http_port,NULL,NULL,APR_HOOK_REALLY_LAST);
+ ap_hook_create_request(http_create_request, NULL, NULL, APR_HOOK_MIDDLE);
ap_register_input_filter("HTTP_IN", ap_http_filter, AP_FTYPE_CONNECTION);
ap_register_input_filter("DECHUNK", ap_dechunk_filter, AP_FTYPE_TRANSCODE);
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.313
diff -u -d -b -w -u -r1.313 http_protocol.c
--- modules/http/http_protocol.c 2001/04/16 21:16:53 1.313
+++ modules/http/http_protocol.c 2001/04/17 21:29:27
@@ -105,6 +105,7 @@
int ka_sent = 0;
int wimpy = ap_find_token(r->pool,
apr_table_get(r->headers_out, "Connection"), "close");
+ ap_http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config, &http_module);
const char *conn = apr_table_get(r->headers_in, "Connection");
/* The following convoluted conditional determines whether or not
@@ -146,7 +147,7 @@
&& r->server->keep_alive
&& (r->server->keep_alive_timeout > 0)
&& ((r->server->keep_alive_max == 0)
- || (r->server->keep_alive_max > r->connection->keepalives))
+ || (r->server->keep_alive_max > hconn->keepalives))
&& !ap_status_drops_connection(r->status)
&& !wimpy
&& !ap_find_token(r->pool, conn, "close")
@@ -154,10 +155,10 @@
|| apr_table_get(r->headers_in, "Via"))
&& ((ka_sent = ap_find_token(r->pool, conn, "keep-alive"))
|| (r->proto_num >= HTTP_VERSION(1,1)))) {
- int left = r->server->keep_alive_max - r->connection->keepalives;
+ int left = r->server->keep_alive_max - hconn->keepalives;
r->connection->keepalive = 1;
- r->connection->keepalives++;
+ hconn->keepalives++;
/* If they sent a Keep-Alive token, send one back */
if (ka_sent) {
Index: modules/http/mod_core.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_core.h,v
retrieving revision 1.6
diff -u -d -b -w -u -r1.6 mod_core.h
--- 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;
+};
+
/*
* These (input) filters are internal to the mod_core operation.
*/
@@ -91,6 +98,8 @@
AP_DECLARE(int) ap_send_http_trace(request_rec *r);
int ap_send_http_options(request_rec *r);
+
+AP_DECLARE_DATA module http_module;
#ifdef __cplusplus
}
Index: modules/loggers/mod_log_config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/loggers/mod_log_config.c,v
retrieving revision 1.52
diff -u -d -b -w -u -r1.52 mod_log_config.c
--- modules/loggers/mod_log_config.c 2001/03/09 20:30:32 1.52
+++ modules/loggers/mod_log_config.c 2001/04/17 21:29:30
@@ -191,6 +191,7 @@
#include "http_core.h" /* For REMOTE_NAME */
#include "http_log.h"
#include "http_protocol.h"
+#include "mod_core.h"
#if APR_HAVE_UNISTD_H
#include <unistd.h>
@@ -527,13 +528,19 @@
}
static const char *log_connection_status(request_rec *r, char *a)
{
+#ifdef AP_HTTP_ENABLED
+ ap_http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
+ &http_module);
+#endif
if (r->connection->aborted)
return "X";
+#ifdef AP_HTTP_ENABLED
if ((r->connection->keepalive) &&
- ((r->server->keep_alive_max - r->connection->keepalives) > 0)) {
+ ((r->server->keep_alive_max - hconn->keepalives) > 0)) {
return "+";
}
+#endif
return "-";
}
Index: server/protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
retrieving revision 1.12
diff -u -d -b -w -u -r1.12 protocol.c
--- server/protocol.c 2001/04/11 23:37:16 1.12
+++ server/protocol.c 2001/04/17 21:30:02
@@ -363,116 +363,6 @@
}
}
-static int read_request_line(request_rec *r)
-{
- char l[DEFAULT_LIMIT_REQUEST_LINE + 2]; /* getline's two extra for \n\0 */
- const char *ll = l;
- const char *uri;
- const char *pro;
-
-#if 0
- conn_rec *conn = r->connection;
-#endif
- int major = 1, minor = 0; /* Assume HTTP/1.0 if non-"HTTP" protocol */
- int len;
-
- /* Read past empty lines until we get a real request line,
- * a read error, the connection closes (EOF), or we timeout.
- *
- * We skip empty lines because browsers have to tack a CRLF on to the end
- * of POSTs to support old CERN webservers. But note that we may not
- * have flushed any previous response completely to the client yet.
- * We delay the flush as long as possible so that we can improve
- * performance for clients that are pipelining requests. If a request
- * is pipelined then we won't block during the (implicit) read() below.
- * If the requests aren't pipelined, then the client is still waiting
- * for the final buffer flush from us, and we will block in the implicit
- * read(). B_SAFEREAD ensures that the BUFF layer flushes if it will
- * have to block during a read.
- */
-
- while ((len = ap_getline(l, sizeof(l), r, 0)) <= 0) {
- if (len < 0) { /* includes EOF */
- /* this is a hack to make sure that request time is set,
- * it's not perfect, but it's better than nothing
- */
- r->request_time = apr_time_now();
- return 0;
- }
- }
- /* we've probably got something to do, ignore graceful restart requests */
-
- /* XXX - sigwait doesn't work if the signal has been SIG_IGNed (under
- * linux 2.0 w/ glibc 2.0, anyway), and this step isn't necessary when
- * we're running a sigwait thread anyway. If/when unthreaded mode is
- * put back in, we should make sure to ignore this signal iff a sigwait
- * thread isn't used. - mvsk
-
-#ifdef SIGWINCH
- apr_signal(SIGWINCH, SIG_IGN);
-#endif
- */
-
- r->request_time = apr_time_now();
- r->the_request = apr_pstrdup(r->pool, l);
- r->method = ap_getword_white(r->pool, &ll);
-
-#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
- uri = ap_getword_white(r->pool, &ll);
-
- /* Provide quick information about the request method as soon as known */
-
- r->method_number = ap_method_number_of(r->method);
- if (r->method_number == M_GET && r->method[0] == 'H') {
- r->header_only = 1;
- }
-
- ap_parse_uri(r, uri);
-
- /* ap_getline returns (size of max buffer - 1) if it fills up the
- * buffer before finding the end-of-line. This is only going to
- * happen if it exceeds the configured limit for a request-line.
- */
- if (len > r->server->limit_req_line) {
- r->status = HTTP_REQUEST_URI_TOO_LARGE;
- r->proto_num = HTTP_VERSION(1,0);
- r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
- return 0;
- }
-
- if (ll[0]) {
- r->assbackwards = 0;
- pro = ll;
- len = strlen(ll);
- } else {
- r->assbackwards = 1;
- pro = "HTTP/0.9";
- len = 8;
- }
- r->protocol = apr_pstrndup(r->pool, pro, len);
-
- /* XXX ap_update_connection_status(conn->id, "Protocol", r->protocol); */
-
- /* Avoid sscanf in the common case */
- if (len == 8 &&
- pro[0] == 'H' && pro[1] == 'T' && pro[2] == 'T' && pro[3] == 'P' &&
- pro[4] == '/' && apr_isdigit(pro[5]) && pro[6] == '.' &&
- apr_isdigit(pro[7])) {
- r->proto_num = HTTP_VERSION(pro[5] - '0', pro[7] - '0');
- } else if (2 == sscanf(r->protocol, "HTTP/%u.%u", &major, &minor)
- && minor < HTTP_VERSION(1,0)) /* don't allow HTTP/0.1000 */
- r->proto_num = HTTP_VERSION(major, minor);
- else
- r->proto_num = HTTP_VERSION(1,0);
-
- return 1;
-}
-
static void get_mime_headers(request_rec *r)
{
char field[DEFAULT_LIMIT_REQUEST_FIELDSIZE + 2]; /* getline's two extra */
@@ -556,9 +446,6 @@
r->connection = conn;
r->server = conn->base_server;
- conn->keptalive = conn->keepalive == 1;
- conn->keepalive = 0;
-
r->user = NULL;
r->ap_auth_type = NULL;
@@ -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;
+ }
r->per_dir_config = r->server->lookup_defaults;
r->sent_bodyct = 0; /* bytect isn't for body */
@@ -584,30 +473,10 @@
r->output_filters = conn->output_filters;
r->input_filters = conn->input_filters;
- apr_setsocketopt(conn->client_socket, APR_SO_TIMEOUT,
- (int)(conn->keptalive
- ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
- : r->server->timeout * APR_USEC_PER_SEC));
-
ap_add_output_filter("BYTERANGE", NULL, r, r->connection);
ap_add_output_filter("CONTENT_LENGTH", NULL, r, r->connection);
ap_add_output_filter("HTTP_HEADER", NULL, r, r->connection);
- /* 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 r;
- }
- return NULL;
- }
- if (r->connection->keptalive) {
- apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
- (int)(r->server->timeout * APR_USEC_PER_SEC));
- }
if (!r->assbackwards) {
get_mime_headers(r);
if (r->status != HTTP_REQUEST_TIME_OUT) {
@@ -645,8 +514,6 @@
/* we may have switched to another server */
r->per_dir_config = r->server->lookup_defaults;
-
- conn->keptalive = 0; /* We now have a request to play with */
if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1,1))) ||
((r->proto_num == HTTP_VERSION(1,1)) &&
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Take two on the conn_rec change
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Apr 17, 2001 at 08:09:49PM -0700, rbb@covalent.net wrote:
> On Tue, 17 Apr 2001, Greg Stein wrote:
>...
> > +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 :-)
>
> As I said when I moved it, it was a guess. Things are likely to move back
> and forth for a little while.
Yup. That wasn't a problem from my standpoint, but just describing my reason
for a +1, while we're all talking about destabilizing :-).
>...
> > > +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)
>
> The only other file that uses this structure is mod_log_config, and that
> is going to change later tonight. I would rather use the structure,
> because it makes it much easier to move stuff into this structure later.
> I hope that more people start to look at the stuff in the conn_rec, and
> move things out of it as soon as they can.
Hmm. All righty. I took a look, and another easy call about be
"vhost_lookup_data". "remain" shouldn't even exist :-). The filters and
notes could probably go, but they're a bit more difficult to justify.
Not really related to the move:
Stuff like the remote_ip and _host are probably derivatives of remote_addr,
right? Same for local_ip and _host. Although the latter is kind of
surprising since the base_server probably has that info.
> > > @@ -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).
>
> DECLINE means that the current module isn't handling the request, but the
> server keeps calling the other modules. At this point in the code, we can
> not receive DECLINED.
Sure. I meant that if all modules DECLINEd, then you'd have to punt. (which
is exactly what happens now)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Take two on the conn_rec change
Posted by rb...@covalent.net.
On Tue, 17 Apr 2001, Greg Stein wrote:
> Okay... I think the patch is fine. Just nits at this point.
I'll fix most of the nits before I actually commit.
> +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 :-)
As I said when I moved it, it was a guess. Things are likely to move back
and forth for a little while.
> 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.
Nothing changed, but I changed the name, which I am going to change it
back, since it is still a static function.
> >...
> > +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"
I used unsigned, because it was unsigned in the structure.
> > --- 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)
The only other file that uses this structure is mod_log_config, and that
is going to change later tonight. I would rather use the structure,
because it makes it much easier to move stuff into this structure later.
I hope that more people start to look at the stuff in the conn_rec, and
move things out of it as soon as they can.
> > @@ -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).
DECLINE means that the current module isn't handling the request, but the
server keeps calling the other modules. At this point in the code, we can
not receive DECLINED.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Take two on the conn_rec change
Posted by Greg Stein <gs...@lyra.org>.
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/