Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
On 01/02/2009 10:50 PM, Ruediger Pluem wrote:
On 01/02/2009 10:27 PM, William A. Rowe, Jr. wrote:
Ruediger Pluem wrote:
Otherwise the table will not be empty anymore in the case that
neither APR_HAS_SO_ACCEPTFILTER nor APR_TCP_DEFER_ACCEPT is set.
Uhm ... huh? What gave you the idea that APR_TCP_DEFER_ACCEPT
is a volatile? It's present in apr 1.3 (our baseline) and will
be sticking around into the foreseeable future.
It is neither a HAS nor HAVE feature flag. In fact it's the reason
I started refactoring this code, it was complete twaddle.
As I dig deeper into this I think that the whole AcceptFilter config
is busted:
ap_apply_accept_filter allows you set up individual accept filters
for each listening socket. This seems right to me.
OTOH AcceptFilter only allows you to do a global mapping of protocols
to accept filters.
Thus if I want to have two different listeners with the same protocol
but different accept filters I cannot do this configuration wise.
This doesn't seem correct to me.
Furthermore if all this stuff is global the following loop from
listen.c::ap_setup_listeners doesn't really make sense to me:
for (lr = ap_listeners; lr; lr = lr-next) {
num_listeners++;
found = 0;
for (ls = s; ls !found; ls = ls-next) {
for (addr = ls-addrs; addr !found; addr = addr-next) {
if (apr_sockaddr_equal(lr-bind_addr, addr-host_addr)
lr-bind_addr-port == addr-host_port) {
found = 1;
ap_apply_accept_filter(s-process-pool, lr, ls);
}
}
}
if (!found) {
ap_apply_accept_filter(s-process-pool, lr, s);
}
}
Why do we need to iterate over the server recs then (2nd loop)?
The config information about protocol filter mappings should be
only in the main server rec.
Regards
Rüdiger
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
Ruediger Pluem wrote: As I dig deeper into this I think that the whole AcceptFilter config is busted: Ahhh, better explanation, thanks! Note AcceptFilter is ONLY accepted in the global context, so all that logic below was nonsense. ap_apply_accept_filter allows you set up individual accept filters for each listening socket. This seems right to me. OTOH AcceptFilter only allows you to do a global mapping of protocols to accept filters. Thus if I want to have two different listeners with the same protocol but different accept filters I cannot do this configuration wise. This doesn't seem correct to me. Furthermore if all this stuff is global the following loop from listen.c::ap_setup_listeners doesn't really make sense to me: You can, as long as they are named by different protocols. There's really not a logical reason for extra flexibility by protocol. If one host exists only to deal with some broken client, http_borked protocol name for that IP would be fine. Why do we need to iterate over the server recs then (2nd loop)? The config information about protocol filter mappings should be only in the main server rec. Sounds right.
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
On 01/02/2009 09:09 PM, wr...@apache.org wrote:
Author: wrowe
Date: Fri Jan 2 12:08:59 2009
New Revision: 730835
URL: http://svn.apache.org/viewvc?rev=730835view=rev
Log:
Clean up fugly initialization of AcceptFilter mappings
Modified:
httpd/httpd/trunk/server/core.c
Modified: httpd/httpd/trunk/server/core.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=730835r1=730834r2=730835view=diff
==
--- httpd/httpd/trunk/server/core.c (original)
+++ httpd/httpd/trunk/server/core.c Fri Jan 2 12:08:59 2009
@@ -441,13 +441,10 @@
conf-subreq_limit = 0;
conf-protocol = NULL;
-conf-accf_map = apr_table_make(a, 5);
-
-#ifdef APR_TCP_DEFER_ACCEPT
-apr_table_set(conf-accf_map, http, data);
-apr_table_set(conf-accf_map, https, data);
-#endif
+conf-accf_map = is_virtual ? NULL : apr_table_make(a, 5);
+/* A mapping only makes sense in the global context */
+if (conf-accf_map) {
#if APR_HAS_SO_ACCEPTFILTER
#ifndef ACCEPT_FILTER_NAME
#define ACCEPT_FILTER_NAME httpready
@@ -458,9 +455,13 @@
#endif
#endif
#endif
-apr_table_set(conf-accf_map, http, ACCEPT_FILTER_NAME);
-apr_table_set(conf-accf_map, https, dataready);
+apr_table_setn(conf-accf_map, http, ACCEPT_FILTER_NAME);
+apr_table_setn(conf-accf_map, https, dataready);
+#else
+apr_table_setn(conf-accf_map, http, data);
+apr_table_setn(conf-accf_map, https, data);
Don't we need to put the two lines above into
#ifdef APR_TCP_DEFER_ACCEPT
#endif
?
Otherwise the table will not be empty anymore in the case that
neither APR_HAS_SO_ACCEPTFILTER nor APR_TCP_DEFER_ACCEPT is set.
Regards
Rüdiger
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
Ruediger Pluem wrote: Otherwise the table will not be empty anymore in the case that neither APR_HAS_SO_ACCEPTFILTER nor APR_TCP_DEFER_ACCEPT is set. Uhm ... huh? What gave you the idea that APR_TCP_DEFER_ACCEPT is a volatile? It's present in apr 1.3 (our baseline) and will be sticking around into the foreseeable future. It is neither a HAS nor HAVE feature flag. In fact it's the reason I started refactoring this code, it was complete twaddle.
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
On 01/02/2009 10:27 PM, William A. Rowe, Jr. wrote:
Ruediger Pluem wrote:
Otherwise the table will not be empty anymore in the case that
neither APR_HAS_SO_ACCEPTFILTER nor APR_TCP_DEFER_ACCEPT is set.
Uhm ... huh? What gave you the idea that APR_TCP_DEFER_ACCEPT
is a volatile? It's present in apr 1.3 (our baseline) and will
be sticking around into the foreseeable future.
It is neither a HAS nor HAVE feature flag. In fact it's the reason
I started refactoring this code, it was complete twaddle.
Ahh, sorry I missed this point. I guess in this case the #ifdef from the
following code in listen.c should also go away:
#ifdef APR_TCP_DEFER_ACCEPT
rv = apr_socket_opt_set(s, APR_TCP_DEFER_ACCEPT, 30);
if (rv != APR_SUCCESS !APR_STATUS_IS_ENOTIMPL(rv)) {
ap_log_perror(APLOG_MARK, APLOG_WARNING, rv, p,
Failed to enable APR_TCP_DEFER_ACCEPT);
}
#endif
Regards
Rüdiger
