You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/01/02 22:22:56 UTC
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
On 01/02/2009 09:09 PM, wrowe@apache.org wrote:
> Author: wrowe
> Date: Fri Jan 2 12:08:59 2009
> New Revision: 730835
>
> URL: http://svn.apache.org/viewvc?rev=730835&view=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=730835&r1=730834&r2=730835&view=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
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
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
Posted by Ruediger Pluem <rp...@apache.org>.
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
Posted by Ruediger Pluem <rp...@apache.org>.
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
Re: svn commit: r730835 - /httpd/httpd/trunk/server/core.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
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.