> It no workee. (timeout handling; see below)
Argh! I fixed this once a long time ago, but now that fix is gone in both
of my trees. Looking at it again. :-(
> > +static int http_create_request(request_rec *r)
> > +{
> > + http_conn_rec *conn = ap_get_module_config(r->connection->conn_config,
>&http_module);
>
> Nit. Please use "hconn" to distinguish this from the "old" conn. It's hard
> to read, when I keep thing conn_rec.
done
> > + conn = apr_palloc(r->pool, sizeof(*conn));
>
> Maybe use pcalloc here? Certainly, for future-proofing.
done
> Take a look at server/protocol.c, where this stuff came from. In between the
> two sets of the timeout, you'll see it goes to the socket to read the
> request line. Basically, it sets the timeout to <something>, reads a line,
> then resets it to <something>.
>
> In the above code, it simply sets it a couple times and exits. Not very
> effective :-)
I'm fixing this now. I'll repost when I am done. This is part of the
problem with splitting a large patch into a couple of small patches. Not
that I am convinced it works correctly in the large patch, but I know I
tackled this exact bug back when I first wrote the large patch. Expect
the new patch later today.
> >...
> > --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> > +++ modules/http/mod_core.h 2001/04/17 03:53:23
>
> Hrm. It would be nice to rename this to mod_http.h before we get too
> attached to the mod_core.h name. (totally unrelated to this patch, but just
> raising it while I'm thinking of it)
I agree 100%
> > +typedef struct http_conn_rec http_conn_rec;
>
> If you're introducing a new structure, then please namespace-protect it. No
> sense digging a deeper hole :-)
I had kind of thought that just namespace protecting it with http_ was
okay. However, now that you mention it, I should use ap_http_, so I have
done that now.
> However, please note that we can/should simply toss the darned bit fields
> from conn_rec, and certainly this one.
I seriously disagree with this. bit fields are incredibly useful, and we
should take advantage of them, not dump them.
> > #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
> > + http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
> > + &http_module);
> > +#endif
>
> Oof! It would be nice to keep the http_conn_rec private. This kinda sucks.
I disagree, this REALLY sucks. :-) I think I know how to fix it, but it
is a relatively large change, so I really don't want to make it here.
What I believe we should basically do, is register an optional function
that takes a format specifier and a function to call. Basically, the same
thing we do for mod_include and mod_cgi. This is a much more extensible
option.
> > r->connection = conn;
> > r->server = conn->base_server;
> >
> > - conn->keptalive = conn->keepalive == 1;
>
> Okay... about conn->keptalive.
finally. :-)
> conn_rec, but used *only* in this function.
>
> Rather than shift it from conn_rec to http_conn_rec, let's just switch it to
> a local variable here(!)
Done!!!! Great Catch Greg!
Ryan
_______________________________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
