Return-Path: 
 <new-httpd-return-15303-apmail-new-httpd-archive=apache.org@apache.org>
Delivered-To: apmail-new-httpd-archive@apache.org
Received: (qmail 90434 invoked by uid 500); 17 Apr 2001 16:54:34 -0000
Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
Precedence: bulk
Reply-To: new-httpd@apache.org
list-help: <mailto:new-httpd-help@apache.org>
list-unsubscribe: <mailto:new-httpd-unsubscribe@apache.org>
list-post: <mailto:new-httpd@apache.org>
Delivered-To: mailing list new-httpd@apache.org
Received: (qmail 90225 invoked from network); 17 Apr 2001 16:54:32 -0000
Date: Tue, 17 Apr 2001 09:55:54 -0700 (PDT)
From: <rbb@covalent.net>
X-Sender: <rbb@koj.rkbloom.net>
To: <new-httpd@apache.org>
Subject: Re: Multi-protocol patch, part 1
In-Reply-To: <005201c0c75d$35506100$7f00a8c0@VAIO>
Message-ID: <Pine.LNX.4.30.0104170950500.28350-100000@koj.rkbloom.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
X-Spam-Rating: h31.sny.collab.net 1.6.2 0/1000/N


I don't think we are pulling in two directions.  I think that increasing
the modularity of the server is a good thing for stability and
maintainability.

I am trying very hard to make the patches very small and thus to not
impact the workings of the server too much.

We have said many times that we all work on what is important to us.  We
have also said that this is a multi-protocol server.  If 2.0 is going to
be multi-protocol, then we need to have the discussion.  If it isn't, then
we need to stop saying it is.

I am 100% against holding up a beta for this stuff.  If we can keep the
patches incredibly small and reviewable, then there is no reason that they
have to destabilize the server.

As for being blunt, isn't that why we all get along?  If anybody on this
list doesn't think they can speak their minds freely and bluntly, then
things just don't work.  :-)

Ryan

On Tue, 17 Apr 2001, David Reid wrote:

> Have we agreed that the multi-protocol stuff that was outlined (and this
> patch is based on) is the correct way to go?  I'm not convinced that it is.
> I REALLY dislike the fact that we appear to be pulling in 2 directions at
> the moment.
>
> One group is trying to get 2.0 bug fixed and ready for release, the other
> trying to destabilize it and insert new potential for bugs and holes!!  If
> the multi-protocol stuff is a *MUST* have for a 2.0 release then we should
> have some real discussions and stop worrying about rolling "beta's".
>
> If it's not a must have then perhaps we should stop worrying about this and
> make it an issue for 2.1?  Sorry to be blunt.
>
> david
> ----- Original Message -----
> From: <rbb@covalent.net>
> To: <new-httpd@apache.org>
> Sent: Tuesday, April 17, 2001 4:34 PM
> Subject: Re: Multi-protocol patch, part 1
>
>
> >
> > > 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                        rbb@apache.org
> > 406 29th St.
> > San Francisco, CA 94131
> > --------------------------------------------------------------------------
> -----
> >
>
>


_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------