You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rb...@locus.apache.org on 2000/08/30 00:16:46 UTC
cvs commit: apache-2.0/src/main http_core.c http_protocol.c util_filter.c
rbb 00/08/29 15:16:46
Modified: src/include util_filter.h
src/main http_core.c http_protocol.c util_filter.c
Log:
Modify the way filters are added to the server. Instead of using a FIFO,
we use a modified LIFO. It is modified, because if we add a filter while
in the middle of another filter, the added filter gets put in after the
current filter. This requires that the server is smart about which filters
are added when. This should be handled by the HTTP protocol, but we will
want to keep and eye on things for a little while.
This change is necessary, because currently when we add a filter it goes
after the last filter of the same type. This is broken whenever we want
to add a filter that has a dependancy. Think about the core and chunking
filters. They are of the same type, and core is always added first. When
we go to insert chunking, it gets added, but it is never called.
Revision Changes Path
1.12 +11 -5 apache-2.0/src/include/util_filter.h
Index: util_filter.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/include/util_filter.h,v
retrieving revision 1.11
retrieving revision 1.12
diff -u -r1.11 -r1.12
--- util_filter.h 2000/08/29 20:57:26 1.11
+++ util_filter.h 2000/08/29 22:16:44 1.12
@@ -255,18 +255,24 @@
* from another request, then this filter will be added before those other
* filters.
*
- * To re-iterate that last comment. This function is building a FIFO
+ * To re-iterate that last comment. This function is building a LIFO
* list of filters. Take note of that when adding your filter to the chain.
*/
/**
- * Add a filter to the current request. Filters are added in a FIFO manner.
- * The first filter added will be the first filter called.
+ * Add a filter to the current request. Filters are added in a LIFO manner.
+ * The first filter added will be the last filter called.
* @param name The name of the filter to add
* @param ctx Any filter specific data to associate with the filter
* @param r The request to add this filter for.
- * @deffunc void ap_add_filter(const char *name, void *ctx, request_rec *r)
+ * @param curr The filter to add this filter after. This is incredibly useful
+ * if you are adding a filter while executing another filter. The
+ * new filter should be added immediately after the current filter.
+ * By passing the current filter into ap_add_filter, this is
+ * accomplished easily.
+ * @deffunc void ap_add_filter(const char *name, void *ctx, request_rec *r, ap_filter_t *curr)
*/
-API_EXPORT(void) ap_add_filter(const char *name, void *ctx, request_rec *r);
+API_EXPORT(void) ap_add_filter(const char *name, void *ctx, request_rec *r,
+ ap_filter_t *curr);
/* The next two filters are for abstraction purposes only. They could be
* done away with, but that would require that we break modules if we ever
1.110 +2 -2 apache-2.0/src/main/http_core.c
Index: http_core.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_core.c,v
retrieving revision 1.109
retrieving revision 1.110
diff -u -r1.109 -r1.110
--- http_core.c 2000/08/29 20:57:26 1.109
+++ http_core.c 2000/08/29 22:16:44 1.110
@@ -3089,7 +3089,7 @@
static void core_register_filter(request_rec *r)
{
- ap_add_filter("CORE", NULL, r);
+ ap_add_filter("CORE", NULL, r, NULL);
}
static void register_hooks(void)
@@ -3109,7 +3109,7 @@
* request-processing time.
*/
ap_hook_insert_filter(core_register_filter, NULL, NULL, AP_HOOK_MIDDLE);
- ap_register_filter("CORE", core_filter, AP_FTYPE_CONNECTION + 1);
+ ap_register_filter("CORE", core_filter, AP_FTYPE_CONNECTION);
ap_register_filter("CHUNK", chunk_filter, AP_FTYPE_CONNECTION);
}
1.117 +1 -1 apache-2.0/src/main/http_protocol.c
Index: http_protocol.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_protocol.c,v
retrieving revision 1.116
retrieving revision 1.117
diff -u -r1.116 -r1.117
--- http_protocol.c 2000/08/29 20:57:26 1.116
+++ http_protocol.c 2000/08/29 22:16:45 1.117
@@ -1862,7 +1862,7 @@
if (r->chunked) {
apr_table_mergen(r->headers_out, "Transfer-Encoding", "chunked");
apr_table_unset(r->headers_out, "Content-Length");
- ap_add_filter("CHUNK", NULL, r);
+ ap_add_filter("CHUNK", NULL, r, NULL);
}
if (r->byterange > 1)
1.11 +7 -10 apache-2.0/src/main/util_filter.c
Index: util_filter.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/util_filter.c,v
retrieving revision 1.10
retrieving revision 1.11
diff -u -r1.10 -r1.11
--- util_filter.c 2000/08/29 20:57:27 1.10
+++ util_filter.c 2000/08/29 22:16:45 1.11
@@ -117,7 +117,8 @@
apr_register_cleanup(FILTER_POOL, NULL, filter_cleanup, apr_null_cleanup);
}
-API_EXPORT(void) ap_add_filter(const char *name, void *ctx, request_rec *r)
+API_EXPORT(void) ap_add_filter(const char *name, void *ctx, request_rec *r,
+ ap_filter_t *curr)
{
ap_filter_rec_t *frec = registered_filters;
@@ -130,18 +131,14 @@
f->ftype = frec->ftype;
f->r = r;
- if (INSERT_BEFORE(f, r->filters)) {
- f->next = r->filters;
- r->filters = f;
+ if (curr) {
+ f->next = curr->next;
+ curr->next = f;
}
else {
- ap_filter_t *fscan = r->filters;
- while (!INSERT_BEFORE(f, fscan->next))
- fscan = fscan->next;
- f->next = fscan->next;
- fscan->next = f;
+ f->next = r->filters;
+ r->filters = f;
}
-
break;
}
}
Re: cvs commit: apache-2.0/src/main http_core.c http_protocol.c
util_filter.c
Posted by rb...@covalent.net.
> > Before people start yelling that this patch was vetoed, I know that. I am
> > committing anyway, because I have asked for two weeks for a technical
> > reason for the veto, and I haven't seen one yet. I got sick of waiting,
> > so I decided to just commit. This may be an unpopular decision, but I am
> > hoping it gets things moving again, because we keep stagnating.
>
> 1) I gave what I thought was a valid technical reason, but you chose to not
> agree. That is your perogative, but it is also mine to veto what I believe
> is an improper change.
No, you never gave a technical reason, you said I don't think this is a
good idea, we thought about it for two weeks. That is not a technical
reason. I have asked three or four times for a TECHNICAL reason for the
veto. It has never showed up.
> 2) I disagree that you can simply say that a veto goes away after time. If
> you wanted to push for a reason, then ask again. I've been busy (as
> evidenced by my lack of emails here) and just haven't cared to deal with the
> issue. But asking once, waiting a while, then unilaterally deciding a veto
> is thereby closed is way wrong.
After asking three or four times for a technical reason, and waiting two
weeks for a reply and not getting anything, it is not way wrong. IMO, it
isn't wrong at all.
> 3) stagnating? Feh. Are you trying to say that your SSI filter can't be
> written and used? Are you saying that Jeff's xlate filter can't be written
> and used? Or how about the chunking filter that was added? The code for that
> has had review and fixes applied and is now used by Apache. That is a lot of
> progress. You have an issue with how these are inserted, but that is a small
> part of the overall problem: in no way is filtering "stagnating".
Take another look Greg. The SSI filter is coming when I get a
chance. The xlate filter will have the same problem that the chunking
filter has, because it is order dependant. Other people can't really
start working on filters, because they can't add and delete them. I had
three +1's for this patch, one from Roy, who originally came up with the
design for bucket brigades. I had one -1 from you, with no reason, just a
I don't see the need, we can solve it by adding another filter type. I
then explained why adding another filter type isn't a viable solution in
the long run, and after two weeks of waiting, there had been NO response.
> "technical reason" you say. Fine. I'll state it again:
>
> Your "LIFO change" (this stuff really isn't a LIFO because we only
> add, there is no "out" ordering) is solving the problem you had with the
> CORE and CHUNK filters, and shoves a similar (reverse) problem on the
> non-connection filters. Namely: connection filters appear to be a bit easier
> with "prepend"; content filters appear to be easier with an "append". We
> chose one: append. Your patch doesn't solve the overall problems, and
> introduces a discrepancy between the timing of insertions via
> "insert-filter" and the process used by ap_add_filter (if you order your
> hooks in a particular way, then the filters go in reversed).
The append doesn't work for content filters. This has been described with
SSIs and GZIP at least twice.
> You certainly damn well could continue with the code that was there. It was
> working quite fine. The "+ 1" in there was just a temporary measure until we
> introduced the updated set of groups. As I mentioned, I haven't been pushing
> on the full resolution there because the code was working fine, and I've
> been doing some other stuff.
This code was a hack, as you admit, and it solved one problem it also made
it impossible to write new filters, because every new filter also required
a possible new filter type. This has been explained before, it just
doesn't work.
> The code implemented the grouping of filters that we discussed at length
> back in April to June, and at the pow-wow. Your checkin completely threw
> that stuff out.
Yes, it implemented something that we designed but never tested. IT
DOESN'T WORK! I have explained multiple times why it doesn't work, and
you never replied with a valid way this could work without adding all of
the original filter types back it. As I have explained, adding new filter
types for each filter group is not a viable long term solution.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/main http_core.c http_protocol.c util_filter.c
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Aug 29, 2000 at 03:24:40PM -0700, rbb@covalent.net wrote:
>
> Before people start yelling that this patch was vetoed, I know that. I am
> committing anyway, because I have asked for two weeks for a technical
> reason for the veto, and I haven't seen one yet. I got sick of waiting,
> so I decided to just commit. This may be an unpopular decision, but I am
> hoping it gets things moving again, because we keep stagnating.
1) I gave what I thought was a valid technical reason, but you chose to not
agree. That is your perogative, but it is also mine to veto what I believe
is an improper change.
2) I disagree that you can simply say that a veto goes away after time. If
you wanted to push for a reason, then ask again. I've been busy (as
evidenced by my lack of emails here) and just haven't cared to deal with the
issue. But asking once, waiting a while, then unilaterally deciding a veto
is thereby closed is way wrong.
3) stagnating? Feh. Are you trying to say that your SSI filter can't be
written and used? Are you saying that Jeff's xlate filter can't be written
and used? Or how about the chunking filter that was added? The code for that
has had review and fixes applied and is now used by Apache. That is a lot of
progress. You have an issue with how these are inserted, but that is a small
part of the overall problem: in no way is filtering "stagnating".
"technical reason" you say. Fine. I'll state it again:
Your "LIFO change" (this stuff really isn't a LIFO because we only
add, there is no "out" ordering) is solving the problem you had with the
CORE and CHUNK filters, and shoves a similar (reverse) problem on the
non-connection filters. Namely: connection filters appear to be a bit easier
with "prepend"; content filters appear to be easier with an "append". We
chose one: append. Your patch doesn't solve the overall problems, and
introduces a discrepancy between the timing of insertions via
"insert-filter" and the process used by ap_add_filter (if you order your
hooks in a particular way, then the filters go in reversed).
> If I pissed people off with this commit, sorry. I would ask that this
> commit not be backed-out until there is a valid technical reason for the
> veto, because I would like to move forward, and I couldn't go anywhere
> with the broken code that was there.
You certainly damn well could continue with the code that was there. It was
working quite fine. The "+ 1" in there was just a temporary measure until we
introduced the updated set of groups. As I mentioned, I haven't been pushing
on the full resolution there because the code was working fine, and I've
been doing some other stuff.
The code implemented the grouping of filters that we discussed at length
back in April to June, and at the pow-wow. Your checkin completely threw
that stuff out.
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/main http_core.c http_protocol.c
util_filter.c
Posted by rb...@covalent.net.
Before people start yelling that this patch was vetoed, I know that. I am
committing anyway, because I have asked for two weeks for a technical
reason for the veto, and I haven't seen one yet. I got sick of waiting,
so I decided to just commit. This may be an unpopular decision, but I am
hoping it gets things moving again, because we keep stagnating.
If I pissed people off with this commit, sorry. I would ask that this
commit not be backed-out until there is a valid technical reason for the
veto, because I would like to move forward, and I couldn't go anywhere
with the broken code that was there.
Ryan
On 29 Aug 2000 rbb@locus.apache.org wrote:
> rbb 00/08/29 15:16:46
>
> Modified: src/include util_filter.h
> src/main http_core.c http_protocol.c util_filter.c
> Log:
> Modify the way filters are added to the server. Instead of using a FIFO,
> we use a modified LIFO. It is modified, because if we add a filter while
> in the middle of another filter, the added filter gets put in after the
> current filter. This requires that the server is smart about which filters
> are added when. This should be handled by the HTTP protocol, but we will
> want to keep and eye on things for a little while.
>
> This change is necessary, because currently when we add a filter it goes
> after the last filter of the same type. This is broken whenever we want
> to add a filter that has a dependancy. Think about the core and chunking
> filters. They are of the same type, and core is always added first. When
> we go to insert chunking, it gets added, but it is never called.
Re: cvs commit: apache-2.0/src/main http_core.c http_protocol.c
util_filter.c
Posted by rb...@covalent.net.
Before people start yelling that this patch was vetoed, I know that. I am
committing anyway, because I have asked for two weeks for a technical
reason for the veto, and I haven't seen one yet. I got sick of waiting,
so I decided to just commit. This may be an unpopular decision, but I am
hoping it gets things moving again, because we keep stagnating.
If I pissed people off with this commit, sorry. I would ask that this
commit not be backed-out until there is a valid technical reason for the
veto, because I would like to move forward, and I couldn't go anywhere
with the broken code that was there.
Ryan
On 29 Aug 2000 rbb@locus.apache.org wrote:
> rbb 00/08/29 15:16:46
>
> Modified: src/include util_filter.h
> src/main http_core.c http_protocol.c util_filter.c
> Log:
> Modify the way filters are added to the server. Instead of using a FIFO,
> we use a modified LIFO. It is modified, because if we add a filter while
> in the middle of another filter, the added filter gets put in after the
> current filter. This requires that the server is smart about which filters
> are added when. This should be handled by the HTTP protocol, but we will
> want to keep and eye on things for a little while.
>
> This change is necessary, because currently when we add a filter it goes
> after the last filter of the same type. This is broken whenever we want
> to add a filter that has a dependancy. Think about the core and chunking
> filters. They are of the same type, and core is always added first. When
> we go to insert chunking, it gets added, but it is never called.