RE: cvs commit: httpd-2.0/server util_filter.c
> rbb 02/03/04 21:21:13 > > Modified:server util_filter.c > Log: > Stop the loop when we have found the correct filter, or when the filter > list is over. Do not stop if the next filter is NULL. This fixes the infinite loop and the seg fault. Sorry it took so long, the code was a bit of a nightmare. Ryan
Re: cvs commit: httpd-2.0/server util_filter.c
At 11:54 AM 3/3/2002, you wrote: >I think you may have missed my message yesterday. It's not adding a >hook - it's allowing a hook to be able to abort processing of a >request. That's all. internal_redirect will take care of >everything. fast_redirect is bogus because it skips hooks in the >new request. I believe that is broken behavior. If we were to >rely on internal_redirect, everything falls into place - without >having to add all of this protocol_filters code that unnecessarily >complicate matters. Justin, yes - it adds complexity. But we can't simply 'abort processing' to speed things up. Perhaps we need to do so, as well, but I'm simply responding to 'this is bogus'. Perhaps it is. However, again consider mod_dir. We have a request. It asks for a dir. We find the dir, and look at DirectoryIndex. We look for the first item, 'index.php'. Nope, no php, destroy that subrequest. We look for 'index.html'. We find that file. In checking for that file, we have run through the process_request cycle. What you are asking is that we re-invoke the entire process_request cycle. That is doable, but it will be costly. And -that- cost, the cycles to completely repeat process_request() for index.html, is why I changed my focus from throwing this code path away, to making this code path work correctly, if it is at all possible or realistic. Rbb's derivative of your fix of my patch identified and solved the filters problem. It helped Ryan notice that we were -really- messed up on r->connection->output_filters never growing any connection filters added to r->output_filters. That was bogus, I'm glad to see it now fixed. There should be no problem 'appropriating' the connection+protocol filters from the subreq->main request and using them to serve the subreq as the top level request. If you see such an issue, please, raise it. Bill
Re: cvs commit: httpd-2.0/server util_filter.c
On Sun, Mar 03, 2002 at 10:20:52AM -0800, Ryan Bloom wrote: > Was that the one about returning a 302 to the client? No way. If every > time we do a multiviews or a mod_dir request we have to send a 302 to > the client, we will cut our throughput horribly. You are actually > talking about making most of the requests we serve require two requests. > Think about it, the most common request people make to a web server is > http://www.example.com/.; To make that request return a 302 EVERY time > is just wrong. Eh, you obviously missed my point. I was saying conceptually what we'd do is to return a 302. That's the real thing we *want* to do. There should be no argument there. I never suggested returning a 302 every time - I just said that's the ideal. But, instead do an internal_redirect from a hook and allow the original request to be aborted. I'd suggest that you read my entire message and attempt to understand what I'm saying. > Plus, the protocol filters are already working. Please re-read this > patch, it didn't add the protocol filters, it just changed how we insert > filters. In reality, this patch is only broken because of the way we > use our filter list. Nope. I think you've missed how the filters are chained together and that they won't be added correctly under any circumstances. But, I'll wait for you to commit what you meant so I can decipher your true meaning. Regardless, this just gets filters working. We still have problems with headers and subreqs too. -- justin
RE: cvs commit: httpd-2.0/server util_filter.c
> > Hooks are not the solution to every problem. We already have people > > complaining that there are so many hooks that they don't really know > > what is possible with the server, nor do they know what they have to do > > to make their modules work. Why would we add more hooks at this point > > when there are other cleaner solutions? > > I think you may have missed my message yesterday. It's not adding a > hook - it's allowing a hook to be able to abort processing of a > request. That's all. internal_redirect will take care of > everything. fast_redirect is bogus because it skips hooks in the > new request. I believe that is broken behavior. If we were to > rely on internal_redirect, everything falls into place - without > having to add all of this protocol_filters code that unnecessarily > complicate matters. Was that the one about returning a 302 to the client? No way. If every time we do a multiviews or a mod_dir request we have to send a 302 to the client, we will cut our throughput horribly. You are actually talking about making most of the requests we serve require two requests. Think about it, the most common request people make to a web server is http://www.example.com/.; To make that request return a 302 EVERY time is just wrong. Plus, the protocol filters are already working. Please re-read this patch, it didn't add the protocol filters, it just changed how we insert filters. In reality, this patch is only broken because of the way we use our filter list. Ryan
Re: cvs commit: httpd-2.0/server util_filter.c
On Sun, Mar 03, 2002 at 08:47:17AM -0800, Ryan Bloom wrote: > My list insertion code was wrong, which was causing this bug. Also, > this patch is required even without the rest of the > mod_dir/mod_negotiation patch. We have a bug, it allows a connection > filter to be added and never called. Try this: > > Crete a request > Add connection filters > Add request filters > Add connection filters > > The second set won't be added. I am fixing this now, it was a simple > mistake of where the if was done. Thanks for figuring out what is wrong. > Because your suggestion makes it even harder impossible to write > modules. Module authors have already told us which filters should > continue through redirects and sub-requests. They did that through > using a different filter type. Why should they then have to repeat that > information in a function? > > Hooks are not the solution to every problem. We already have people > complaining that there are so many hooks that they don't really know > what is possible with the server, nor do they know what they have to do > to make their modules work. Why would we add more hooks at this point > when there are other cleaner solutions? I think you may have missed my message yesterday. It's not adding a hook - it's allowing a hook to be able to abort processing of a request. That's all. internal_redirect will take care of everything. fast_redirect is bogus because it skips hooks in the new request. I believe that is broken behavior. If we were to rely on internal_redirect, everything falls into place - without having to add all of this protocol_filters code that unnecessarily complicate matters. And, the hook that I was asking about is already there - insert_filters. (I just didn't realize it was there.) I believe there is no justification for adding protocol-level filters - as they merely hide the problem rather than fixing it. If you wish to add it, they should be added with a proto_rec as OtherBill suggested. And, if you add that, I ask for you to open 2.1. -- justin
RE: cvs commit: httpd-2.0/server util_filter.c
My list insertion code was wrong, which was causing this bug. Also, this patch is required even without the rest of the mod_dir/mod_negotiation patch. We have a bug, it allows a connection filter to be added and never called. Try this: Crete a request Add connection filters Add request filters Add connection filters The second set won't be added. I am fixing this now, it was a simple mistake of where the if was done. > I still fail to see why you think protocol filters would solve > this problem. I've explained my reasoning behind why I think this > isn't a good idea. I've also pointed out a suggestion that I > believe would work that is rather simple and would increase our > flexibility. I'd like to hear your reasons why my approach isn't > feasible. Because your suggestion makes it even harder impossible to write modules. Module authors have already told us which filters should continue through redirects and sub-requests. They did that through using a different filter type. Why should they then have to repeat that information in a function? Hooks are not the solution to every problem. We already have people complaining that there are so many hooks that they don't really know what is possible with the server, nor do they know what they have to do to make their modules work. Why would we add more hooks at this point when there are other cleaner solutions? However, I have been tracing this patch. The problem is that we are using a simple list, but we don't actually have a simple list. We have multiple lists converging in one place. The subrequests are breaking the filter list now, because I have converted this to a doubly linked list. I will be working on fixing this ASAP, but it may a few hours to fix. This has VERY little to do with the mod_dir/mod_negotiation bug though. That fix was put committed in the commit before this one. This patch fixes a major problem with our filter lists, and we need to have this fixed. Ryan
Re: cvs commit: httpd-2.0/server util_filter.c
On Sun, Mar 03, 2002 at 06:04:08AM -, [EMAIL PROTECTED] wrote: > rbb 02/03/02 22:04:08 > > Modified:.STATUS >include util_filter.h >server util_filter.c > Log: > This finishes the mod_dir/mod_negotiation bug. This final part of the > solution ensures that we don't lose filters if they are added later than > we expect. The problem could be seen if a connection filter was added > after a request-based filter was added in the past. The problem was that > the request-based filters pointed to the first filter in the connection > record, so the new connection filter was never called. Now, all filters > are put on their correct filter lists, and we are sure to always update > all pointers when adding a filter. Please fix this or back it out (your choice). You just broke mod_dir and mod_negotation far worse than they were before. The header filters aren't getting added correctly, and at times, too many header filters are getting added. I have included two simple traces below (stock httpd-std.conf - no modifications). I still fail to see why you think protocol filters would solve this problem. I've explained my reasoning behind why I think this isn't a good idea. I've also pointed out a suggestion that I believe would work that is rather simple and would increase our flexibility. I'd like to hear your reasons why my approach isn't feasible. Regardless, these commits are broken. -- justin Trying 127.0.0.1... Connected to localhost.localdomain. Escape character is '^]'. GET / HTTP/1.1 Host: foo HTTP/1.1 200 OK Date: Sun, 03 Mar 2002 07:28:10 GMT Server: Apache/2.0.33-dev (Unix) DAV/2 Content-Location: index.html.en Vary: negotiate,accept,accept-language,accept-charset TCN: choice Content-Length: 1456 Content-Type: text/html; charset=ISO-8859-1 http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";> http://www.w3.org/1999/xhtml";> Test Page for Apache Installation If you can see this, it means that the installation of the http://www.apache.org/foundation/preFAQ.html";>Apache web server software on this system was successful. You may now add content to this directory and replace this page. Seeing this instead of the website you expected? This page is here because the site administrator has changed the configuration of this web server. Please contact the person responsible for maintaining this server with questions. The Apache Software Foundation, which wrote the web server software this site administrator is using, has nothing to do with maintaining this site and cannot help resolve configuration issues. The Apache documentation has been included with this distribution. You are free to use the image below on an Apache-powered web server. Thanks for using Apache! HTTP/1.1 200 OK Date: Sun, 03 Mar 2002 07:28:10 GMT Server: Apache/2.0.33-dev (Unix) DAV/2 Content-Location: index.html.en Vary: negotiate,accept,accept-language,accept-charset TCN: choice Content-Location: index.html.en TCN: choice Content-Length: 1456 Content-Type: text/html; charset=ISO-8859-1 HTTP/1.1 200 OK Date: Sun, 03 Mar 2002 07:28:10 GMT Server: Apache/2.0.33-dev (Unix) DAV/2 Content-Location: index.html.en Vary: negotiate,accept,accept-language,accept-charset TCN: choice Content-Location: index.html.en TCN: choice Content-Location: index.html.en TCN: choice Content-Length: 1456 Content-Type: text/html; charset=ISO-8859-1 Trying 127.0.0.1... Connected to localhost.localdomain. Escape character is '^]'. GET /manual/ HTTP/1.1 Host: foo http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";> http://www.w3.org/1999/xhtml";> Apache HTTP Server Version 2.0 Documentation FAQ SiteMap Directives Modules http://www.apache.org/search.html";>Search Apache HTTP Server Version 2.0 http://search.apache.org/";> Release Notes New Features in Version 2.0 Upgrading to Version 2.0 Apache License Reference Manual Compiling and Installing Starting Stop
Re: cvs commit: httpd-2.0/server util_filter.c
From: "Roy T. Fielding" <[EMAIL PROTECTED]> Sent: Thursday, September 20, 2001 5:30 AM > > That is complete BS. We have a long standing tradition of NOT making > > commits just to follow the code style. There is no need for a vote, because > > this has been discussed to death and formatting only commits have been > > vetoed in the past in every thread that they come up in. Review the archives > > for Roy and Dean's opinions of formatting changes. They are completely > > bogus, and just serve to make CVS hard to use. > > I don't know what you are talking about. Any code in our repository that > does not match our style is a bug waiting to happen and will be reformatted > as soon as I get around to it [otherwise known as "never"]. Readability of > the code IS a goal of the httpd project. Under no circumstance will there > ever be a freeze on changes that make the code easier to read. Welcome home :-)
Re: cvs commit: httpd-2.0/server util_filter.c
> That is complete BS. We have a long standing tradition of NOT making > commits just to follow the code style. There is no need for a vote, because > this has been discussed to death and formatting only commits have been > vetoed in the past in every thread that they come up in. Review the archives > for Roy and Dean's opinions of formatting changes. They are completely > bogus, and just serve to make CVS hard to use. I don't know what you are talking about. Any code in our repository that does not match our style is a bug waiting to happen and will be reformatted as soon as I get around to it [otherwise known as "never"]. Readability of the code IS a goal of the httpd project. Under no circumstance will there ever be a freeze on changes that make the code easier to read. There does not exist any longstanding opinion that such reformats are bad, simply the longstanding opinion that Dean believes in the one true tab width and too many people are too lazy about tabs to keep them consistent within a file. This doesn't change the FACTS that tabs don't survive cut and paste and often get mangled in the mail and cause the CVS commitlogs to be misaligned if some lines begin with tabs and others begin with spaces. The only rule we have is to not make reformats in the same commit as changes. The general guideline is that new/modified code within an existing file should match the tab/space usage of the code around that being modified, for the simple reason that it makes the cvs log easier to read and doesn't piss off the person who spends most of their time maintaining that code. The other general rule is that reformats should take place before major releases, rather than after them, because otherwise context diffs from our users get hosed. None of this is ever necessary in *my* code, because I am not lazy about following the style guidelines. Anyone who commits code that doesn't follow the guidelines has no say in how many times it needs to be reformatted in the future, for the same reason that people who aren't willing to write documentation have no vote on its contents. Roy
Re: cvs commit: httpd-2.0/server util_filter.c
On Tue, 4 Sep 2001, Ryan Bloom wrote: > On Tuesday 04 September 2001 09:16, Justin Erenkrantz wrote: > > > Ryan, you may veto this commit. However, if you wish to repeal or > > modify our style guide, I suggest you call a vote. -- justin > > That is complete BS. We have a long standing tradition of NOT making > commits just to follow the code style. There is no need for a vote, because > this has been discussed to death and formatting only commits have been > vetoed in the past in every thread that they come up in. Review the archives > for Roy and Dean's opinions of formatting changes. They are completely > bogus, and just serve to make CVS hard to use. heheheh. this thread again :) aside from the obvious difficulty of dealing with differences across style commits, here's my favourite way of explaining my position on this issue: coding styles are much like language dialects and accents. if you can operate in only one coding-style then you can live happily in your own little valley, but once you wander up and over the hills and visit other valleys you will be confused as all hell. learning other coding styles allows one to share in the intellectual wealth of others. (i thought i'd posted this to the list years ago, but it seems i've only sent it in private email.) -dean
Re: cvs commit: httpd-2.0/server util_filter.c
Jeff Trawick wrote:
>[EMAIL PROTECTED] writes:
>
>>jerenkrantz01/09/03 23:50:52
>>
>> Modified:server util_filter.c
>> Log:
>> 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.
>> Submitted by: Brian Pane <[EMAIL PROTECTED]>
>> Reviewed by:Justin Erenkrantz
>>
>> Revision ChangesPath
>> 1.66 +30 -10httpd-2.0/server/util_filter.c
>>
>> Index: util_filter.c
>> ===
>> RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
>> retrieving revision 1.65
>> retrieving revision 1.66
>> diff -u -r1.65 -r1.66
>> --- util_filter.c 2001/08/30 05:25:31 1.65
>> +++ util_filter.c 2001/09/04 06:50:52 1.66
>> @@ -126,12 +132,26 @@
>>
>> static ap_filter_t *add_any_filter(const char *name, void *ctx,
>> request_rec *r, conn_rec *c,
>> - ap_filter_rec_t *frec,
>> + apr_hash_t *reg_filter_set,
>> ap_filter_t **r_filters,
>> ap_filter_t **c_filters)
>> {
>> -for (; frec != NULL; frec = frec->next) {
>> -if (!strcasecmp(name, frec->name)) {
>> +if (reg_filter_set) {
>> +ap_filter_rec_t *frec;
>> +int len = strlen(name);
>> +int size = len + 1;
>> +char name_lower[size];
>>
>
>not portable, not to mention unbounded stack size; the HP C compiler
>won't compile it (maybe a reason to keep it as-is :) )
>
>"array size must be constant expression"
>
Sorry about that; here's the more portable alternative,
if anybody wants to commit it:
char *name_lower = apr_palloc(r ? r->pool : c->pool, size);
--Brian
Re: cvs commit: httpd-2.0/server util_filter.c
On Tuesday 04 September 2001 09:16, Justin Erenkrantz wrote: > On Tue, Sep 04, 2001 at 06:40:03AM -0700, Ryan Bloom wrote: > > Didn't we decide a LONG time ago not to do this unless it was absolutely > > necessary? Format changes just add cruft to the CVS logs. I have > > noticed a lot of changes to the format in this patch that were more > > opinion than code style. For example, the ap_pass_brigade declaration. > > We had three characters wrapping to the next line, but this patch split > > it into two lines. That was unnecessary. > > Well, I use terminals with 80 column width. Therefore, long lines are > a PITA for me. I will *not* work with source files that do not fit > our standard. I'm an asshole in this respect. > > I feel that code in our repository should meet our code standard: > > http://dev.apache.org/styleguide.html > > If *anyone* wants to submit patches to bring a file into compliance with > our published style guide, I *will* commit it. > > Ryan, you may veto this commit. However, if you wish to repeal or > modify our style guide, I suggest you call a vote. -- justin That is complete BS. We have a long standing tradition of NOT making commits just to follow the code style. There is no need for a vote, because this has been discussed to death and formatting only commits have been vetoed in the past in every thread that they come up in. Review the archives for Roy and Dean's opinions of formatting changes. They are completely bogus, and just serve to make CVS hard to use. I won't veto the commit, because backing it out just serves to muck up CVS even worse. Please do not commit any more of them. The group decided a LONG time ago that they were completely bogus, and you are going against a long-standing decision by the group to NOT make commits like this. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
Re: cvs commit: httpd-2.0/server util_filter.c
On Tue, Sep 04, 2001 at 04:28:45PM -, [EMAIL PROTECTED] wrote: > jerenkrantz01/09/04 09:28:45 > > Modified:server util_filter.c > Log: > Jeff pointed out that the character array must be constant. > Well, it's not, so make it allocated from the correct pool rather than > the heap. Obviously I meant "s/heap/stack/" -- justin
Re: cvs commit: httpd-2.0/server util_filter.c
On Tue, Sep 04, 2001 at 06:40:03AM -0700, Ryan Bloom wrote: > Didn't we decide a LONG time ago not to do this unless it was absolutely > necessary? Format changes just add cruft to the CVS logs. I have noticed > a lot of changes to the format in this patch that were more opinion than code > style. For example, the ap_pass_brigade declaration. We had three characters > wrapping to the next line, but this patch split it into two lines. That was > unnecessary. Well, I use terminals with 80 column width. Therefore, long lines are a PITA for me. I will *not* work with source files that do not fit our standard. I'm an asshole in this respect. I feel that code in our repository should meet our code standard: http://dev.apache.org/styleguide.html If *anyone* wants to submit patches to bring a file into compliance with our published style guide, I *will* commit it. Ryan, you may veto this commit. However, if you wish to repeal or modify our style guide, I suggest you call a vote. -- justin
Re: cvs commit: httpd-2.0/server util_filter.c
[EMAIL PROTECTED] writes:
> jerenkrantz01/09/03 23:50:52
>
> Modified:server util_filter.c
> Log:
> 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.
> Submitted by: Brian Pane <[EMAIL PROTECTED]>
> Reviewed by:Justin Erenkrantz
>
> Revision ChangesPath
> 1.66 +30 -10httpd-2.0/server/util_filter.c
>
> Index: util_filter.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
> retrieving revision 1.65
> retrieving revision 1.66
> diff -u -r1.65 -r1.66
> --- util_filter.c 2001/08/30 05:25:31 1.65
> +++ util_filter.c 2001/09/04 06:50:52 1.66
> @@ -126,12 +132,26 @@
>
>static ap_filter_t *add_any_filter(const char *name, void *ctx,
> request_rec *r, conn_rec *c,
> - ap_filter_rec_t *frec,
> + apr_hash_t *reg_filter_set,
> ap_filter_t **r_filters,
> ap_filter_t **c_filters)
>{
> -for (; frec != NULL; frec = frec->next) {
> -if (!strcasecmp(name, frec->name)) {
> +if (reg_filter_set) {
> +ap_filter_rec_t *frec;
> +int len = strlen(name);
> +int size = len + 1;
> +char name_lower[size];
not portable, not to mention unbounded stack size; the HP C compiler
won't compile it (maybe a reason to keep it as-is :) )
"array size must be constant expression"
--
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server util_filter.c
On Monday 03 September 2001 23:57, [EMAIL PROTECTED] wrote: > jerenkrantz01/09/03 23:57:58 > > Modified:server util_filter.c > Log: > **NO CODE CHANGES** > This is a reformat commit *ONLY* > Please drive on through. > > (One spelling tpyo fixed...) Didn't we decide a LONG time ago not to do this unless it was absolutely necessary? Format changes just add cruft to the CVS logs. I have noticed a lot of changes to the format in this patch that were more opinion than code style. For example, the ap_pass_brigade declaration. We had three characters wrapping to the next line, but this patch split it into two lines. That was unnecessary. Formatting changes should only be committed to CVS if you are about to do a major re-writing of the code, and need the style to be changed for read-ability. And even then, it is a bad idea, because it makes it much harder to actually trace through the history, because we have a useless patch in the middle of the history of the file. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
Re: cvs commit: httpd-2.0/server util_filter.c
Sebastian Bergmann <[EMAIL PROTECTED]> writes: > [EMAIL PROTECTED] wrote: > > rbb 01/08/26 23:00:51 > > > > Modified:.CHANGES > >include util_filter.h > >modules/ssl mod_ssl.c ssl_engine_io.c > >server util_filter.c > > util_filter.c > httpd-2.0\server\util_filter.c(205) : > warning C4098: 'ap_remove_input_filter' : > 'void' function returns a value > httpd-2.0\server\util_filter.c(210) : > warning C4098: 'ap_remove_output_filter' : > 'void' function returns a value fixed (it was sort of cute though; I thought I had seen it all) -- Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site: http://www.geocities.com/SiliconValley/Park/9289/ Born in Roswell... married an alien...
Re: cvs commit: httpd-2.0/server util_filter.c
[EMAIL PROTECTED] wrote: > rbb 01/08/26 23:00:51 > > Modified:.CHANGES >include util_filter.h >modules/ssl mod_ssl.c ssl_engine_io.c >server util_filter.c util_filter.c httpd-2.0\server\util_filter.c(205) : warning C4098: 'ap_remove_input_filter' : 'void' function returns a value httpd-2.0\server\util_filter.c(210) : warning C4098: 'ap_remove_output_filter' : 'void' function returns a value -- Sebastian Bergmann Measure Traffic & Usability http://sebastian-bergmann.de/http://phpOpenTracker.de/
