Re: cvs commit: httpd-2.0/modules/ssl mod_ssl.c mod_ssl.h ssl_engine_ext.c ssl_engine_vars.c ssl_expr_parse.c ssl_expr_parse.h ssl_expr_scan.c ssl_scache.c
On Wed, Aug 22, 2001 at 10:58:08PM -, [EMAIL PROTECTED] wrote: > wrowe 01/08/22 15:58:08 > > Modified:modules/ssl mod_ssl.c mod_ssl.h ssl_engine_ext.c > ssl_engine_vars.c ssl_expr_parse.c ssl_expr_parse.h > ssl_expr_scan.c ssl_scache.c > Log: > Remove a ton o' cruft. Moves the mod_log_config 'var' extensions to > ssl_engine_vars.c. >... > --- ssl_expr_parse.c2001/05/05 13:21:53 1.3 > +++ ssl_expr_parse.c2001/08/22 22:58:07 1.4 >... > + > +/* A Bison parser, made from ssl_expr_parse.y > +by GNU Bison version 1.28 */ Why do we have Bison output in our tree? Per our standard guidelines, all generated files should stay out of the tree... >... > --- ssl_expr_scan.c 2001/08/22 15:40:28 1.4 > +++ ssl_expr_scan.c 2001/08/22 22:58:07 1.5 > @@ -18,6 +18,7 @@ >/* A lexical scanner generated by flex */ > >/* Scanner skeleton version: > + * $Header: /home/cvs/httpd-2.0/modules/ssl/ssl_expr_scan.c,v 1.5 2001/08/22 >22:58:07 wrowe Exp $ > */ Same here. And we probably don't want that $Header$ in there, either. I would think the Bison/flex output would be generated during buildconf (rather than a Makefile) so that we don't have to expect users to generate them at build time (meaning they would have to have Bison/flex). Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Letting Table store non-char data
On Wed, Aug 22, 2001 at 03:59:56PM -0700, Ian Holsman wrote:
> On Wed, 2001-08-22 at 15:41, Brian Pane wrote:
> > Ian Holsman wrote:
> > >On Wed, 2001-08-22 at 14:12, Ryan Bloom wrote:
> > >>We had binary tables, but we removed them, because that is what hash
> > >>tables are for.
> > >>
> > >agreed.
> > >but there are no hash tables on the request rec structure.
Yes there is. apr_pool_userdata_set(..., r->pool)
> > >My problem is that I have a module which implements a custom SSI tag and
> > >a 'postfix' hook.
> > >the postfix hook sets up the key based on the incoming request.
apr_pool_userdata_set("ian-ssi-tag-data", data, apr_pool_cleanup_null,
r->pool);
> > >when the SSI tag gets called (~10-50 per page) it needs to get the key
> > >the postfix hook had set up.
apr_pool_userdata_get(&data, "ian-ssi-tag-data", r->pool);
>...
> > How about changing request_rec->notes to a hash table? This would be
> > a relatively simple change, as the notes field isn't used in a huge
> > number of places in the standard modules.
No need. IMO, r->notes should just go away. It is pointless nowadays.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Letting Table store non-char data
On Wed, Aug 22, 2001 at 10:38:44PM -0700, Brian Pane wrote: > Greg Stein wrote: >... > >Yes there is. apr_pool_userdata_set(..., r->pool) >... > Using the "userdata" functions on r->pool as a replacement for a > hash-based r->notes would be a mistake. The interface to the userdata > in a pool is limited to "get" and "set" methods. The API is missing > essential operations like "iterate over the set of elements in the userdata" > and "merge the userdata for a subrequest pool into the parent's r->pool." I would posit those operations are not needed. The notes or userdata are for specific bits of information. It is not a simple collection that you can iterate over. In fact, since you can't know the values of each key, you cannot perform a useful iteration nor do a useful merge. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Letting Table store non-char data
On Thu, Aug 23, 2001 at 01:22:00AM -0700, Brian Pane wrote: > Greg Stein wrote: > >I would posit those operations are not needed. The notes or userdata are for > >specific bits of information. It is not a simple collection that you can > >iterate over. In fact, since you can't know the values of each key, you > >cannot perform a useful iteration nor do a useful merge. > > > Hmmm...I think I agree. I do have some cases in which the ability to > iterate over the notes table is essential for sharing extensible sets > of data among modules, but they depend on naming conventions for the > elements in the table (so that the collaborating modules know what elements > are relevant and can interpret the values meaningfully). But it's a > lot cleaner to store this data in an appliation-specific data structure > that in turn is stored as a userdata element in the pool. Yup. You could store an app-specific hash table in the userdata, and assign specific semantics to the use of that structure (including things like arbitrary scanning of its contents). That would allow for the operations you're thinking of. But just iterating over the userdata in a pool could be a problem. Heck... the key might not even be a valid pointer :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: 2.0.24 ready for beta?
On Thu, Aug 23, 2001 at 02:53:03PM -0400, Greg Ames wrote: >... > However, the bugs are getting more subtle and take longer to debug and > fix. With our current process, a great deal of new code can be > committed while the gnarly problem in last tarball is being debugged. > Why would we think that the new code is any less likely to contain > serious bugs than the previous set of changes? How do we get off this > treadmill? We got "off the treadmill" by stopping this silly business of holding up tarballs. Snap a tarball. Give it a once over. Release it as an alpha. Bam. Done. Come back a week later and upgrade it to a beta. Not so hard. We are right back where we were last year: releases take forever. We have completely lost the "snap. release." routine. Releases of *any* quality can be happening every couple weeks or even faster. But since everybody is "oh no, it *MUST* be a beta" we're waiting forever. It's silly. If you want a release, then make a release. Label it as beta or gold or whatever later. Nobody said we can't change the quality level over time. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Letting Table store non-char data
On Thu, Aug 23, 2001 at 07:50:03AM -0500, William A. Rowe, Jr. wrote: > From: "Greg Stein" <[EMAIL PROTECTED]> > Sent: Thursday, August 23, 2001 2:37 AM > > > > On Wed, Aug 22, 2001 at 10:38:44PM -0700, Brian Pane wrote: > > > Greg Stein wrote: > > >... > > > >Yes there is. apr_pool_userdata_set(..., r->pool) > > >... > > > Using the "userdata" functions on r->pool as a replacement for a > > > hash-based r->notes would be a mistake. The interface to the userdata > > > in a pool is limited to "get" and "set" methods. The API is missing > > > essential operations like "iterate over the set of elements in the userdata" > > > and "merge the userdata for a subrequest pool into the parent's r->pool." > > > > I would posit those operations are not needed. The notes or userdata are for > > specific bits of information. It is not a simple collection that you can > > iterate over. In fact, since you can't know the values of each key, you > > cannot perform a useful iteration nor do a useful merge. > > Precisely. > > What negative consequences would that pose for subrequests? Um. None... we have zero handling in there for any kind of munging or manipulation of r->notes or subreq->notes. It is all simple get/set. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/mappers mod_negotiation.c
On Thu, Aug 23, 2001 at 11:54:46PM -0700, Roy T. Fielding wrote: > > who's job is it to answer an M_OPTIONS request? Every module's? Then the mod_cgi > > family is broken (this is what led me to discover that viewcvs.cgi hangs on HEAD, > > maybe other cgi's are hanging, as well.) > > In this case, it is the responsibility of each CGI. That has always been > the case for CGI -- it is a gateway interface. ISTR that CGI scripts would never be passed the OPTIONS methods. So it would be impossible for it to be "the responsibility of each CGI." :-) I ran into this when somebody asked about writing a DAV server using CGI. They never got the OPTIONS methods... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .libs/httpd
On Fri, Aug 24, 2001 at 12:15:17AM -0700, Doug MacEachern wrote: > On Thu, 23 Aug 2001, Roy T. Fielding wrote: > > > As I recall, libtool modifies the runtime library path within the exec > > when it installs the file, or something like that. > > libuncooltool. i didn't realize libapr was now linked as a shared > library. that explains everything. You could probably pass --disable-shared at Apache's config line. Of course, then *everything* would be built statically and you'd get a big ass blob of an executable :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: appending to the content brigade
On Fri, Aug 24, 2001 at 04:00:46PM -0400, Cliff Woolley wrote:
> On Fri, 24 Aug 2001, Eric Prud'hommeaux wrote:
>...
> > > Note that if the last bucket is not eos, depending on how your filter is
> > > supposed to work that might be an indication that you need to stash the
> > > brigade away until you DO get an eos, because the content you're wrapping
Okay... let's back up. I believe that some poor advice may have been given.
For now, let's assume that bPayload *does* have the entire content. Given
that, there is *no* reason to copy the darn payload into a new brigade.
The whole *idea* of the filters and the brigades is that you modify the
brigade passed to you, and then pass it on down. Thus, your operation is
like this:
#define BOUNDARY " boundary "
/* insert into head in reverse order (like pushing a stack) */
bucket = apr_bucket_immortal_create(BOUNDARY, sizeof(BOUNDARY)-1);
APR_BRIGADE_INSERT_HEAD(bPayload, bucket); /* boundary of data/payload */
my_data_bucket = ...
APR_BRIGADE_INSERT_HEAD(bPayload, my_data_bucket);
bucket = apr_bucket_immortal_create(BOUNDARY, sizeof(BOUNDARY)-1);
APR_BRIGADE_INSERT_HEAD(bPayload, bucket); /* first boundary */
/* insert a boundary before the EOS bucket */
eos = APR_BRIGADE_LAST(bPayload);
APR_BRIGADE_INSERT_BEFORE(eos, bucket);
return ap_pass_brigade(f->next, bPayload);
Construction of the boundary is probably dynamic, and I'd bet you have much
more data to insert. But the concept demonstrated above is how you "should"
deal with the passing brigade, and how to pass it along.
Copying the brigade is *not* the answer, if you can avoid it.
Now... the second point is that, as Cliff said, you may not get the EOS in
your brigade. Not a problem. On the *first* invocation of your filter,
insert the stuff up front and pass it along. (record the "first" in a
filter-specific context structure). When you see an EOS bucket at the end of
the brigade (using APR_BRIGADE_LAST and APR_BUCKET_IS_EOS), then you insert
the final boundary.
If your boundary is dynamically constructed, the store the string into your
context (to reuse on the various invocations of your filter). When you go to
insert it, wrap it in a POOL bucket, using whatever pool your boundary was
allocated in (to ensure the lifetimes match).
>...
> > Oops, I forgot to include the code snippet to which I was
> > referring. From ap_byterange_filter:
>...
> Hooo, that's ugly. You don't have to do that for what you want. The
> reason it's done here is that byteranges might overlap within the same
> request, so rather than just moving buckets to where they need to go, it
> has to make copies of all the requisite buckets for each of the ranges.
To be clearer, the client might request the same range multiple times. Thus,
we need multiple copies of the buckets.
[ and yes: we could optimize the thing to avoid copying by analyzing the
ranges up front, determining where overlaps may occur, and only copy those
portions; thus using the original brigade as much as possible, without any
copies.
that said: many buckets are very efficient at copying because of the whole
refcount and sub-bucket stuff. ]
>...
> heap) and then THAT can be copied. Actually, the block here is broken
> because it puts str into two buckets at a time, which will double-free
> str when the second bucket gets destroyed. It should read:
>
> if ((rv = apr_bucket_copy(ec, &foo)) != APR_SUCCESS) {
> if (rv == APR_ENOTIMPL) {
> /* morph to a copyable bucket by reading */
> apr_bucket_read(ec, &str, &len, APR_BLOCK_READ);
> apr_bucket_copy(ec, &foo);
Or, it could simply pass 1 to heap_create to COPY the data passed. That *is*
what that parameter is for :-)
> > foo = apr_bucket_heap_create(str, len, 0, NULL);
becomes:
> > foo = apr_bucket_heap_create(str, len, 1, NULL);
>...
> That way, ec and foo end up as two apr_buckets of type heap that share a
> single apr_bucket_heap structure which points to str with a refcount of
> 2, rather than having two entirely separate apr_bucket/apr_bucket_heap
> pairs both of which point to str and each of which has a refcount of
> 1.
But that is the cooler solution :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http http_protocol.c
On Fri, Aug 24, 2001 at 04:39:32PM -0400, Cliff Woolley wrote: >... > It just occurred to me that by the time we arrive at this particular > block, the byterange filter should have normalized all such buckets > because it calls apr_brigade_length(), which reads in all buckets of > indeterminate length. So this block is probably never reached, but it's > worthwhile to have it right anyway. It should be right. It would be very easy for a bucket to be based on a pipe or a socket, thus being uncopyable, but it knows the length of data to be read from that pipe/socket. Imagine if you have a particular protocol running over some socket to a backend server. You know the next chunk of data is 1000 bytes. You insert your custom socket bucket, saying " socket, 1000 bytes". It can be read, but it can't be copied. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/include http_core.h
On Fri, Aug 24, 2001 at 04:24:35PM -0500, William A. Rowe, Jr. wrote: >... > I'm not approaching this again until I've slept on it. I have the patch > essentially finished, adding a per-'dir' (proxy location, really) pattern > of p, with a isregex flag of r. That's about as complicated as it gets. I'm not sure that I follow what the pattern thing is all about. >... > There will be no more 'faux' filenames. It's either an absolute path, or > undefined (and irrelevant.) A few modules will get to deal with in anyways > (such as mod_mime.) > > I'll start tommorow and wrap up tommorow, so everyone can take a look at what > else this impacts. But getting rid of 'specials' is the only way to clean out > directory_walk of all it's very heavy cruft. Note that r->filename and friends (finfo, canonical_filename, etc) are all related to the filesystem. As such, they are all completely bogus when you're talking about purely virtual resources (such as /server-status or content coming from a database). Thus, they should not be part of the request_rec, but instead part of a private structure to something like mod_filesystem. That mod_filesystem would implement the various translation and other fs-related items that occur. And no... I'm not suggesting this happen any time soon :-) But it will one day, if I can figure out how to tackle it and can set aside the time. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .libs/httpd
On Fri, Aug 24, 2001 at 02:42:30PM -0700, Ryan Bloom wrote: >... > Because it is absolutely bogus that I have to choose whether EVERYTHING > in the build is static or dynamic. I should be able to configure APR to be built > as a static lib, but still have shared modules. Then you should have stopped the "one libtool for everybody" push. As long as we have a single libtool, and that thing is responsible for building static and/or dynamic outputs, then we can only build in the way specified by that one libtool. Then you could build APR statically, and the rest of the system shared. Of course, this also assumes that httpd-2.0/configure has the right set of switches to prepare such a system. But without the configure switches, you have no hope to do it anyways. So you aren't even at a libtool problem yet. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .libs/httpd
On Fri, Aug 24, 2001 at 08:27:05AM -0700, Ryan Bloom wrote: > On Friday 24 August 2001 00:15, Doug MacEachern wrote: > > On Thu, 23 Aug 2001, Roy T. Fielding wrote: > > > As I recall, libtool modifies the runtime library path within the exec > > > when it installs the file, or something like that. > > > > libuncooltool. i didn't realize libapr was now linked as a shared > > library. that explains everything. > > That's because libtool SUCKS! Has nothing to do with libtool. That is just a convenient scapegoat. APR can/should be a shared library. Should have been a long time ago, but we didn't have installation running for it, so we made it a static library. That was turned off recently, making things as they should be. What are you blaming libtool for? Can you state the specific problem? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .libs/httpd
On Fri, Aug 24, 2001 at 08:27:57AM -0700, Ryan Bloom wrote: > On Friday 24 August 2001 00:33, Greg Stein wrote: > > On Fri, Aug 24, 2001 at 12:15:17AM -0700, Doug MacEachern wrote: >... > > > libuncooltool. i didn't realize libapr was now linked as a shared > > > library. that explains everything. > > > > You could probably pass --disable-shared at Apache's config line. Of > > course, then *everything* would be built statically and you'd get a big ass > > blob of an executable :-) > > Please don't do that. Passing --disable-shared is what caused the shared > modules to stop working. Apache should be able to work as one big monolithic executable. Shared modules should still be able to work because Apache would still have DSO loading capabilities. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/include http_core.h
On Fri, Aug 24, 2001 at 06:12:02PM -, [EMAIL PROTECTED] wrote: > wrowe 01/08/24 11:12:02 > > Modified:server core.c request.c >include http_core.h > Log: > sec, sec, who's got a sec? This gave me a headache, but I had to clear > out the last patch before I rearranged this to be _readable_. > > Next step for everyone's sanity, provide directives ;) *THANK YOU* I ran into that crap when I was doing the "ap_conf_vector_t" work. Took a lot of duct tape to repair my head. Just never got a chance to go back and fix the stuff. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: mod_include and POST (fwd)
On Fri, Aug 24, 2001 at 07:45:29PM -0400, Cliff Woolley wrote: >... > I've verified this as a security problem in 2.0. 1.3 seems to return a > 405 Method Not Allowed error when you attempt to POST to a server-parsed > file, so I've attempted to duplicate that behavior as best I can. Does > the following patch look correct? If so, I'll commit. The SSI stuff is a handler in 1.3, so it is the proper guy to state what is handled. And it says "no POST" :-) As a filter in 2.0, mod_include should just stay out of it. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: mod_include and POST (fwd)
On Sat, Aug 25, 2001 at 12:45:13AM -0400, Cliff Woolley wrote:
> On Fri, 24 Aug 2001, William A. Rowe, Jr. wrote:
>
> > don't forget to _permit_ OPTIONS.
>
> It turns out that OPTIONS was more complicated than I expected. By the
> time we get to the includes filter, the default handler has already called
> ap_send_http_options() which is what sets the Allow header and which is
> what called down the filter stack (to us!).
The handler is the thing responding to the request. NOT the filter. The
filter should never touch the allowed methods, nor should it test for which
method was used. The filter has been placed into the filter stack, so it
should run.
If you don't want the filter running, then it should not have gone into
filter stack.
But remember: the *handler* is the guys that knows what methods to respond
to, and how. It should deal with all that.
>...
> > Also, this (AP_METHOD_BIT << M_GET);
> > is (slightly) bogus since there are macros to make it more legible.
>
> Actually, it's _totally_ bogus. r->allowed is now completely useless as
> it turns out. It's still set throughout the server by various modules,
> but it's never used! make_allow() uses r->allowed_methods instead these
> days. r->allowed needs to go away and be replaced by calls to
> ap_allow_methods(), which sets up r->allowed_methods. There might be
> other security problems in various modules due to this problem, where they
> THINK they're limiting the allowed methods and they're really not.
OPTIONS is a difficult problem. There are many facilities that could be
present in the server, and each of these may need a shot at responding to
the OPTIONS request. Not necessarily just the handler who happened to
receive the OPTIONS.
> --- mod_include.c 2001/08/24 06:47:35 1.132
> +++ mod_include.c 2001/08/25 04:34:22
> @@ -2728,9 +2728,17 @@
> if (!(ap_allow_options(r) & OPT_INCLUDES)) {
> return ap_pass_brigade(f->next, b);
> }
> - r->allowed |= (AP_METHOD_BIT << M_GET);
> if (r->method_number != M_GET) {
I say the above lines should simply be removed.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c request.c
On Sat, Aug 25, 2001 at 11:43:19PM -, [EMAIL PROTECTED] wrote: > wrowe 01/08/25 16:43:19 > > Modified:.CHANGES >include http_request.h >modules/http http_core.c http_protocol.c http_request.c > mod_core.h >server core.c request.c > Log: > Introduce the map_to_storage hook, which allows modules to bypass > the directory_walk and file_walk for non-file requests. TRACE How is this different from the translate_name hook? Your new hook and the translate_name are both about translating URLs into the backend storage. Why was this new one added? (did I miss some discussion somewhere?) And if so, then why does translate_name stick around? -- Greg Stein, http://www.lyra.org/
Re: mod_include and POST (fwd)
[ moving to [EMAIL PROTECTED] ]
On Sat, Aug 25, 2001 at 09:53:51PM -0400, Cliff Woolley wrote:
> On Sat, 25 Aug 2001, Brian Pane wrote:
> > However, it also seems to break something else:
> >
> > With this new code, any POST response that goes through the mod_include
> > filter will result in a 405. That's bad news for anybody who needs to
> > stream the output of a POST-based CGI/JSP/etc app through mod_include
> > for SSI processing.
> >
> > Would it make sense to replace:
> > if (r->method_number != M_GET) {
> > with:
> > if ((r->method_number != M_GET) && (r->method_number != M_POST)) {
> > in order to support SSI parsing of dynamically generated reponses to
> > POSTs?
>
> It's fine by me... it was not supported by 1.3, but then again the ability
> to server-parse dynamically generated content is a new feature. I'll make
> the change. It does work, BTW-- I tried it yesterday. It makes the stuff
> to fight with OPTIONS requests a bit redundant, but it's probably good to
> keep it in anyway in case somebody tries to stick mod_include on top of a
> handler that supports more than just GET and POST.
Detecting the method in there is almost always the wrong thing to do. I
could easily imagine a case where a server responds to a XXX method with an
error document that is server-parsed. (say, XXX is MOVE or COPY or
MY-CUSTOM-METHOD)
Hmm... guess I should go read the original motivation for this, but it seems
that whatever the problem was, it should be solved differently.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: dependencies
On Sun, Aug 26, 2001 at 05:15:54PM -0700, Ryan Bloom wrote: > On Sunday 26 August 2001 17:16, Greg Stein wrote: > > On Sun, Aug 26, 2001 at 11:06:55AM -0700, Marc Slemko wrote: >... > > > In 1.3, dependencies were generated periodically and then included in the > > > makefiles... in 2.0, you have no dependencies unless you manually run > > > "make depend" is the checked out tree... is it practical to have that > > > automatically done somewhere? (buildconf?) > > > > The (apparent) consensus around dependencies focused around two points: > > I don't believe there was ever consensus around this. I believe half the developers > wanted to see dependancies in the tree, and the other half didn't. "apparent" ... that was just my interpretation of what came out of those discussions. As you say, some did want them, some didn't. And that's what I said in my two points below :-) IOW, "developers appeared to have consensus in that people worked differently and desired different things, and to address this discrepancy, we should have a system that worked for boths types of developers" > > 1) some developers may/may not want them, so we should accomodate that [by > >not forcing dependency generation] > > > > 2) end users do not require dependencies since they simply unpack and > >compile the server. > > > > Given the above two points, a developer (or a user!) that wants > > dependencies can do a "make depend". Leaving it out of buildconf and > > configure means that the others users are also satisfied. > > I have also never seen those two points before. Of course not. Those points were my summarization of the events that led to the current system. We never wrote them down, voted on them, and held them up as This Is The Way Things Will Be. :-) > The only argument I have > ever seen around not having dependancies in CVS, is that we shouldn't > have generated information stored in CVS. Agreed, and I believe almost everbody would agree, too. The *real* question is whether buildconf or configure should figure out dependencies or not. configure is probably a Bad Thing because there isn't much reason for end-users to have the dependencies recomputed every time they run the darn thing. buildconf is really the place to do it. *If* we want to impose dependency generation and use on all developers. That last sentence is the cruz :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: chunking of content in mod_include?
On Mon, Aug 27, 2001 at 08:24:50PM -0700, Ryan Bloom wrote: >... > > This code was put in because we were seeing the mod_include code buffer up > > the entire collection of buckets until an SSI tag was found. If you have a > > 200 MB file with an SSI tag footer at the end of the brigade, the whole > > thing was being buffered. How do you propose that this be done differently? > > I don't care if mod_include buffers 200 Megs, as long as it is constantly doing > something with the data. If we have a 200 Meg file that has no SSI tags in > it, but we can get all 200 Meg at one time, then we shouldn't have any problem > just scanning through the entire 200 Megs very quickly. Worst case, we do what > Brian suggested, and just check the bucket length once we have finished > processing all of the data in that bucket. The buffering only becomes a > real problem when we sit waiting for data from a CGI or some other slow > content generator. It becomes a problem if you load too much of that file into the heap. Thus, we have the threshold. The threshold continues to make sense. Using a bucket boundary is a very good optimization. If a FILE bucket manages to map the whole sucker into memory, and returns one massive 200M memory buffer to scan ... fine. But after that scan, we probably ought to deliver it down the stack :-) But if that FILE continues spitting out 8k HEAP buffers, then we also need to flush them out of memory. Brian's recently posted patch looks good for this. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Apache 1.3.21-dev for NetWare builds (5)
On Mon, Aug 27, 2001 at 04:08:25PM +0200, Pavel Novy wrote: >... > I agree this is an undesirable way for bitfields to be aligned, but this > agrees with MSVC, upon which we based the bitfield layout. (In MSVC the > smallest we can make the struct is 4 bytes!) > > I think the way to work around this is to change the bitfield base type to > "unsigned char". The type of the bitfield is used in determining how it is > aligned; thus, using a smaller type will require a smaller alignment. >... > --- util_uri.h.orig Mon Feb 26 16:49:32 2001 > +++ util_uri.hMon Aug 27 15:44:41 2001 > @@ -106,10 +106,10 @@ > > unsigned short port; /* The port number, numeric, valid only if port_str != >NULL */ > > -unsigned is_initialized:1; > +unsigned char is_initialized:1; > > -unsigned dns_looked_up:1; > -unsigned dns_resolved:1; > +unsigned char dns_looked_up:1; > +unsigned char dns_resolved:1; > > } uri_components; Screw the bitfields. Just change all of them to plain old unsigned chars. (and apr_byte_t in APR). There is no reason to be miserly with bits here. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] worker MPM: reuse transaction pools
On Mon, Aug 27, 2001 at 05:09:01PM -0700, Aaron Bannert wrote: > This patch implements a resource pool of context pools -- a queue of > available pools that the listener thread can pull from when accepting > a request. The worker thread that picks up that request then uses > that pool for the lifetime of that transaction, clear()ing the pool > and releasing it back to what I'm calling the "pool_queue" (har har). > This replaces the prior implementation that would create and destroy > a transaction pool for each and every request. > > I'm seeing a small performance improvement with this patch, but I suspect > the fd_queue code could be improved for better parallelism. I also > suspect that with better testing this algorithm may prove more scalable. What does "small" mean? I can't believe it is all that large. Pool construction/destruction is actually quite fast. The bulk of the time is clearing the pool, which you must do anyways. I don't see how a pool queue can provide any benefit. IOW, why should this complexity be added? Just how much does it improve things, and are you testing on a single or multi processor machine? Cheers, -g p.s and yes, I know Ryan just applied it, but that doesn't mean it should stay there :-) -- Greg Stein, http://www.lyra.org/
Re: [PATCH] worker MPM: reuse transaction pools
On Tue, Aug 28, 2001 at 01:01:11PM -0700, Ryan Bloom wrote: > On Tuesday 28 August 2001 12:17, Greg Stein wrote: > > On Mon, Aug 27, 2001 at 05:09:01PM -0700, Aaron Bannert wrote: > > > This patch implements a resource pool of context pools -- a queue of >... > > IOW, why should this complexity be added? Just how much does it improve > > things, and are you testing on a single or multi processor machine? > > Creating a pool requires locking a mutex. The more we scale, the worse we > perform. And manipulating the pool queue also requires a mutex, right? You can't just let people arbitrarily hit that queue. Locking is needed. Thus... why have the complexity of a queue? I don't see where the saving happens. We still have locks, we still have pool clearing, etc. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: time for 2.0.25?
On Wed, Aug 29, 2001 at 11:30:53AM -0700, Ryan Bloom wrote: > The tarballs are up now! Let's get to beta this time,PLEASE If it isn't beta quality, then release it as alpha. But it should be released, no matter what. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: time for 2.0.25?
On Wed, Aug 29, 2001 at 01:54:42PM -0700, Ryan Bloom wrote: > On Wednesday 29 August 2001 13:42, Ian Holsman wrote: >... > > Solaris 8/Sparc runs. > > but PROXY generates a core dump > > when i expand have a SSI tag printenv in it. > > Proxy is not currently a part of the Apache 2.0 dist, so I don't consider > this seg fault a showstopper. Although, when we roll mod_proxy into > 2.0.25, and thus tag mod_proxy, this should be fixed. Agreed. We should release 2.0.25 independently of mod_proxy. In fact, we could release 2.0.25 as a beta, and the combo as an alpha. Or we could get proxy to work against the tagged-25 and then release the combo as beta. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: 2.0.25 on FreeBSD 4.2-R -- 404 returns text/plain error page
On Wed, Aug 29, 2001 at 06:01:35PM -0700, Marc Slemko wrote: > On Wed, 29 Aug 2001, Justin Erenkrantz wrote: > > On Wed, Aug 29, 2001 at 04:14:39PM -0600, Charles Randall wrote: > > > Using the prefork mpm on FreeBSD 4.2-R with a default installation > > > (./configure --prefix=/my/full/path -with-port=8080) a request for a > > > non-existant page (E.g., /bogus) returns the HTML error document as > > > text/plain. > > > > Yup, that's a bug. I don't *think* it is a showstopper though. I can > > Erm... it is a showstopper from making it beta, that's for sure. No big deal. Let's release it as an alpha. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Add -X back in...
On Wed, Aug 29, 2001 at 10:06:20PM -0700, Justin Erenkrantz wrote: > Assuming no one vetoes this, this can get committed before we tag > tomorrow. No rush though. =-) > > - Updates upgrading.html > - Reverts Aaron's earlier docco patch (sorry...) > - Adds -X to all mpms in the tree > > I won't be around tomorrow morning to commit this before 2.0.26 is > tagged. If it doesn't make it into 2.0.26 and no one has vetoed it, > then I'll commit it on Friday. -- justin This continues to use the "defines" mechanism, so I'm happy. +1 Cheers, -g (i.e. changing it to the hack that used to be there -- some kind of global that each MPM had to deal with... that was ooky; this patch looks good) -- Greg Stein, http://www.lyra.org/
Re: [PATCH] RE: make distclean doesn't
On Fri, Aug 31, 2001 at 03:02:32PM -0700, Ryan Bloom wrote: >... > exports.c shouldn't be cleaned, correct, because it is a part of the > distribution, or at least it should be if it isn't already. config.nice is > not a part of the distribution however, and should be removed by > make distclean. -1 on *any* form of clean that tosses config.nice That holds *my* information about how I repeatedly configure Apache. That is a file that I use, and is outside of the scope of the config/build/whatever processes. Its entire existence is to retain the information. Cleaning it is not right. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] RE: make distclean doesn't
On Fri, Aug 31, 2001 at 09:16:15PM -0700, Ryan Bloom wrote: > On Friday 31 August 2001 19:31, William A. Rowe, Jr. wrote: > > From: "Greg Stein" <[EMAIL PROTECTED]> > > Sent: Friday, August 31, 2001 9:30 PM > > > > > On Fri, Aug 31, 2001 at 03:02:32PM -0700, Ryan Bloom wrote: > > > >... > > > > exports.c shouldn't be cleaned, correct, because it is a part of the > > > > distribution, or at least it should be if it isn't already. > > > > config.nice is not a part of the distribution however, and should be > > > > removed by make distclean. > > > > > > -1 on *any* form of clean that tosses config.nice > > > > > > That holds *my* information about how I repeatedly configure Apache. That > > > is a file that I use, and is outside of the scope of the > > > config/build/whatever processes. Its entire existence is to retain the > > > information. Cleaning it is not right. > > > > What are you talking about? We are talking about cleaning for packaging to > > _other_ computers, not yours. That's what rbb is speaking of by > > 'distclean', clean enough for redistribution. > > Exactly. The whole point and definition of make distclean, is that it cleans > things to the point that it could be redistributed to another machine. If you > are just trying to clean the directory, then make clean is what you want. If > make clean doesn't remove enough for you, then something else is wrong. I use distclean on my computer all the time. Along with extraclean. Neither of those targets should toss config.nice. *That* is what I mean. To be clear: nothing in our build/config/whatever should remove config.nice "Clean" rules are about cleaning out state that might affect a build in some way. So we toss object files, generate makefiles, the configure script, whatever. But config.nice doesn't fall into that camp because it is not a "stateful" file. It is for the user to rebuild what they had before. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Add mod_gz to httpd-2.0
On Sat, Sep 01, 2001 at 07:50:19PM -0700, Ryan Bloom wrote: > On Saturday 01 September 2001 18:53, Cliff Woolley wrote: > > On Sat, 1 Sep 2001, Ryan Bloom wrote: >... > > > 2) I keep hearing that zlib has more memory leaks than a sieve. > > > > Maybe it does, but that can be dealt with. Even so, it shouldn't be a > > consideration here IMO, at least not yet. If it really is leaky (which it > > very well might be but we should prove it rather than just going on what > > we heard), then it's a consideration, but it'd be better for somebody to > > just *fix* zlib than for us to throw out both mod_gz and mod_gzip because > > of zlib's deficiencies (assuming we care, which we probably do). > > I disagree that this shouldn't be a consideration. If we are distributing a module > that relies on a library that leaks, then we are suggesting people use a leaking > library. I would be fine fixing zlib, but if that can't be done, then a memory leak > in zlib would make me a -0.5 very quickly. We ship code that uses it. zlib fixes their code (or somebody else fixes it). Now our module rocks. Chicken and egg... A ton of other projects use zlib. I see no reason for us to avoid it. If it *does* happen to have leaks, then (when somebody finds this to be true) they could fix it. > > > 3) I don't believe that we should be adding every possible module to > > > the core distribution. I personally think we should leave the core as > > > minimal as possible, and only add more modules if they implement a > > > part of the HTTP spec. The gzip content encoding is part of the HTTP spec. > > My personal opinion is that this one is important enough that it should go > > in. Most clients support gzip transfer coding, and it's a very real > > solution to the problem of network bandwidth being the limiting factor on > > many heavily-loaded web servers and on thin-piped clients (read: modem Agreed! > > users). mod_gz(ip) could provide a significant throughput improvement in > > those cases, where the CPU is twiddling its thumbs while the network pipe > > is saturated. This fills a gap in Apache that could be a very big deal to > > our users. (It's not like it's a big or obtrusive module either, but size > > is not the final consideration in what goes in and what doesn't.) > > You know what's really funny? Every time this has been brought up before, > the Apache core has always said, if you want to have gzip'ed data, then > gzip it when you create the site. That way, your computer doesn't have to > waste cycles while it is trying hard to serve requests. I personally stand by > that statement. If you want to use gzip, then zip your data before putting it > on-line. That doesn't help generated pages, but perl can already do gzip, as > can PHP. But it isn't "invisible" if you do it with Perl, PHP, Python, or CGI. A person has to explicitly code it. I'm really looking forward to mod_gz(ip) in Apache 2.0 so that Subversion can transfer its content in compressed form. All of that comes out of a database... it can't be precompressed, so that leaves a filter to do the job as it hits the wire. Doing large checkouts are almost *always* network bound rather than server/client bound. Compressing those files is a *huge* win. > -1 (vote, not veto), for putting mod_gz in the core. Please use -0.9 or something. It gets really confusing to see -1 and never know what it means. As I've said before: -1 really ought to always mean a veto. But... that's just me :-) Needless to say, I'm +1 on the concept. It's a big win for everybody. I haven't reviewed the code yet, so no commentary there. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/filters mod_include.c
Very cool patch!
On Sun, Sep 02, 2001 at 08:43:23AM -, [EMAIL PROTECTED] wrote:
>...
> +module include_module;
Hmm. That needs more stuff in there. I believe it should be:
module AP_MODULE_DECLARE_DATA include_module;
>...
> +/* Sentinel value to store in subprocess_env for items that
> + * shouldn't be evaluated until/unless they're actually used
> + */
> +static char lazy_eval_sentinel;
Please make that "static const char". Omitting the "const" means that it
goes into the process' modifiable data segment. Each time you fork, this
will contribute to another bit of working space. By making it "const", every
process shares the same const data.
[ yes, it *is* a "only" a byte of working space, but it is hard to know what
kind of padding is occurring, and keeping it const is a good habit. ]
>...
> +apr_table_setn(e, "DATE_LOCAL", &lazy_eval_sentinel);
> +apr_table_setn(e, "DATE_GMT", &lazy_eval_sentinel);
> +apr_table_setn(e, "LAST_MODIFIED", &lazy_eval_sentinel);
Maybe a comment to "see add_include_vars_lazy()" so that authors will know
where to go to find what is going on, and where to go to add new lazy
variables.
I would also recommend:
#define LAZY_VALUE (&lazy_eval_sentinel)
That will clarify a good chunk of code.
>...
>val = apr_table_get(r->subprocess_env, start_of_var_name);
> +if (val == &lazy_eval_sentinel) {
> +val = add_include_vars_lazy(r, start_of_var_name);
> +}
>*end_of_var_name = tmp_store;
>
> if (val) {
> @@ -962,12 +1020,21 @@
>if (!strcmp(tag, "var")) {
>const char *val = apr_table_get(r->subprocess_env, tag_val);
>
> +if (val == &lazy_eval_sentinel) {
> +val = add_include_vars_lazy(r, tag_val);
> +}
Two patterns of "apr_table_get, check for LAZY_VALUE, call
add_include_vars_lazy". I'd recommend a single "get_include_value()"
function to encapsulate this logic.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] RE: make distclean doesn't
On Sun, Sep 02, 2001 at 02:01:15PM -0700, Ryan Bloom wrote: > On Sunday 02 September 2001 10:28, Jim Winstead wrote: >... > > it may be worth following the gnu project's lead on these targets, > > since they use the same names. > > > > http://www.gnu.org/prep/standards_55.html#SEC55 > > > > (for them, distclean == what is in the tarball.) > > +1. If we are going to use their syntax, we should also use their > semantics. I will check with some other packages later today to see > what they do with make distclean. Few projects have a file like config.nice, so it doesn't apply. Don't sweat your time. I've explained my reasons, Cliff and Justin seem to agree quite wholeheartedly. And I'll repeat: -1 on anything rm'ing config.nice Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] RE: make distclean doesn't
On Sun, Sep 02, 2001 at 12:15:33PM -0500, William A. Rowe, Jr. wrote: > From: "Greg Stein" <[EMAIL PROTECTED]> > Sent: Sunday, September 02, 2001 3:22 AM > > > I use distclean on my computer all the time. Along with extraclean. Neither > > of those targets should toss config.nice. *That* is what I mean. > > > > To be clear: nothing in our build/config/whatever should remove config.nice > > Then it goes into the tarball. Is that what you intend? Don't be argumentative. Of course not. Why would a config.nice ever appear in our tarballs? We never run "configure" in there, so it should never appear. > > "Clean" rules are about cleaning out state that might affect a build in some > > way. So we toss object files, generate makefiles, the configure script, > > whatever. But config.nice doesn't fall into that camp because it is not a > > "stateful" file. It is for the user to rebuild what they had before. > > Which means it has nothing to do with cleaning the tree to a distribution > state (or state 'ready for distribution'.) It *does* return it to a distribution state. There is no leftover state that could erroneously affect another configure/build process. config.nice is outside of all that. And "ready for distribution" is not the right semantic, nor does it apply (as above) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/http mod_mime.c
I'm in complete agreement with Justin on this one.
"Add" says "add" to me. And filters *are* additive.
I wouldn't agree with Joshua's comments about tossing filter directives and
rely on each module to provide their own (how would you order them?), but I
think his meta-comment about "this stuff is confoozled" applies. A step back
and a rethink is probably necessary, rather than poking and prodding at the
various config directives.
Cheers,
-g
On Sun, Sep 02, 2001 at 09:05:50PM -0700, Justin Erenkrantz wrote:
> On Sun, Sep 02, 2001 at 10:49:52PM -0500, William A. Rowe, Jr. wrote:
> > Not this way. No other mod_mime variable behaves the way you you are trying.
> > I'm not kidding about adding a Set{Input|Output}FilterByType/SetHandlerByType
> > so when we ask folks to rely upon mime types, they can actually do so.
>
> And, that's additive?
>
> So, I could do:
>
> SetOutputFilterByType BAR text/*
> SetOutputFilterByType FOO text/plain
>
> As a user, I *expect* that both filters are activated.
>
> I think you make it sound like we have to do:
>
> SetOutputFilterByType BAR text/*
> SetOutputFilterByType BAR;FOO text/plain
>
> Yuck, yuck, yuck, yuck, yuck. (Did I mention I think this is yucky?)
>
> > Yes... please read mod_mime.html. AddSomething is not additive, and can't be.
> > The server config is often a nightmare to grok as things stand today. Don't make
> > things harder by absusing fairly consistant definitions such as AddSomething or
> > SetSomething. The inner container always overrides the outer.
>
> As a user, I expect that to be additive *in the case of a filter*. I
> expect it to override in the other cases - just not this case. You
> can't have multiple handlers, but you can certainly have multiple
> filters.
>
> > So the inner needs AddOutputFilter FOO;BAR html - as of today. I suggested an
> > entire +|- syntax as well, it was somewhat booed since existing +|- syntaxes are
> > perceived as confusing. Here, well I think it's necessary.
>
> That's confusing. I think the cleanest way is for it to be additive
> (with a RemoveOutputFilter to remove one from a higher level -
> ignoring this directive if the filter doesn't exist from a prior
> level).
>
> > None of this is addressing filter ordering across commands yet. I said 8 months
> > ago we've done an inadequte job of defining the use-filters syntax. I'm saying
> > the very same thing today.
>
> Yeah, I expect that the ordering of filter execution isn't going to be
> right given the code we have now. -- justin
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Add mod_gz to httpd-2.0
On Sun, Sep 02, 2001 at 11:42:48AM -0500, William A. Rowe, Jr. wrote: >... > WebDAV isn't a 'done' protocol, and really started out as the WebDA protocol The portion of WebDAV defined by RFC 2518 is *very* done. It kinda has to be, to be an RFC :-) > (versioning deferred until we figure out how it should be described.) mod_dav already supports DeltaV, provided a backend that actually implements it. There may need to be some tweaks for the recent drafts, but Apache's support is done. (I doubt mod_dav_fs would *ever* want to natively support DeltaV; that is definitely for third party backends) > I'm sure it will grow. Actually, I would doubt that. It's got all the bits in there already. If anything, it could shrink over time. Stuff like "If:" header handling is appropriate to the whole server, not just DAV. And there are some utilities and other items that are redundant or can be migrated (e.g. time formatting). In truth, mod_dav is a (functionally) stable piece of code with a highly dedicated maintainer. There are ways to improve it, and I'm actually in process on an improvement currently (it *shrinks* mod_dav, although mod_dav_fs grows some). It isn't like mod_proxy which used to be buggy and under-featured, or mod_rewrite without an RFC or a maintainer. > In a perfect world, apache-2.0.26-gold-core.tar.gz contains just the core. Then > give the world apache-2.0.26-gold-all-gold-20010915.tar.gz with all _released_ > subprojects effective 2001.09.15 rolled in! The adventurous could try the > apache-2.0.26-gold-all-beta-20010915.tar.gz. Finally, the looney (most folks > that follow this list) can grab apache-2.0.26-gold-all-alpha-20010915.tar.gz > for alpha releases of every module (probably a longer list, since some subprojects > at a given time likely haven't gone gold just yet.) I don't like your package/version names :-), but I completely agree with the concept. It's what I advocated when we were discussing what to do about the proxy stuff (and each time since). Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] hash table for registered filter list
On Mon, Sep 03, 2001 at 11:53:59PM -0700, Justin Erenkrantz wrote: > On Mon, Sep 03, 2001 at 10:50:01PM -0700, Brian Pane wrote: > > The ap_add_input_filter/ap_add_output_filter functions do an O(n) scan > > through the list of registered filters. This patch replaces the linear > > list with a hash table for better performance. > > Yup. The "### use a hash table" was a dead giveaway that someone > else thought this was needed. That was me :-) +1 on the concept, but the patch is busted. Seeing that this is early in my set of new messages to read, I bet somebody has already discovered that... :-) > Committed. Thanks. -- justin eek. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Make filter names lowercase
On Tue, Sep 04, 2001 at 12:08:36PM -0700, Justin Erenkrantz wrote: > Both Cliff and OtherBill sent me emails complaining about the > change to strcasecmp in some places to support Brian Pane's > new hash code in util_filter.c. (Never mind that a lot of > the code already did strcasecmp...) IMO, code examining the filter chain is suspect to begin with. It breaks a lot of abstractions for code to "reach in" and examine any of those values. >... > Please review and test. My commits are getting an awful lot > of negative feedback lately, so I'm going to switch to R-T-C. > So, I'll need 3 +1s to commit this. -- justin That's total bullshit. You have commit privileges. If something is the right thing to do, then commit it. If people have issues with the commits, then it can be backed out or adjusted. There is also an assumption that you'll self-correct over time. But if other people are being overly harsh, then you can tell them so. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] RE: make distclean doesn't
On Tue, Sep 04, 2001 at 08:15:08AM -0700, Ryan Bloom wrote: > On Sunday 02 September 2001 01:22, Greg Stein wrote: >... > > I use distclean on my computer all the time. Along with extraclean. Neither > > of those targets should toss config.nice. *That* is what I mean. > > > > To be clear: nothing in our build/config/whatever should remove config.nice > > > > > > "Clean" rules are about cleaning out state that might affect a build in > > some way. So we toss object files, generate makefiles, the configure > > script, whatever. But config.nice doesn't fall into that camp because it is > > not a "stateful" file. It is for the user to rebuild what they had before. > > Just to point out, Apache 1.3 had config.status which is analogous to 2.0's > config.nice. It turns out that make distclean in 1.3 removes config.status. > > I would say this is proof that we should be removing config.nice with 2.0. That isn't proof, that is an opinion -- that you happen to like what was done in 1.3. I see it as 1.3 attempted to look like ./configure and create a config.status, and along those lines it torched config.status. But in Apache 2.0, we have a *real* config.status which gets tossed because it is stateful and you should be tossing it. config.nice is for the user to retain the information about how to reconfigure their Apache after a thorough cleaning. It contains no state that could mess up a future config and build. And it retains *very* useful information for the user. The only thing that tossing config.nice will do is inconvenience our users. What is the point in that? I'm for helping users, not pissing them off. How many more times do I need to say this, Ryan? Here is number three: -1 on removing config.nice. Drop it already. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Add mod_gz to httpd-2.0
On Mon, Sep 03, 2001 at 05:47:02PM -0700, Ryan Bloom wrote: >... > I have a big problem with this. We had a hard enough time contributing > patches back to MM. The only reason we keep expat and pcre up to date, > is that we NEVER make any changes to them. I would be very much against > adding zlib to our tree. Not to mention that I'm also an Expat developer, so I can cross-port changes back and forth between the trees. :-) But yes: the stability of PCRE and Expat are a big help. However, I'd point out that we didn't make a lot of changes to MM either; it was quite stable, too. No idea why the changes didn't go back and forth, tho. In fact, the ASF has been a very good influence on Expat. Our ideas are being used for its config/build setup. A lot of the portability work and research that the ASF is good for, is being reflected into Expat to ensure that it is just as portable. Cheers, -g -- Greg Stein, http://www.lyra.org/
zlib inclusion and mod_gz(ip) recap (was: [PATCH] Add mod_gz to httpd-2.0)
module in there now, *before* beta hits. You say that you don't want to release it while the APIs are in flux -- that they should be stable. But that is bogus. If we include mod_gzip *today*, then it will get fixed along with everything else as we change the APIs. You aren't going to be responsible for keeping it up to date with the changes. We are. That is part of what going into the core means -- that we have an obligation and responsibility to maintain it. And we will. If it goes in. Summary: * zlib is not going to be included in any fashion, we'll use it by reference if/when we need to * you should release your 2.0 mod_gzip so that everybody can make an informed decision about putting compression functionality into Apache * if mod_gzip goes in, then it would do so earlier rather than later, and we will deal with any API changes that occur between now and ship -- it isn't your burden to bear, so there shouldn't be a reason to withold. Cheers, -g -- Greg Stein, http://www.lyra.org/
patch comments Re: [PATCH] Add mod_gz to httpd-2.0
On Sat, Sep 01, 2001 at 02:56:45PM -0700, Justin Erenkrantz wrote: >... > I told him I'd look at it a while ago, but never got a chance to do > so. So, I spent this morning cleaning up the configuration and a bit > of the code to fit our style (nothing major). You shouldn't have to do that. Ian should, and he must learn to deal with it if he wants to continue contributing. We all learn different habits to work in this group. > I'd like to add this to the modules/filters directory (which seems > like the most appropriate place). Yes. >... > We could remove GZFilter as it really serves no purpose as well as the > text/html check in mod_gz. s/could/should/ I agree that it doesn't make sense to be in there. We should be using more consistent mechanism to handle filter inclusion and management. One-off commands here and there are simply serving to make the filter system more complicated. > I'd like to commit something that is close > to what Ian originally submitted and then tweak it slightly. Push it back to him to tweak. It appears we have a bit of time :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
the rollup issue (was: Re: [PATCH] Add mod_gz to httpd-2.0)
On Sat, Sep 01, 2001 at 06:19:32PM -0700, Ryan Bloom wrote: >... > 3) I don't believe that we > should be adding every possible module to the core distribution. I > personally think we should leave the core as minimal as possible, and > only add more modules if they implement a part of the HTTP spec. The list has already covered the "minimalist" thing, but the rollup issue is still outstanding. A suggestion: create httpd-rollup to hold the tools/scripts/web pages and whatnot for creating a combo of httpd releases plus supporting modules. The -rollup project could create rollups of just ASF bits (proxy and ???), but could also be a way to rollup third-party modules (like Ralf's module bundles). I believe we have also discussed how the Apache Toolbox could become an ASF project. I'd suggest that it goes into httpd-rollup as one of its "outputs". For example: /home/cvs/httpd-rollup contrib-1.3/Apache 1.3 plus a bunch of contrib modules toolbox/Apache Toolbox asf-2.0/Apache 2.0 plus ASF bits contrib-2.0/Apache 2.0 plus ASF plus contribs ... Under httpd-site, we'd create the /rollup/ subdirectory and tightly incorporate references to it with our distribution pages (to ensure that the blob with proxy in it is just as easy to find/download as the core). Does this seem like a reasonable approach to get out of the rollup logjam? Given this kind of arrangement, would some module contributions or inclusion have a "lower bar" to become part of ASF-distributed bits? etc. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: sub requests are all "GET"s
Take a look at ap_sub_req_method_uri. That might do the trick for you. I don't think there is a similar one for files right now. Cheers, -g On Wed, Sep 05, 2001 at 08:17:15AM -0400, Eric Prud'hommeaux wrote: > Can anybody explain why ap_set_sub_req_protocol does > rnew->method = "GET"; > rnew->method_number = M_GET; > instead of > rnew->method = r->method; > rnew->method_number = r->method_number; > ? The consequence is that functions like negotiation > sub_req = ap_sub_req_lookup_file(dirent.name, r, NULL); > check auth on the wrong method. You can check this by POSTing to > foo and having a limit on POST for foo.php3 (as opposed to the > whole directory). A quick way to check is to set a breakpoint in > ap_set_sub_req_protocol and > telnet localhost 80 > POST /Overview HTTP/1.0 > Content-Length: 5 > > abcd > Any calls to the auth modules will have a method of GET despite > the POST action they will eventually execute. > > All auth modules and the like could check for this: > int method = r->main ? r->main->method_number : r->method_number; > but it seems better to have the sub request default to the method > of the request that inspired it. There may be some modules that > may count on the default behavior, like mod_include, but I think > they should specifically make the new method be a GET as they are > not duplicating the parent request's behaviour. > > -- > -eric > > ([EMAIL PROTECTED]) > Feel free to forward this message to any list for any purpose other than > email address distribution. -- Greg Stein, http://www.lyra.org/
Re: zlib inclusion and mod_gz(ip) recap
On Wed, Sep 05, 2001 at 03:56:32AM -0400, [EMAIL PROTECTED] wrote: > Greg Stein wrote... >... > > As stated elsewhere, pcre and expat are in there because they aren't > > typically available, like zlib is. > > Ah... so that's the criteria? Ok. Generally, yes. But size matters :-) OpenSSL 0.9.6 isn't commonly installed, but it is too big for us to include. (not to mention the general crypto issues) > I believe the other libs you mention have been 'tweaked' > and that's another reason why the source is there, yes? We try to avoid it. Speaking for Expat, any differnces from Expat's CVS is simply because I haven't done the cross-patching needed yet. > What if you ever need to 'tweak' ZLIB because it was never > really designed to be a real-time data compression engine? > Would that remove the 'no ZLIB source' dictum? It certainly could, yes. > > Nothing needs to happen, so it shouldn't be surprising :-). If/when we need > > it, then we will include it. As I said, it is just config macros. > > You are putting an awful lot of faith in ZLIB and the assumption that it > will suit your needs 'out of the box' but I hear ya... Until I learn differently, I'll trust that all the other zillions of zlib users have ironed out the problems. At this point, I have no concrete evidence that problems exist in zlib; just some FUD :-) > > There are three options on the table: > > > > 1) include mod_gzip > > 2) include mod_gz > > 3) include neither > > > > I believe there is consensus that (3) is not an option. > > Huh? > > I didn't see any real consensus on that. > Only a real vote will tell you that. We don't always need votes to get a general feeling of people's thoughts on various issues. > Why note call for one?... not a vote on any particular piece of code Because we don't need to. General consensus is supportive. We'll continue on the assumption that *something* will go in, until somebody actually gets up and vetoes it. But that probably wouldn't happen until we've selected mod_gz or mod_gzip for what goes into Apache. >... > Ryan himself said he prefers 3 right off the bat when Jerry > said 'Let's dump Ian's mod_gz into the core!' which is what > started this whole entire thread. Ask him what he thinks now :-) Knowing Ryan, he is probably fine with adding it at this point. >... > No one ( including myself ) has made any kind of airtight case There is very little that goes into Apache nowadays that has an airtight case. A general feeling of consensus implies we don't need it. >... > > Despite yours and > > Peters pushing and stressing and overbearing "sell job" to get mod_gz(ip) > > type functionality into the core, it was just preaching to the choir. (well, > > okay: maybe Ryan didn't want to see it in there :-) That sell job mostly > > served to create an air of hostility. > > Yea... okay... whatever... we are the 'bad guys' again for trying to > improve your Server. Mea Culpa. Now you're just misconstruing what I said. I talked about your *approach*. Your intent/hope to improve Apache is welcome and appreciated. But you consistently aggravate people with how you approach and handle discussions. >... > > But then you say to look at the 1.3 version. > > What I meant was... if anyone wants to see right away if a whole bunch > of pure 'coding style' bullshit is going to be the show stopper right off the Coding style is never a showstopper. When it goes it, we will make it fit the Apache coding style. Just ask Ben Laurie what some of his contributions look like now :-) Yes, people have complained about stuff in mod_gzip 1.x, but don't misinterpret that as a flat out veto. > bat then there's no need to wait for anything. As I have said 3 times now... > it doesn't take rocket scientist to look at an existing module and imagine > what it will STILL look like with filtering calls in it. Of course. But people want to see mod_gzip 2.0, not the old one. > I assure you... I did not rewrite the 1.3.x module just for 2.0. All I did > was add the filtering calls. It was no big deal. The post the fucker already. Just what is your problem with posting it? If you want us to integrate it, then why not post it NOW? Please explain. >... > I also haven't gotten an answer to my question regarding the weird comment > that appeared in this discussion about any module being submitted that > supports both 1.3.x AND 2.0 in the same code ( which the mod_gzip I have > here certainly does ) will be rejected for that reason alone. As with the coding style, you are conflat
Re: remaining CPU bottlenecks in 2.0
On Tue, Sep 04, 2001 at 11:56:48PM -0500, William A. Rowe, Jr. wrote: >... > You were discussing the possibility of parsing for
Re: Add mod_gz to httpd-2.0 ( Thread restart )
On Wed, Sep 05, 2001 at 08:07:26PM -0700, Justin Erenkrantz wrote: > On Wed, Sep 05, 2001 at 08:05:27PM -0700, Ian Holsman wrote: > > Some performance results with mod_gz are available at > > http://webperf.org/a2/v25/ > > (no core dumps.. pages look ok on a real browser while running test) > > I'm going to be re-running the tests for a longer period to see if > > there are memory leaks (as well as quantify/purify on it next week) > > > > brief summary: > > * numbers are based on pages going through mod-include > > * once of the runs looks a bit flaky.. and I'm re-running it > > * cpu usage (USR) is VERY high (from 100 without gz to 500) > > * page requests down 16% when using GZ What does this mean? Total bytes transferred? If so, then why only 16% The number of page requests should be constant, unless mod_gz introduces some kind of caching. > > * page resposne times are slower with GZ, but have a lower deviation > > The expectation is that the network bandwidth used should be much > lower. The tradeoff is that the CPU should get nailed. -- justin Also: latency should increase and delivery time should decrease. cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Fix relative internal file...
I don't understand what you're trying to solve here, and how this solves it.
Cheers,
-g
On Wed, Sep 05, 2001 at 08:15:37PM -0700, Justin Erenkrantz wrote:
> $.10 hack. Works for me.
>
> The other solutions would require a rewrite of the logic. That
> is something I don't have time to do and I also bet that OtherBill
> has ideas about how to fix this the "right way." Or not. =-)
> -- justin
>
> Index: server/request.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/request.c,v
> retrieving revision 1.48
> diff -u -r1.48 request.c
> --- server/request.c 2001/09/01 05:21:16 1.48
> +++ server/request.c 2001/09/06 03:11:00
> @@ -1581,9 +1581,7 @@
> * either, if the base paths match, we can pick up where we leave off.
> */
>
> -if (strncmp(rnew->filename, fdir, fdirlen) == 0
> - && rnew->filename[fdirlen]
> - && ap_strchr_c(rnew->filename + fdirlen, '/') == NULL)
> +if (strncmp(rnew->filename, fdir, fdirlen) == 0 && rnew->filename[fdirlen])
> {
> char *udir = ap_make_dirstr_parent(rnew->pool, r->uri);
>
--
Greg Stein, http://www.lyra.org/
Re: zlib inclusion and mod_gz(ip) recap
On Wed, Sep 05, 2001 at 11:05:50AM -0700, Doug MacEachern wrote: > On Wed, 5 Sep 2001, Ryan Bloom wrote: > > > > Ryan himself said he prefers 3 right off the bat when Jerry > > > > said 'Let's dump Ian's mod_gz into the core!' which is what > > > > started this whole entire thread. > > > > > > Ask him what he thinks now :-) Knowing Ryan, he is probably fine with > > > adding it at this point. > > > > Nope. My opinion hasn't changed. I won't veto, but I continue to think > > this is a bad idea. > > i have the same opinion, for the same reason i was stunned (and still > am) to see ldap modules in the 2.0 tree. That was a surprise to me, too. I was out during July. I thought we would have some simple ldap stuff in APRUTIL... not a huge codebase in httpd. (when I discussed a division of the code, I was expecting a separate vote for adding mod_ldap to httpd-2.0, which I would then vote against :-) Since then, I had also thought we agreed to move mod_ldap *out* of the core, into httpd-ldap. To make all of this workable, I just recently posted a thought on how to deal with rollup releases. No comments appeared on that yet, tho. > new modules at this point are > only going to further delay the release of 2.0, hell even discussing > adding new modules is contributing to the delay. If Ian is not assisting with stabilizing the httpd core, then how could his work on mod_gz delay the core? IOW, he is not "subtracting" anything from the stabilization process. If the argument is that people who *are* stabilizing the core will now be distracted... that is their problem :-) mod_gz is just a little bugger off to the side that the core people don't have to truly worry about. It will get enough attention from the "fringe" people, if you will (my pardons to people who were just called fringe :-). > i do think apache should be bundled with one or the other (mod_gz or > mod_gzip), but it should wait until 2.1. i'm pretty sure most people > (myself included), are most interested in being able to just use 2.0 > feature-wise as-is and won't mind waiting until 2.1+ for new features. Introducing mod_gz isn't going to slow down a 2.0 release. And it *is* a part of RFC 2616, supported by a bunch of browsers, and it is definitely missing functionality (re: its lack was pasted over by mod_perl's and PHP's band-aid solution to compression). > we're in the 9th month of year 2001, i saw the first glimpse of a '2.0' > server in early 1996 (rob thau's), i have no problem waiting longer for > bug fixes, performance, "doing things right", etc., but there is no good > reason to add new modules or big features at this point. they should wait > for 2.1+. If people want a stable server, then they should work on that, and not pay attention to mod_gz within the server. It should be stable enough, and it *does* simply use the existing module and filter interfaces. Why should core people worry about it? It can go in now and be fixed over time. There are a good number of people in this group, and each of those people concentrate on different items, for different reasons. If you want a stable server, with a sooner-than-later release, then concentrate on the core. Some of those other people will work on modules which have *nothing* to do with stability. If the core developers are distracted by mod_gz, then that is their own fault, not the fault of introducing a new module. Cheers, -g -- Greg Stein, http://www.lyra.org/
why not post mod_gzip 2.0? (was: Re: [PATCH] Add mod_gz to httpd-2.0)
On Wed, Sep 05, 2001 at 01:46:55PM -0600, Peter J. Cranstone wrote: > I suppose the only thing we can do is contribute. Kevin has, mod_gzip > was released under an ASF license which was approved by the ASF Board. > If there is a hidden agenda there then you're better than I at spotting > it. > > Mod_gzip is available for 1.3.x > > It will be available for 2.x when you hit beta. >... > Now tell me where the hidden agenda is. You and Kevin never answered my simple question: Why won't you post mod_gzip 2.0 *today*? I asked several times, but it never got answered. I seem to recall somebody stating it was being held, pending stability of the internal Apache APIs. But that isn't an issue if it is to be incorporated into Apache. Another response was "look at 1.3 and port it forwards to 2.0." Why should that happen, when you've done the work? If you intend to contribute it, then why the delay, making us repeat your work? Without that simple question answered, then yes: I would think there is some kind of hidden agenda. Avoiding that question, and not posting mod_gzip 2.0 makes it appear that you are trying to hide something. The more conspiratorial-minded of us would simply believe this whole thread is to create a division and weaken the voting for mod_gz. Much like politicians will introduce a second, similar bill to confuse and divide supporters and then crush both bills. But of course that wouldn't happen here, now would it? :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Add mod_gz to httpd-2.0 ( Thread restart )
On Wed, Sep 05, 2001 at 09:15:56PM -0700, Ian Holsman wrote: > Greg Stein wrote: > >>On Wed, Sep 05, 2001 at 08:05:27PM -0700, Ian Holsman wrote: > >>>Some performance results with mod_gz are available at > >>>http://webperf.org/a2/v25/ > >>>(no core dumps.. pages look ok on a real browser while running test) > >>>I'm going to be re-running the tests for a longer period to see if > >>>there are memory leaks (as well as quantify/purify on it next week) > >>> > >>>brief summary: > >>> * numbers are based on pages going through mod-include > >>> * once of the runs looks a bit flaky.. and I'm re-running it > >>> * cpu usage (USR) is VERY high (from 100 without gz to 500) > >>> * page requests down 16% when using GZ > > > > What does this mean? Total bytes transferred? If so, then why only 16% > > > > The number of page requests should be constant, unless mod_gz introduces > > some kind of caching. > > the number of concurrent requests are constant in all of these tests. > we have 50 processes (spread across 10 machines) doing GET's of a page. > when a page is retrived is GET's it again (and again..) > so if the page response is slower, then the pages/second will also be. > > we have a more sophisticated comercial machine which can generate better > traffic, but it is core dumping at the moment so I can run't the tests through it. Ah! By "page requests down..." you mean page requests *per second* ?? You omitted that "per second", so the statistic made no sense... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Fix relative internal file...
On Wed, Sep 05, 2001 at 09:10:01PM -0700, Ian Holsman wrote: > Cliff Woolley wrote: > > On Wed, 5 Sep 2001, Greg Stein wrote: > >>I don't understand what you're trying to solve here, and how this > >>solves it. > > > > It's an attempt to fix the mod_include > > segfault. It "solves" it by setting r->uri to something valid rather than > > setting it to "INTERNALLY GENERATED...". It's as yet unclear as to > > whether it's a correct solution, though. I'm looking into it. > > > > --Cliff > > If the request is always relative to where we are, > and we know the URI where we are... couldn't we generate the URI > correctly ??? apr_pstrcat(r->uri, '/',file,NULL) ??? Maybe the specified file does not have a corresponding URI. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: zlib inclusion and mod_gz(ip) recap
On Thu, Sep 06, 2001 at 08:54:58AM -0700, Doug MacEachern wrote: > On Wed, 5 Sep 2001, Greg Stein wrote: > > > mod_gz is just a little bugger off to the side that the core people don't > > have to truly worry about. > ... > > It can go in now and be fixed over time. > ... > > modules which have *nothing* to do with stability. > > so you're saying mod_gz would go into modules/experimental ? If you're opposed to modules/filters, then sure. That is actually a pretty decent place to put it while it is worked on. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: zlib inclusion and mod_gz(ip) recap
On Fri, Sep 07, 2001 at 12:27:05PM -0400, Jim Jagielski wrote: > Ben Hyde wrote: > > > > To repeat all these inform the debate, they are a list > > of things to think about, but we don't have and don't > > want some objective scoring scheme for these; the > > public debate process while tedious is good - particularly > > when we all remain gentlemenly. > > Here here. My intent was not scoring, but rather, as you said, > an honest *discussion* of the module, and some topics that > should be discussed about it. I just don't want us to not do > What's Right in this case simply because of the emotions > that the debate caused. There shouldn't be any emotions at this point. There is no contention between two modules. We are only talking about mod_gz, which Ian posted a while back. Current consensus appears to be to add it to modules/experimental. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: zlib inclusion and mod_gz(ip) recap
On Fri, Sep 07, 2001 at 09:33:13PM -0400, Rodent of Unusual Size wrote: > * On 2001-09-07 at 21:21, > Ryan Bloom <[EMAIL PROTECTED]> excited the electrons to say: > > > > I don't see how that could possibly be the consensus, since I have -1 in > > the STATUS file. > > By the way, consensus does not always mean unanimity. It also > means general or majority agreement.. Right. That was my intent. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
On Fri, Sep 07, 2001 at 10:38:45PM -0700, William A. Rowe, Jr. wrote: >... > Ryan's veto has effectively tabled this for now. I'm begining to respect > this from the perspective of putting a release in peoples hands. It can > be introduced soon afterwards, or if someone likes, a subproject can be > created. This has been too long people, let's put 2.0 to bed. Some people believe his veto is illegitimate -- that there is no technical reason for vetoing the inclusion into modules/experimental. Cheers, -g -- Greg Stein, http://www.lyra.org/
why mod_gz "now" ? (was: Re: cvs commit: httpd-2.0 STATUS)
On Mon, Sep 10, 2001 at 12:26:58PM -0400, [EMAIL PROTECTED] wrote: >... > If everyone really agrees with Justin/Ian and others that the > development tree needs a GZIP filter BEFORE the next > beta and it ( for some reason ) has to become part of the > core *at this moment* then just go with mod_gz. For people to easily work with it, test it, fix it, etc, then placing it into modules/experimental is the way to go. Consider the caching stuff in there right now. Maybe that stuff works, maybe not. But its presence in source control means that more than one person can easily work on it. It means that when somebody makes a change, a commit email is sent out for review. It means that people can configure it into their server and try it out. Living as a [PATCH] on a mailing list doesn't provide for any of those capabilities. >... > Heck... mod_gzip doesn't even use ZLIB so if my conversations > with Mark Adler about the specifics of you guys using ZLIB > if you want to doesn't prove we really are just trying to help > and make sure you have all the information you need to make > and intelligent decisin then I don't know what would. We appreciate the information about zlib, but I think it will be easier for us to work with alternatives and other stuff when we have the stuff in our CVS repository. >... > I am just pointing out that no one has done a really good > code review of mod_gz even if the 'consensus' is to drop > it into the core. Yup. It was posted as a patch. Its potential for addition to the repository was in question, so why would somebody spend precious time to review a black sheep? :-) Consensus is to have it in the core, insofar as modules/experimental can be called the core. > I've already pointed out one or two problems > ( minor ) but a new one I found is that it seems to duplicate > the addition of the GZIP header at all times ( gets added twice?) > before the EOS bucket is sent down the pike. Client-side results > could be unpredictable. Depends on the browser. It is also completely > ignoring the FLUSH bucket when it arrives and has no way to add > 'Content-length:' if/when it is necessary. All good points. There are also a bazillion other things that mod_gzip 1.x does, which mod_gz can't do (and you've explained that it should). That work can be done with the module placed into the experimental directory. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
On Mon, Sep 10, 2001 at 11:33:43AM -0400, Rodent of Unusual Size wrote: > * On 2001-09-10 at 11:17, > Ryan Bloom <[EMAIL PROTECTED]> excited the electrons to say: > > > > I have removed my veto. Although, I would point out that illegitimate veto > > or not, nobody in this group has ever gotten away with going through a veto. > > The only reason I have removed my veto is that it really looks like everybody > > was about to ignore it anyway. > > I doubt it mightily. I think people are/were trying to show you that > *they* support it. People are permitted to have opinions, you know. > And even though I personally thought your veto was bogus, it was > still a veto and I would not have countenanced it being overridden. > > > This whole thing just leaves me with a bad > > taste in my mouth. All I keep thinking, is that we are trying > > to spite RC by adding a different GZ module. > > That may be the case for others, but not for me. I feel strongly > that this functionality belongs in our distribution. Until > last week's unpleasantness convinced me of certain things, I > would not have cared which implementation was accepted (modulo > the various concerns already voiced by myself and others). I agree with Ken on all points (veto, spite, rationale for addition). mod_gz implements a part of RFC 2616, and a piece which can be very helpful for servers on the side of a slow connection (eg. my upstream is just 128k) and for users on their side, too (and don't say modems will do the compression; there are more factors than that last leg, and we've already discussed the ssl-then-no-compress vs compress-then-ssl thing) We're a reference for HTTP serving. I'd like to see S3.5 implemented :-) (I'm also psyched to try out decompression in Python's httplib module; if zlib is present, then httplib could transparently decompress for the user) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [proposal] httpd-extras
On Sun, Sep 09, 2001 at 10:15:31AM -0700, Ian Holsman wrote: > I was thinking. > that instead of creating all these little sub-projects why not create a > 'httpd-extras' one. First, I would suggest that we get the httpd-rollup (or somesuch) working first (I suggested this last week), so that we can do rollup releases. Once we have that framework in place, then something like -extras could make sense. But any use of repositories outside of httpd-2.0 is going to simply make our existing rollup problem even worse. Cheers, -g -- Greg Stein, http://www.lyra.org/
what about proxy? (was: Re: General Availability release qualities?)
On Sun, Sep 09, 2001 at 02:16:25PM +0200, Graham Leggett wrote: > Justin Erenkrantz wrote: > > > - Resolve the proxy situation. A lot of committers (FirstBill > > comes to mind) would like to see this come back in the core. > > If it is ready and the group consensus is to add it, then let's > > add it back in before GA. > > As FirstBill said, adding it back received the needed +1's when it went > to vote. After all the discussion had subsided though, it was simply not > done. > > As I said on thursday, later this evening I'll be comitting the proxy > module to it's placeholder in the core. When you say placeholder, are you suggesting that you're going to commit the code back into the httpd-2.0 branch? Rather than doing that, why don't we work on the rollup mechanism instead? Proxy has been moving along terrifically while in its own CVS module. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
On Mon, Sep 10, 2001 at 11:37:20AM -0400, Rodent of Unusual Size wrote: > * On 2001-09-10 at 11:17, > Jim Jagielski <[EMAIL PROTECTED]> excited the electrons to say: > > > > I dislike the feeling that 'experimental' is a valid dumping ground > > for modules that we think are cool can can't be bothered with. > > Is that supposed to be 'but can't be bothered with'? If so, that > is not the case at all -- people *want* to bother with it, > but the 2.0-ASAP crowd felt it would detract from *their* agenda. :-D Yup. As I explained to Kevin Kiley, having it in experimental *means* that we can bother with it. It is really difficult to do that while it is a patch being "maintained" by a single person. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
On Mon, Sep 10, 2001 at 11:39:28AM -0400, Rodent of Unusual Size wrote: > * On 2001-09-10 at 11:17, > Ryan Bloom <[EMAIL PROTECTED]> excited the electrons to say: > > > > We've been over this too much for me to explain again why the module > > doesn't belong in the core. > > In *your* opinion. Other people have different but equally valid > opinions. No one person sets an absolute for the project. Well, until they whip out their veto pen :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
I'm with Joshua. I don't see the need/purpose for creating a second mechanism for aliasing locations to filesystem paths. Why have two? Why have to answer "which should I use?" etc etc Cheers, -g On Mon, Sep 10, 2001 at 11:48:57AM -0400, Joshua Slive wrote: > > > +* Allow the DocumentRoot directive within scopes? This > > + allows the beloved (crusty) Alias /foo/ /somepath/foo/ followed > > + by a to become simply > > + DocumentRoot /somefile/foo (IMHO a bit more > legible > > + and in-your-face.) This proposed change would not depricate > Alias. > > I don't like that. (Sorry to be a constant nay-sayer on config changes ;-) > > 1. It overloads DocumentRoot. "Root" should mean "/". > > 2. It encourages people to use where they should be using > . > (Imagine if you protected this block with basic auth, but forgot > that another URL can access the same directory.) > > If you wanted to add a "URLMapTo" directive that does something similar, I > would be less opposed. But I still would not be in favour. > > Joshua. -- Greg Stein, http://www.lyra.org/
httpd-extras (was: Re: cvs commit: httpd-2.0 STATUS)
On Mon, Sep 10, 2001 at 04:51:41PM +0200, Graham Leggett wrote: > "William A. Rowe, Jr." wrote: > > > http://modules.apache.org/ > > > > Perhaps this isn't frequently referenced enough from our own pages??? > > There is currently no way to distinguish between apache-supported (but > not included in the core) modules, and third party modules which have no > implied level of code quality. This distinction is very important from a > user perspective. Agreed. It is really difficult to get any measure of release frequency or maintenance. Ideally, modules.apache.org ought to be more like Freshmeat -- a good amount of detail on each module and its release history. > I agree with Ian's httpd-extra's proposal, which is a release of extra > stuff that is not part of the core, but is "blessed" and supported by > the folks here. I'd tend to agree with it also, but with two changes: 1) don't drop them into different places in modules/, but just have them all go into modules/extra/... That would allow us to have a cvs root at modules/extra (the intermixed form implies only using a tarball) 2) set it up *after* we establish the httpd-rollup subproject, so that we have a consistent, easy work pattern for multiple/rollup releases Cheers, -g -- Greg Stein, http://www.lyra.org/
merge macros (was: Re: mod_ssl broken)
On Mon, Sep 10, 2001 at 09:23:59AM -0500, William A. Rowe, Jr. wrote: > From: "Ryan Bloom" <[EMAIL PROTECTED]> > Sent: Monday, September 10, 2001 8:39 AM > > > > I don't mind putting a patch together that does this (and to use > > > it in all core modules). I would appreciate suggestions for the > > > final names though (naming isn't my strong side). > > > > > > AP_CFG_MERGE > > > AP_CFG_MERGE_ARRAY > > > > Don't do that. That is just Ralf's coding style. It is not proof that we need >this > > in every single core module. > > HUH? The point is that even subversion uses such helpers for clarity. It would > help all authors to offer these, consistently. > > We've (collectively) proven that config merge errors are simple to author, and > difficult to debug. And the SVN merge macros came from mod_dav. The only reason mod_dav_fs doesn't have a similar macro is that it only has a single item to merge, so I just spelled it out. So you could say that I'm hitting 3 for 3 with using that INHERIT_VALUE macro, and would appreciate a core version of it. AP_CFG_MERGE_* located in http_config.h would make some sense to me. In the mod_ssl version, there was the "unset" param; I would suggest the simplest for assumes NULL. A second one would take an "unset" param. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: merge macros
On Mon, Sep 10, 2001 at 03:18:35PM -0700, Ryan Bloom wrote: > On Monday 10 September 2001 15:11, Sander Striker wrote: > > Ryan, was that a -1, or just a 'don't invest time in it, I don't > > think it's worth it'? > > The latter. I would never veto something like this, I just don't believe that > it is required. Translated as, "I wouldn't spend time on it, but your time is yours -- feel free spend your time on it if you want." IMO, things that clarify or make the codebase more consistent are always a good thing. In some cases, not enough of this happens and the codebase begins to show its creakiness. Adding comments, increasing consistency, refactoring, etc. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] resubmit new and improved ap_vrprintf fix to handle > 4K
On Mon, Sep 10, 2001 at 09:13:46PM -0700, dean gaudet wrote:
> buhhh?
>
> you guys built a zero-copy infrastructure and you're putting in a one-copy
> sprintf?
sprintf varieties must be at least one-copy, based on our vformatter code
(which BUFF used, too). It seems you're implying sprintf should be zero-copy
also. ??
> i don't get it. this new ap_rsprintf is less efficient than the
> ap_bsprintf in apache-1.3.
I don't doubt it. The primary intent for ap_r* was compatibility; the most
optimal mechanism for delivering content is through the new brigade
mechanisms (down thru the filter chain).
> also, wow, ap_rputc really sucks now. that's a lot of code (in
> buffer_output) compared to:
>
> *buf++ = c;
>
> guess all your abstraction has hidden the real buffer and you can't do any
> useful optimisations?
At this point, we are not trying to optimize "around" the filter chain.
Since we can't know what is next in the chain, buffer_output we must use
ap_fwrite() to buffer the data.
In the future? Yah. Maybe we'll be able to do more, but that won't happen
for a while.
>...
> oh ouch i gotta stop looking:
>
> /* this will typically exit on the first test */
> for (f = r->output_filters; f != NULL; f = f->next)
> if (strcasecmp("OLD_WRITE", f->frec->name) == 0)
> break;
>
> i'm puking. strcasecmp strings??
Yah :-) It could be optimized by recording a flag in the request_rec,
stating that the filter was inserted (rather than looking for it). The
strcasecmp() shouldn't be necessary because we uppercase f->frec->name
nowadays.
Not that it will improve things overall, but there ya go. :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Apache-1.3: question about mod_unique_id
On Tue, Sep 11, 2001 at 12:06:13AM -0500, William A. Rowe, Jr. wrote: >... > > where the code uses a pid (getpid()) you need to use a concatenation of > > pid and thread id. > > > > otherwise you violate some of the uniqueness guarantees. > > > > i just didn't bother trying to figure out how to get a thread id in any > > sort of portable way and left that to folks who actually wanted the module > > on those platforms. > > > > (this is actually mentioned in the comments near the top of the file > > describing how it works and the assumptions made.) > > As I've read. IIRC there are some other subtle issues with the size of the > unique key. Be careful that we keep everyone in sync. > > Seems that we now have 64 bit cpu's with potentially 64 bit pids. This adds > to the equation, it looks like we need to cut over to those extra four bytes > across all platforms. Oh, screw all that mess :-) Just use a UUID for the unique id. It has lots of bits, and it is already unique (by definition and without work). Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: what about proxy?
On Tue, Sep 11, 2001 at 04:11:16AM -0400, Chuck Murcko wrote: > > On Tuesday, September 11, 2001, at 04:01 AM, Graham Leggett wrote: > > >> Rather than doing that, why don't we work on the rollup mechanism > >> instead? > >> Proxy has been moving along terrifically while in its own CVS module. > > > > We can do this too - but then I'd say we would need to finalise the > > rollup release issue before we can go GA. Who is willing to sort out the > > rollup release? > > Most of the stuff to build a rollup release is already running in the > proxy builds. My suggestion is to take what you learned in proxy, and establish the httpd-rollup project (I posted on this earlier, something with "rollup" in the subject). Since you guys have already been doing it, then it makes sense to formalize that into a project. The project would mostly just be some scripts and things, but it could grab code from "anywhere" to roll together a release. Hell... we haven't even started talking about bundling some of our Java and XML tools with the web server. Lots of neat things that could happen there, too. I'm +1 on creating httpd-rollup, and -0.5 on putting proxy back in. [ if (since?) mod_ldap is going out, then we have to deal with the rollup anyways, which would totally kill any impetus for putting proxy back in. thus, I'm against bringing proxy back into the core. ] Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Hard coded HTTPD_ROOT in httpd.h...
On Wed, Sep 12, 2001 at 10:34:03PM -0400, Rodent of Unusual Size wrote: > * On 2001-09-12 at 21:11, > Ryan Bloom <[EMAIL PROTECTED]> excited the electrons to say: > > > > This is why we have config.layout. > > It is not just -f, it is -d as well. I think most installations use > either one of these or the default location set by the layout. > Not a problem, I think. If the hard-coded default is never used, then why don't we just toss it, and require people to pass/use a root? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: another map_to_storage gotcha.
On Thu, Sep 13, 2001 at 12:24:03PM -0400, Joshua Slive wrote: > > -Original Message- > > From: Ian Holsman [mailto:[EMAIL PROTECTED]] > > What I am suggesting is to move some of these configurations to > > where they belong. > > I don't see any config directives that "belong" in . > Please be more specific before you do anything. Options Indexes AllowOverride None Order allow,deny Allow from all AddHandler type-map var DirectoryIndex index.html index.html.var The only thing that is filesystem specific are: Options FollowSymLinks > > (BTW.. there are NO '' directives installed in the > > httpd-std.conf) > > There are commented out location directives for /server-status and > /server-info > > In general, I think this is proper. Most people serve content from the > filesystem. When serving content from the filesystem, should be > used in preference to for the reasons I mentioned a couple days > ago in another thread. We should not be encouraging people to use > except in specific circumstances. is different. It is setting global configuration for the whole server. It ensures that you don't miss some critical bits. And your cut/dry "Directory good. Location bad." does not hold. All of my configurations are Location based. That is because accessing the content through one location gives you DAV capabilities, and the other location gives your PHP- or SSI-processed content. With Subversion, we *only* have Location directives. There is absolutely nothing on the disk to serve. is an important directive and needs to be used in many, many cases. In this particular case, the problem arose because the attempt to set "global" options broke down. It was set only for one directory, not the whole server URL space. Using fixes that. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Apache-1.3: question about mod_unique_id
On Thu, Sep 13, 2001 at 09:35:43AM -0700, dean gaudet wrote: > On Tue, 11 Sep 2001, Greg Stein wrote: >... > > > Seems that we now have 64 bit cpu's with potentially 64 bit pids. This adds > > > to the equation, it looks like we need to cut over to those extra four bytes > > > across all platforms. > > > > Oh, screw all that mess :-) Just use a UUID for the unique id. It has lots > > of bits, and it is already unique (by definition and without work). > > i'm not convinced the UUID implementation in 2.0 is unique, or is as > robust as the technique mod_unique_id uses ... i started digging into this > months ago and never quite finished. hopefully i'll get back to it and > see if i'm right or wrong. Well, for Apache 1.3, you definitely can't use the APR UUID stuff :-) On Windows, for 1.3, you can simply call CoCreateGuid() and be done with it. And the Windows guids are unique. [ I seem to recall this thread started as a result of a Windows problem; UUIDs would solve it for that platform. maybe it was Netware? ] Regarding APR's UUID solution: if you think it isn't "good enough", then I'd be interested in knowing. It collects a decent amount of see and then hashes it to get a "random" result. If there is more seed data that we could use, then we should. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Apache-1.3: question about mod_unique_id
Does Netware have UUIDs or a similar thing? Cheers, -g On Thu, Sep 13, 2001 at 03:06:21PM -0600, Brad Nicholes wrote: >I checked with our CLIB people and they tell me that GetThreadID is *not* unique. > In other words, if a thread is created, it is assigned an ID. If that thread is >terminated and a new thread is created, the new thread may reuse the ID from the >terminated thread. > > Brad > > >>> [EMAIL PROTECTED] Thursday, September 13, 2001 2:57:08 PM >>> > Hi Bill, > > From: "dean gaudet" <[EMAIL PROTECTED]> > > Sent: Thursday, September 13, 2001 11:45 AM > > > >> > >> but on windows you'll need to use both getpid() and GetThreadID... i > >> think > >> it's called something similar to that. it may be "Get Thread Handle" ... > >> what you really want is the thread * that create_thread returns, or you > >> could use the scoreboard slot, but i don't see an obvious way to get at > >> either. also -- add an extra field to unique_id_rec for the thread id. > > > I suggest we add a gettid() if one does not exist, via the os.h/os.c for > > MULTITHREAD platforms. Anyone who wants to hack on this, just assume it's > > available, and where it isn't, someone needs to implement (I'm happy to do > > so for Win32.) > ok, sounds good. Then based on Dean's post and when Brad confirms that GetThreadID >is always unique for NetWare we have only to add: > #define gettid GetThreadID > > and in mod_unique_id.c we can remove the error warning and then change: > pid = getpid(); > to: > #ifdef MULTITHREAD > pid = gettid(); > #else > pid = getpid(); > #endif > > On the other side I saw that some other sources inside Apache use getpid(), is this >not a problem there too? And if only Win32 has an insufficient getpid() definition, >it may be enough to redefine this only there and leave all other as is...?? > > Guenter. -- Greg Stein, http://www.lyra.org/
Re: Apache-1.3: question about mod_unique_id
On Thu, Sep 13, 2001 at 01:50:50PM -0500, William A. Rowe, Jr. wrote: > From: "dean gaudet" <[EMAIL PROTECTED]> > Sent: Thursday, September 13, 2001 11:45 AM > > > but on windows you'll need to use both getpid() and GetThreadID... i think > > it's called something similar to that. it may be "Get Thread Handle" ... > > what you really want is the thread * that create_thread returns, or you > > could use the scoreboard slot, but i don't see an obvious way to get at > > either. also -- add an extra field to unique_id_rec for the thread id. > > I suggest we add a gettid() if one does not exist, via the os.h/os.c for > MULTITHREAD platforms. Anyone who wants to hack on this, just assume it's > available, and where it isn't, someone needs to implement (I'm happy to do > so for Win32.) For *Windows*, why bother? Use a UUID. You *know* it is unique. For other platforms, the discussion could be longer, but I don't understand why you would want to tie yourself in a knot when CoCreateGuid() is just calling out to solve this. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: UUIDs in 2.0
On Fri, Sep 14, 2001 at 08:35:45AM +0200, Sander Striker wrote: > > From: dean gaudet [mailto:[EMAIL PROTECTED]] > > Sent: 14 September 2001 07:57 > > > On Thu, 13 Sep 2001, Greg Stein wrote: > > > > > Regarding APR's UUID solution: if you think it isn't "good enough", then I'd > > > be interested in knowing. It collects a decent amount of see and then hashes > > > it to get a "random" result. If there is more seed data that we could use, > > > then we should. > > > > i have several complaints about UUIDs really, and i wanted to research > > more before posting (such as reading the justification of others for the > > particular implementation in unix/getuuid.c), but maybe you can save me > > the time: > > > > - one is their text representation length... 36 characters long when 20 > > or so is sufficient. Standard format. Within the source file, there is a reference to the Internet Draft for this stuff: http://www.webdav.org/specs/draft-leach-uuids-guids-01.txt See section 3.5 for the format. > I believe uuids are modelled after dcerpc uuids. There is a Paul Leach > draft on uuids floating around on the net. I would appreciate it if this > could stay, it would benefit other projects aswell. Sander: you should have read the code before posting. The reference to the draft is right in the code. > > - i don't understand md5 crypto enough to know the random properties of > > taking only 6 characters of a rather longer md5 result. (see > > get_random_info()) Uniform randomness. MD5 (and SHA-1) are very effective hashing functions. Thus, for a given input, the resulting bits are uniformly random. > > - "more secure because we don't pass out our MAC address" is security > > through obscurity :) it's better to say "we don't want to portably figure > > out our MAC address". However you want to phrase it. :-) I considered it more secure, but yah: it is merely obscurity. And then there is the real nasty bit about figuring out a MAC address :-) > Yes, I would like to see the MAC addr implementation aswell. This is > optional though. Non-portable, and not required. The draft is quite explicit about options for not using a MAC address. > > - /* crap. this isn't crypto quality, but it will be Good Enough */ > > uh huh. confidance inspiring. i hope the original UUID proposal says > > it's ok to use not quite crypto quality numbers here. Section 4 explicit discusses using various input bits and then hashing them with MD5 or SHA1. So yes... it *is* Good Enough. The situation is that we aren't trying for crypto strength, we're simply trying for randomness in the hope that our random result won't match anybody else's random result. No crypto needed there. The spec does say that crypto quality is best, but definitely not required. > The uuid draft states a truly random source of data should be used. > Crypto quality. No. See section 4. > > - why should srand() be used in the middle of a library routine? that's > > stealing the seed from the application -- people can't now generate > > repeatable code (which is an important debugging tool). It probably shouldn't. Good point. > > - get_current_time() is not thread safe As noted in the code, yes. >... > > i would say though that my single worst complaint is the size of the > > things, i rather like the base64 encoding over base16. but i understand > > that these are somewhat of a standard now? oh well. the thread safe > > thing should be fixed. The thread safety and the srand() thing. The code could also use some clarification and simplification. It is still a bit weirdly organized, because of its way-back origin in the sample implementation from the draft. Re: the size: you have the raw 16 bytes available. You could do a base64 encoding yourself, if you wanted to. But the provided functions will just give you the standard format. *shrug* Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Tag time?
On Sun, Sep 16, 2001 at 04:46:08PM -0400, Cliff Woolley wrote: > On Sun, 16 Sep 2001, Justin Erenkrantz wrote: > > > Not that I volunteer to be RM or anything, but any reason why we > > couldn't try to get a beta out this week? -- justin > > I was going to send out a message yesterday asking what the remaining > showstoppers were. Did we ever track down that segv that surfaced when > using mod_php and then others? I need to go back and find that email > where an example of a troublesome config was given. What other problems > are out there? It'd be nice to wrap up this buckets alloc thing before a > new tag since it involves API changes, but I guess it's not a showstopper. Apache's prefork MPM seems to be having problems with persistent connections. They've been dogging some of my testing and SVN. I'm gonna dig into it this evening. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
This is an ugly bug. * open a connection to apache. toss in 100 GET requests. no problem. * try the same, but with "hello" in the body of the GET request. Apache closes the connection after sending back the first response. Of course, substitute your favorite POST, PUT, PROPFIND, or whatever other method that contains a body. Bam! Apache is gonna close the connection. "wtf?" you say... Well, I tracked it down to a problem with the HTTP_IN filter and its call to apr_brigade_split() on line 690. The new, returned brigade is created in the same pool as "b" -- that pool is the request pool. Thus, at the end of the request ctx->b is cleared out, and on the next entry to the HTTP_IN filter (read_request_line for the next request), it tries to refill ctx->b with data. CORE_IN punts with APR_EOF, so the request logic thinks "done. let's shut down." There are a few band-aid solutions to the problem: 1) ctx->b->p = f->c->pool (and somehow tweak the cleanups) 2) copy the returned brigade into a properly-allocated brigade 3) pass a pool to apr_brigade_split() for the new brigade IMO, the right answer is to fix up the input processing so we don't even have to do all of that splitting and brigade munging. (otherwise, (3) is the better of the above options) Cheers, -g On Mon, Sep 17, 2001 at 11:22:53AM -, [EMAIL PROTECTED] wrote: > gstein 01/09/17 04:22:53 > > Modified:.STATUS > Log: > Found the persistent connection problem I've been observing. Nasty result of > the problems in the input filter stack. Time to rejigger, per the > discussions we've had. > > Revision ChangesPath > 1.296 +24 -1 httpd-2.0/STATUS > > Index: STATUS > === > RCS file: /home/cvs/httpd-2.0/STATUS,v > retrieving revision 1.295 > retrieving revision 1.296 > diff -u -r1.295 -r1.296 > --- STATUS 2001/09/17 02:33:40 1.295 > +++ STATUS 2001/09/17 11:22:52 1.296 > @@ -1,5 +1,5 @@ >APACHE 2.0 STATUS: -*-text-*- > -Last modified at [$Date: 2001/09/17 02:33:40 $] > +Last modified at [$Date: 2001/09/17 11:22:52 $] > >Release: > > @@ -107,6 +107,29 @@ > >This builds mod_headers as a DSO (good) but builds mod_mime >as a compiled-in module (bad). > + > +* revamp the input filter semantics, per discussions since > + February (and especially at the hackathon last > + April). Specifically, ap_get_brigade will return a brigade with > + *up to* a specific number of bytes, or a "line" of data. The > + read may be blocking or nonblocking. ap_getline() will be > + refactored into apr_brigade_getline(), and then DECHUNK can use > + f->next (ap_getline will always read "top of input stack"). Also > + fix the bug where request body content will end up closing the > + connection (buggering up persistent conns). > + > + - socket bucket and core input filter changes. see end of > +message ID (Feb 27): <[EMAIL PROTECTED]> > + > + - fix up ap_get_brigade() semantics, fix bug in DECHUNK / > +ap_getline. many messages (plus their threads) (Apr/May): > + Message-ID: <[EMAIL PROTECTED]> > + Message-ID: <[EMAIL PROTECTED]> > + Message-ID: <[EMAIL PROTECTED]> > + > + - further work with combining/tweaking the builtin filters: > + Message-ID: <[EMAIL PROTECTED]> > + > >RELEASE NON-SHOWSTOPPERS BUT WOULD BE REAL NICE TO WRAP THESE UP: > > > > -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Switch back to SIGUSR1 and use SIGWINCH on Linux 2.0
On Mon, Sep 17, 2001 at 11:38:25PM -0700, Justin Erenkrantz wrote: >... > Since there is a veto against the use of SIGWINCH by default, the > current code in CVS is broken. I would like to resolve this > before we tag and roll again. -- justin By definition, it must be resolved before the next release. Of course, we could tag/roll, but release? nope :-) Personally, I'm in favor of tossing SIGWINCH, too. Kind of sucks to tell Apache to not detach from the terminal, and have it go wonky when I resize the bugger. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Mon, Sep 17, 2001 at 03:52:21PM -0700, Jon Travis wrote: > I've got a bit of code that needs to run after a connection to a client > has been closed. Right now I can (kind of) spoof this by setting the > keepalive for the client to 0, and registering a cleanup on the > request_req pool. Unfortunately the code in there is somewhat bulky, > so any subsequent cleanups that it registers will never get called > (apparently registering a cleanup within a cleanup no workie). > > I'd like to propose a new hook which gets run after connection to > the client has been closed. (in my case, I run some cleanup code > which can take a while, and would like the client to be on its way). > > Any thoughts? Yes. :-) You've confused the issue with your subject line (everybody is bugging out because they're relating it to logging). It should not have anything to do with "log". We have a pre-connection hook, so call yours post-connection. That is when you want to run the hook, so we're all set. Now the question is whether to run it before or after the ap_lingering_close call. If it is before, then connection.c::ap_process_connection can do it. If it is after, then there are two options: 1) make all MPMs run the hook 2) move the ap_lingering_close inside ap_process_connection, then call it from with ap_process_connection. This *almost* works. All MPMs have a call to ap_process_connection followed by a call to ap_lingering_close. The only MPM that does other funny stuff in there is the winnt MPM. Conceivably, you could pass a flag that says "I'll manage the shutdown, thankyouvermuch", and winnt would do its close followed by the post-connection hook. Note that I *would* think that it should run afterwards, to ensure the client has got all the data and has fully detached. And all your spoofy nonsense can stay in your module, but it can't go in the core. That would just be too hard to understand and maintain over the long haul. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/ssl Makefile.in
This shouldn't be needed... isn't the whole point of VPATH to look in $(top_srcdir) if the file isn't present in the build dir? The value passed to flex would need the $(top_srcdir) since Make can't get in there and fix it, but the dependency line "shouldn't" need the change. Cheers, -g On Tue, Sep 18, 2001 at 08:31:37PM -, [EMAIL PROTECTED] wrote: > rbb 01/09/18 13:31:37 > > Modified:modules/ssl Makefile.in > Log: > Allow VPATH builds for SSL. > Submitted by:Cody Sherr <[EMAIL PROTECTED]> > > Revision ChangesPath > 1.3 +4 -4 httpd-2.0/modules/ssl/Makefile.in > > Index: Makefile.in > === > RCS file: /home/cvs/httpd-2.0/modules/ssl/Makefile.in,v > retrieving revision 1.2 > retrieving revision 1.3 > diff -u -r1.2 -r1.3 > --- Makefile.in 2001/05/05 13:21:52 1.2 > +++ Makefile.in 2001/09/18 20:31:37 1.3 > @@ -57,12 +57,12 @@ ># (we really don't expect end users to use these targets!) ># > > -ssl_expr_scan.c: ssl_expr_scan.l ssl_expr_parse.h > - flex -Pssl_expr_yy -s -B ssl_expr_scan.l > +ssl_expr_scan.c: $(top_srcdir)/modules/ssl/ssl_expr_scan.l ssl_expr_parse.h > + flex -Pssl_expr_yy -s -B $(top_srcdir)/modules/ssl/ssl_expr_scan.l > sed -e '/$$Header:/d' ssl_expr_scan.c && rm -f >lex.ssl_expr_yy.c > > -ssl_expr_parse.c ssl_expr_parse.h: ssl_expr_parse.y > - yacc -d ssl_expr_parse.y > +ssl_expr_parse.c ssl_expr_parse.h: $(top_srcdir)/modules/ssl/ssl_expr_parse.y > + yacc -d $(top_srcdir)/modules/ssl/ssl_expr_parse.y > sed -e 's;yy;ssl_expr_yy;g' \ > -e '/#if defined(c_plusplus) || defined(__cplusplus)/,/#endif/d' \ > ssl_expr_parse.c && rm -f y.tab.c > > > -- Greg Stein, http://www.lyra.org/
Re: Q1: Rollup Release Format
On Wed, Sep 19, 2001 at 01:05:27AM +0200, Graham Leggett wrote: >... > Ok - first question - what do we call the rollup release: >... > o Option B: apache-2.x.x.tar.gz, apache-modules-2.x.x.tar.gz > > Combine httpd-2.0, apr, apr-util into apache-2.x.x.tar.gz, > and combine httpd-proxy, httpd-ldap into apache-modules-2.x.x.tar.gz. +1 -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c fdqueue.h
On Tue, Sep 18, 2001 at 11:09:12PM -, [EMAIL PROTECTED] wrote:
> rbb 01/09/18 16:09:12
>
> Modified:.CHANGES
>server/mpm/worker fdqueue.c fdqueue.h
> Log:
> Turn the worker MPM's queue into a LIFO. This may
> improve cache-hit performance under some conditions.
>
> Submitted by: Aaron Bannert <[EMAIL PROTECTED]>
If you're just using a tail and LIFO, then this is more like a stack.
>...
> --- fdqueue.c 2001/09/18 21:14:18 1.6
> +++ fdqueue.c 2001/09/18 23:09:12 1.7
> @@ -73,7 +73,6 @@
> */
>static int ap_queue_empty(fd_queue_t *queue)
>{
> -/*return (queue->head == queue->tail);*/
>return (queue->blanks >= queue->bounds - 1);
What are ->blanks and ->bounds for? The test is easier done as:
return queue->tail == 0;
Toss ->blanks and rename ->bounds as size or capacity or something. It is
only an upper-bound, so "bounds" isn't quite right semantically.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
I agree with OtherBill. Cleanups are not always the answer. When they are run, many things associated with that pool could be torn down already because their cleanups have already run. If you need a known state to perform *operations* (as it sounds like Jon is doing), then you can't use a cleanup. Hooks are orderable, but more importantly: they run at a specified time. In this case, before the pool is cleaned up. Lingering close should not be part of the connection pool cleanup. It is a specific action that needs to occur *before* we are done with the connection pool. That is why Bill's suggestion of running it as a hook is great. It also gives people a chance to perform actions relative to that close and before pool cleanup occurs. Cheers, -g On Tue, Sep 18, 2001 at 02:20:35PM -0500, William A. Rowe, Jr. wrote: > From: "Ryan Bloom" <[EMAIL PROTECTED]> > Sent: Tuesday, September 18, 2001 11:44 AM > > > > On Tuesday 18 September 2001 08:17 am, William A. Rowe, Jr. wrote: > > > Why not let the MPM register the lingerclose with APR_HOOK_MIDDLE in the > > > post_connection hook? That way, if Jon's (or any other author's) intent is > > > to work before the lingering close, then it can be APR_HOOK_FIRST. > > > Otherwise register it APR_HOOK_LAST. > > > > It shouldn't be a hook. This should just be done with a pool cleanup. Hooks > > aren't the answer to every problem in the server. Doing something after a > > specific action, like the close of the connection should be done by registering > > a pool cleanup. Fix the bug that you can't register a cleanup within a cleanup, > > and Jon's problem goes away completely, because he can use the cleanup > > that he is already using. > > The pool cleanup has one disadvantage (assuming the register cleanup within cleanup > bug is fixed), the order of cleanups is a strict LIFO. > > There _may_ be an advantage to an orderable hook. At this point I agree, fix the > register cleanup in cleanup bug, let Jon experiment with that solution, and then > argue the merits for a new hook. > > Bill -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Tue, Sep 18, 2001 at 06:52:35AM -0400, Jeff Trawick wrote: > Greg Stein <[EMAIL PROTECTED]> writes: > > > 2) move the ap_lingering_close inside ap_process_connection, then call it > >from with ap_process_connection. This *almost* works. All MPMs have a > >call to ap_process_connection followed by a call to ap_lingering_close. > >The only MPM that does other funny stuff in there is the winnt MPM. > >Conceivably, you could pass a flag that says "I'll manage the shutdown, > >thankyouvermuch", and winnt would do its close followed by the > >post-connection hook. > > It is nice for ap_lingering_close() to be handled in the MPM. It > shouldn't be too hard for the MPM to implement its own lingering close > in a manner that doesn't consume one thread per closing connection > (e.g., dedicate one thread per MPM to do all the lingering close > logic). Of course there could always be another hook :) Great idea, and yes: switching to a hook would still provide for a complex lingering close capability. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, Sep 19, 2001 at 06:34:11AM -, [EMAIL PROTECTED] wrote: > jwoolley01/09/18 23:34:11 > > Modified:server/mpm/worker worker.c > Log: > I was kinda hoping those (void)some_function() and (request_rec *)NULL > casts would go away before this committed, but alas I didn't say anything. > :-) This gets rid of them and a few others just like them that I also > found in worker.c. Um. I'm leaning towards a -1 on removing those (void) casts. The functions return a value, and it is not being used. The (void) makes it clear that the values are being discarded. And generally, a discarded value isn't a Good Thing, so those (void) casts leave markers for somebody to fix the code at some future time. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c fdqueue.h
On Tue, Sep 18, 2001 at 06:33:14PM -0700, Aaron Bannert wrote:
> On Tue, Sep 18, 2001 at 06:01:35PM -0700, Greg Stein wrote:
>...
> > >...
> > > --- fdqueue.c 2001/09/18 21:14:18 1.6
> > > +++ fdqueue.c 2001/09/18 23:09:12 1.7
> > > @@ -73,7 +73,6 @@
> > > */
> > >static int ap_queue_empty(fd_queue_t *queue)
> > >{
> > > -/*return (queue->head == queue->tail);*/
> > >return (queue->blanks >= queue->bounds - 1);
> >
> > What are ->blanks and ->bounds for? The test is easier done as:
>
> remnants of when fdqueue was more complicated and offered more functions.
Understood. I meant "there is more cleanup that can be done."
> > return queue->tail == 0;
> >
> > Toss ->blanks and rename ->bounds as size or capacity or something. It is
> > only an upper-bound, so "bounds" isn't quite right semantically.
>
> I've been had for name changes in the past, so I opted for the least changes
> to reach correctness. As soon as these other major worker changes start
> settling down, I'll definately go through and do names changes and minor
> format fixes where appropriate for posterity.
Name changes are no big deal if you're improving clarity. Gratuitous changes
are usually bad :-)
In this case, name changes are going to be very localized, so it's easy. And
tossing ->blanks also simplifies things. ->tail and ->blanks are redundant.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Wed, Sep 19, 2001 at 11:17:36AM -0500, William A. Rowe, Jr. wrote: > From: "Ryan Bloom" <[EMAIL PROTECTED]> > Sent: Wednesday, September 19, 2001 9:52 AM > > > > On Tuesday 18 September 2001 06:09 pm, Greg Stein wrote: > > > I agree with OtherBill. > > > > > > Cleanups are not always the answer. When they are run, many things > > > associated with that pool could be torn down already because their cleanups > > > have already run. > > > > Jon is already using a pool cleanup to solve this problem. In fact, his > > initial thought was to use a pool cleanup, but he couldn't register cleanups > > from within a cleanup. Adding a new hook was a second solution from Jon, > > because he just needed to solve his problem. > > Note (for those not paying that close attention) just trying to open a file within > a cleanup, do something, and close it will attempt to create a cleanup :( > > This bug still must be fixed. It isn't a bug. Cleanups are for just wrapping things up, not doing work. You might not even be able to open and use that file if your pool is in the process of being destroyed. Doing significant work at cleanup time, *especially* things which involve that same pool, is flat out prone to bugs. The cleanups are not ordered (they shouldn't be), and the fact they are running means that the pool's usability is marginable. Note that the pool's subpools have already been destroyed. It isn't a bug. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote: > Greg Stein wrote: > > It isn't a bug. Cleanups are for just wrapping things up, > > not doing work. > > If that's the authoritative answer, then we need to provide > a supported way for 'doing work' at cleanup time. > > > You might not even be able to open and use that file if > > your pool is n the process of being destroyed. > > That sounds like utter tripe. If you can't depend on the > pool lasting at least until your cleanup routine ends, > then the whole cleanup mechanism is seriously borked. AFAIK > it isn't, so I think the above assertion *is*. The problem is cross-dependency between the cleanup actions. One can destroy something another cleanup needs. If you restrict their actions to "simple" things, then the cross-dependencies are (hopefully!) removed. Consider a platform that creates a subpool to manage a set of file descriptors for the apr_file_t objects. That subpool is linked from the userdata of specified pool. At cleanup time, that subpool has been tossed, so you cannot create files on the given pool. Or maybe it is as simple as APR registers a cleanup on the pool to do some cleanup, which clears out some basic structure needed for creating FOO objects. And that APR's cleanup runs before yours. The basic issue is that cleanups are generally non-deterministic. A cleanup could run before another and blast some basic service, preventing the second cleanup from doing "work". Think about database connection pools, shared memory stuff, temporary files, etc. The list goes on and on. That said: if you want to create a second list of "do some work when the pool is cleared" type thing, then fine. As long as its understood that *that* list is for non-destructive work, and the cleanups are for destructive work. The work-list can run before subpools are tossed, which means the pools are "unchanged". Cheers, -g p.s. "utter tripe" indeed... that was rather inflammatory... -- Greg Stein, http://www.lyra.org/
pool cleanup (was: Re: New post-log-transaction hook?)
On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote: > On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote: > > From: "Greg Stein" <[EMAIL PROTECTED]> > > Sent: Wednesday, September 19, 2001 1:26 PM >... > > > The problem is cross-dependency between the cleanup actions. One can > > > destroy something another cleanup needs. If you restrict their actions to > > > "simple" things, then the cross-dependencies are (hopefully!) removed. > > > > Really? No. Cleanups are run as a LIFO stack. Anything that existed when > > something was added to the pool must exist when that something is removed > > from the pool. They are not strictly LIFO. You can remove a cleanup and insert a new one at any time. Let's say that the cleanup list looked like: cleanups: A and you add a new one to the "front": cleanups: B A and now case 1, where A needs to rejigger its cleanup param a bit: cleanups: A' B or case 2, where A simply removes its cleanup: cleanups: B Case 2 actually happens quite often. > > IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so > > that will also be true of subpools. Subpools are entire entities. There shouldn't be any cross-dependencies between those. Care in ordering isn't necessary. > > Considering how we use pools for dynamic libraries and the rest, it's > > absolutely vital that they are unspun from the pool in LIFO order of their > > creation. Ah. That is a good example... had forgotten about that one (I even ran into this once). Using the symbology from above, what happens if B is using code from a dso that installed A? What happens if A is unloaded and reloaded, thus rearrange the queue as in case 1. The point is: I've observed problems with cleanup ordering. And the DSO thing is particular nasty. At one point, I was considering adjusting Apache's pool structure to better order when the DSOs were unloaded. For example, if we loaded DSOs into Pool X and then used subpool Y for all module-related allocations (e.g. pconf == Y and X is internal). > I agree with Bill. Having reviewed the code quite deeply yesterday, pool > cleanups follow a very clean rule, and registering a cleanup from within > a cleanup will always work if done correctly. Huh? If you register a cleanup within a cleanup, it *won't* be run. We grab the list, then run it. Most types of changes to that list won't be seen; particularly insertions at the front of it. > If you have data in a pool > when the cleanup is registered, it is gauranteed to be there when the cleanup > is run. The pool data might, but associated resources may not be. For example, the apr_file_t structure might still be in the pool (simply cuz it can't be freed), but the underlying file descriptor could be closed. > Anything else is completely broken. Depends on your interpretation :-) I think the cleanup behavior is very well defined, and has certain restrictions on the kinds of things that you can do in there. And one of those restrictions is watching out for dependending on something else in your pool. As a result, arbitrary work can definitely be affected. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Q1: Rollup Release Format - Score So Far...
On Wed, Sep 19, 2001 at 04:32:12PM -0400, Joshua Slive wrote: > > -Original Message- > > From: Graham Leggett [mailto:[EMAIL PROTECTED]] > > > The winner seems to be that the Apache group releases a rollup release > > with all (at least apr, apr-util, httpd-2.0, httpd-proxy, httpd-ldap) > > the projects included, and that release is called "apache-2.x.x.tar.gz". > > Am I right on this one? > > I have no problem with that (although it seems that the only difference > between this and what the group has always done is that everything lives in > different cvs repositories). Seems that way. We can have multiple CVS repositories and then make decisions on which ones go into the rollup. For example, we could include mod_proxy and mod_ldap repositories, but exclude the mod_pop3 repository. And on into the future... > But I think we also have concensus that the name shouldn't be "apache". > "apache-httpd-2.x.x.tar.gz" seems better. Agreed. I am +0 on httpd-2... and +1 on apache-httpd-2... btw, no dates in those either (a suggestion from otherbill). The version number tells us what we need to know. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, Sep 20, 2001 at 12:53:39AM -0700, Alex Stewart wrote: > On a largely unrelated note, but something I found a little ironic given > the nature of this list: > > Aaron Bannert wrote: > > > >http://www.apachelabs.org/apache-mbox/199902.mbox/<[EMAIL PROTECTED]> > > Please note that the above is not a valid URL. Specifically, the "<" Agreed. >... > Whoever does the software behind apache-mbox (I take it this is > mod_mbox?) might want to take note that it's spitting out invalid URLs.. The URLs produced by mod_mbox are fine. Aaron must have posted an unescaped version of the URL. (go to a mod_mbox page and view the source...) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
>...
> Does this fix it for you? All testmem tests passed for me and your code
> above properly flushes "Cleanup" to the file.
>
> (Someone needs to check my work on run_child_cleanups() and make sure
> that the popping is necessary. It looked to be in the same situation.)
Calling pop_cleanup() on every iteration is a bit much. Consider the
following patch:
while ((c = p->cleanups) != NULL) {
p->cleanups = NULL;
run_cleanups(c);
}
You don't even have to change run_cleanups or run_child_cleanups.
Basically, the above code processes the cleanups in batches. Everything that
was initially registered is processed, then everything registerd during the
first cleanup round, etc.
It does not maintain the LIFO behavior where cleanup A registers cleanup B
and expects B to run *just after* cleanup A finishes. If A wanted that, then
it could just calll B. But the most important part: all cleanups *do* get
run.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: pool cleanup
On Thu, Sep 20, 2001 at 07:02:58AM -0700, Ryan Bloom wrote: > On Wednesday 19 September 2001 02:21 pm, Greg Stein wrote: >... > > They are not strictly LIFO. You can remove a cleanup and insert a new one > > at any time. Let's say that the cleanup list looked like: > > > > cleanups: A > > > > and you add a new one to the "front": > > > > cleanups: B A > > > > and now case 1, where A needs to rejigger its cleanup param a bit: > > > > cleanups: A' B > > > > or case 2, where A simply removes its cleanup: > > > > cleanups: B > > > > > > Case 2 actually happens quite often. > > This is all true, but it is also orthogonal to this conversation. Partly. The conversation moved into "what can you do in a cleanup". If you want to look at the simple issue of registering cleanups... okay. But when people were expecting to be able to do "anything" in a cleanup... that is intrinsically incorrect. > The question we are > trying to answer here, is can you register a cleanup within a cleanup. Aaron posted a patch, but it introduces too many function calls in the processing. I posted one that is much more optimal, processing the cleanups in batches. That would fix your issue. > If we are in > the middle of running the cleanups, and somebody actually calls cleanup_run > or cleanup_kill from within a cleanup, they are broken and it may not work. My above case wasn't talking about doing those from within a cleanup (which is definitely and always wrong). I was showing how the cleanups could be reordered; therefore, how you cannot depend upon particular cross-cleanup ordering dependencies. Thus, you are actually somewhat limited in what kinds of things you can truly do in a cleanup. > It also doesn't make any sense, because the reason to run a cleanup, is to perform > some action sooner than you would have otherwise, but in this case, we are going > to perform that action in a few seconds anyway. I don't get this part. A cleanup is to do just that: clean up after yourself when the pool goes away. It provides a point in time for specific types of actions. I'm not sure how that gives you "sooner"; if anything, a cleanup is for running things later. > Since the two cases above require a programer to either remove or run a cleanup, > they don't really make sense in the context of registering a cleanup within a >cleanup. > This means that is safe to register a cleanup within a cleanup, assuming the code > is patched correctly. Agreed. My point was addressing the "arbitrary work in a cleanup" meme that was brought up. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three
>assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
quot;
> > - ### it presumes that the next filter up will *only* call us
> > - ### with readbytes set to the Content-Length of the request.
> > - ### that may not always be true, and this code is *definitely*
> > - ### too presumptive of the caller's intent. the point is: this
> > - ### filter is not the guy that is parsing the headers or the
> > - ### chunks to determine where the end of the request is, so we
> > - ### shouldn't be monkeying with EOS buckets.
> > -*/
> > -if (*readbytes == 0) {
> > -apr_bucket *eos = apr_bucket_eos_create();
> > -
> > -APR_BRIGADE_INSERT_TAIL(b, eos);
> > -}
Somebody has to return an EOS somewhere.
Consider the highest-level input filter. How does it know when it has read
all of the data for the current request? It needs to see an EOS bucket.
I'll wait for the new patch and review again. All the formatting changes
were getting in the way.
But I really think the proper tack is to fix the amount-returned, and to
combine the HTTP_IN and DECHUNK filters.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 08:34:00PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 06:58 pm, Justin Erenkrantz wrote: >... > > Other significant changes is that the HTTP_IN (ap_http_filter) is > > now a request-level filter rather than a connection-level filter. > > This makes a lot more sense to me. +1 >... > I don't see how this works at all for pipelined requests. You moved the HTTP_IN > filter from a connection filter to a request filter. But, the only connection filter >we > have is the CORE_iN filter. All that filter does, is to pass the socket up to the > HTTP_IN filter. The problem is that HTTP_IN will be destroyed in between requests, > so the second request will be lost. I am continuing to read the code, so I may see > what I am missing, but in the mean time, Justin, if you could explain that to me, > it would make my review go a lot faster. There is a create_request hook which inserts the HTTP_IN filter when the request is created. This seems like a good idea: each instantiation of the HTTP_IN filter is for *one* request and one request only. We can be much more sure that state is not spanning requests. Even better, this enables things like the Upgrade: header (RFC 2616, S14.42). One request specifies an upgrade, and the next request should be handled by a different protocol. [ of course, that still won't work since we'll continue inserting the HTTP_IN filter for each request, but making HTTP_IN a request filter takes us a step in the right direction. ] Along the mantra of "an input filter should not return more than is asked for", we can also be sure that the HTTP_IN filter is going to receive exactly the amount of data that it asked for, and no more. Thus, the next request will still be sitting in the next filter down. Okay. Now all that said. Some random notes about the current code: * CORE_IN is returning a new socket bucket each time. That socket bucket goes into a brigade associated with the request, so it gets destroyed with the request. Not really a problem since the actual socket is cleared by a pool, not by the bucket. When the next request asks for more data, it will get a new socket bucket. I don't think this is The Right Thing, but until we pass an "amount requested" parameter through the input filter chain, this will work quite fine. * The uses of ap_getline are destined to eventual failure, which I've noted before. They always read from the top of the filter stack -- even if you have a mid-level filter (HTTP_IN) trying to read a line(!). This is solved again by passing "amount requested" down thru the input chain, where one of the "amounts" is a qualified constant saying "one line". We support that read-request mode through a brigade function to read a line into a pool-allocated buffer, into a provided buffer, or maybe into a new brigade (e.g. split a brigade at the newline point, and return the initial brigade). I need to review the code some more. As Justin pointed out, the massive change to http_protocol.c makes that hard to review. Other notes may be coming soon... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
On Sun, Sep 23, 2001 at 03:04:54PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 02:53 pm, Greg Stein wrote: > > On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote: > > > None of this can be committed until it has passed all of the tests in the > > > perl test framework. The last time somebody (me) mucked with these > > > filters it broke all of the CGI script handling. I would also like to > > > hear from Jeff about this, because when we wrote the filters originally, > > > we had input filtering use this design, and Jeff and Greg were adamant > > > that filters needed to be able to return more than they were asked for. > > > > Euh... not sure if you typoed. Input filters should *NOT* return more than > > they were asked for. Never. > > > > If a filter did that, it could end up passing data up to a filter that is > > not responsible for that data. For example, reading past the end of a > > request, or reading past the headers yet that portion of the body was > > supposed to go into a newly-inserted unzip or dechunk filter. > > > > I can think of more examples, if called upon :-) > > And I am saying, that was the original design, and Jeff Trawick and Greg > Ames Ah. An unqualified "Greg" in your post :-) Shouldn't he be OtherGreg, following in OtherBill's footsteps? hehe... > posted a couple of examples that proved that design incorrect. The typical example that I recall was related to decompression filters. "If my caller asks for 100 bytes, then I read 100 bytes from the lower level, and decompress to 150 bytes, then my caller is going to receive 150 bytes." The correct answer is "return the 100 bytes they asked for, and set the other 50 aside for the next invocation." Consider the case where you have a multipart message coming in, and the entire message is compressed (a Transfer-Encoding is used for the message; Content-Encoding is used to compress the entities in the multipart). Next, let us say that each multipart is handled by a different filter (say, because they are handling different Content-Encodings, such as base64 or quoted-printable). To apply some symbology here: M is the message-body filter which is decompressing the message according to the Transfer-Encoding header. E1 and E2 are the entity-related filters, handling each entity in the multipart message. Now, let's say that E1 asks for 100 bytes. M returns 150. But E1 *only* wanted 100 bytes. Those other fifty were supposed to go to E2. Oops :-) Instead, M should decompress the incoming data, return 100 (to E1) and put the other 50 aside for the next filter invocation. E2 comes along and asks for 300 bytes. We return the 50 we had set aside. (M might decide to read more data, but it is also allowed to return less than asked for, assuming the caller will simply ask again for more) If Jeff/OtherGreg :-) have an example where returning more than asked is *required*, then it should be brought up again. I can't imagine a case where it would ever be safe to return more than is desired. [ other examples may relate to character decoding, where you want to avoid returning a partial multibyte character, so you return N+3 to get the rest of the character in the response; I say that you return N-1 and put rest of the character aside for the next invocation ] Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 09:27:58PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 09:21 pm, Greg Stein wrote: >... > > There is a create_request hook which inserts the HTTP_IN filter when the > > request is created. > > > > This seems like a good idea: each instantiation of the HTTP_IN filter is > > for *one* request and one request only. We can be much more sure that state > > is not spanning requests. > > No, this is a bad idea, and I still don't see how it works for pipelined requests. > The problem is that it is impossible to make sure that you only read a specific > amount from the socket. Whoops. Hell ya... you're so right. All right. Until CORE_IN can return only a specified amount, the new, combined filter should remain a connection filter. The associated per-request logic in this patch should go away. The code will put the socket bucket (and other unread/unused data) into the filter's context, which will survive until the next request. Justin... want to try a third pass? :-) btw, does the test suite actually try out persistent connections? There was a problem with persistent connections for a good long while, until I fixed it last week. (while I like the flatten function, maybe we can leave that out of this patch for now; shoot for that one at another time, and replace similar uses across the board (rather than one spot)) In some future pass, we can reinstate the per-request behavior, but that is a discussion for another time... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 09:35:31PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 09:22 pm, Justin Erenkrantz wrote: > > On Sun, Sep 23, 2001 at 08:34:00PM -0700, Ryan Bloom wrote: >... > I don't see this at all. It is not possible to specify how much is read from the > socket itself. I just read through the bucket_socket code, and we always > try to read APR_BUCKET_BUFF_SIZE. This seems to mean that we are > going to read more from the socket than we should, and the second request > will be lost. Ryan's quite right. >... > > register a cleanup with the request pool that setasides everything > > that was read but not processed (i.e. all buckets except for the > > socket) to now live as long as the connection pool. The question > > now becomes how do we retrieve this data back. My first guess would > > be to store it via a pool userdata option, but maybe we could shove > > it back down into the core (which definitely breaks some abstractions). > > So, I think I'm leaning towards the userdata - use "HTTP-IN-" > > to specify the uniqueness should work. Eww. No... just move the filter back to the connection and you're fine. We can make it a request filter later on. > No. I fought against this VERY strenuously last time, and I will fight it again. Me, too :-) > This is why it needs to be a connection filter. To keep the abstraction valid, > the data being read from the socket is stored in the filter's context, the same > way it is for the output filters. For now, yes: a connection filter is appropriate. Later, when we can guarantee that the request-level filter will never read past the end of the request (i.e. it never requests more from the lower-level input filter), then we can move it to a request filter (which, as I mentioned before has interesting properties). Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
const char *tenc, *lenp;
> +tenc = apr_table_get(f->r->headers_in, "Transfer-Encoding");
> +lenp = apr_table_get(f->r->headers_in, "Content-Length");
> +
> +if (tenc) {
> +if (!strcasecmp(tenc, "chunked")) {
> +char line[30];
>
>...
> +if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> +return rv;
> +}
> +ctx->state = BODY_CHUNK;
> +ctx->remaining = get_chunk_size(line);
The above section of code, and some of the stuff following is duplicated
from ap_setup_client_block(). There should be a comment about this
duplicated code. Especially because this copy doesn't contain all of the
error checking and responses of the ap_setup_client_block version. A next
step would be to figure out how to remove that duplication.
Specifically, I'd think that we figure out some of this information when we
insert the [request form of this] filter. The ap_read_request() function
would set up the filter context and set it when the filter is added; the
context would indicate the particular parameters for body reading.
[ maybe at some point in the future, the filter can read the request line,
headers, body, etc, and do all of the computation and checking itself ]
>...
> +else if (ctx->state == BODY_CHUNK)
> +{
> +char line[30];
> +
> +ctx->state = BODY_NONE;
>...
> +/* We need to read the CRLF after the chunk. */
> +if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> +return rv;
> +}
Is the BODY_NONE thing so that the ap_getline() recursion thingy works
properly, and returns a line?
>...
> +/* This had better be equal to zero now! */
> +if (ctx->status)
> +ctx->remaining -= total;
What better be zero? status or remaining?
I'm not sure why ctx->status would ever be zero. Could you explain? Is that
at some point where r->status becomes OK (rather than HTTP_OK). A lot of
stuff seems to key off ctx->status != 0. I don't see why...
>...
> AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t
>bufsiz)
Probably just leave this be, since we don't have the apr_brigade_to_buffer
function for now.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
