Thanks for the patch, Yann! Applied in r1701178. > Am 03.09.2015 um 17:46 schrieb Yann Ylavic <[email protected]>: > > On Thu, Sep 3, 2015 at 2:45 PM, <[email protected]> wrote: >> Author: icing >> Date: Thu Sep 3 12:45:26 2015 >> New Revision: 1701005 >> >> URL: http://svn.apache.org/r1701005 >> Log: >> changed Protocols default to http/1.1 only, updated documentation, changed >> ap_select_protocol() to return NULL when no protocol could be agreed upon >> >> Modified: > [] >> httpd/httpd/trunk/server/protocol.c > [] >> >> Modified: httpd/httpd/trunk/server/protocol.c >> URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1701005&r1=1701004&r2=1701005&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/server/protocol.c (original) >> +++ httpd/httpd/trunk/server/protocol.c Thu Sep 3 12:45:26 2015 >> @@ -1982,53 +1982,77 @@ AP_DECLARE(const char *) ap_select_proto >> apr_array_header_t *choices) >> { >> apr_pool_t *pool = r? r->pool : c->pool; >> - apr_array_header_t *proposals; >> - const char *protocol = NULL, *existing = ap_get_protocol(c); >> core_server_config *conf = ap_get_core_module_config(s->module_config); >> + const char *protocol = NULL, *existing = ap_get_protocol(c);; >> + apr_array_header_t *proposals; > > I think we should not check for NULL "choices" below, it's up to > caller to provide a valid pointer (or we crash where we should). > > Also it should probably be a const apr_array_header_t*. > >> >> if (APLOGcdebug(c)) { >> const char *p = apr_array_pstrcat(pool, conf->protocols, ','); >> ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, >> "select protocol from %s, choices=%s for server %s", >> - p, apr_array_pstrcat(pool, choices, ','), >> + p, choices? >> + apr_array_pstrcat(pool, choices, ',') : "NULL", >> s->server_hostname); >> } >> >> - proposals = apr_array_make(pool, choices->nelts+1, sizeof(char *)); >> + proposals = apr_array_make(pool, choices? choices->nelts+1 : 5, >> + sizeof(char *)); >> ap_run_protocol_propose(c, r, s, choices, proposals); >> >> if (proposals->nelts > 0) { >> int i; >> - apr_array_header_t *prefs = ((conf->protocols_honor_order > 0 >> - && conf->protocols->nelts > 0)? >> - conf->protocols : choices); >> + apr_array_header_t *prefs = NULL; >> + >> + /* Default for protocols_honor_order is 'on' or != 0 */ >> + if (conf->protocols_honor_order == 0 && choices && choices->nelts > >> 0) { >> + prefs = choices; >> + } >> + else { >> + prefs = conf->protocols; >> + } >> >> /* If the existing protocol has not been proposed, but is a choice, >> * add it to the proposals implicitly. >> */ >> - if (!ap_array_str_contains(proposals, existing) >> + if (choices >> + && !ap_array_str_contains(proposals, existing) >> && ap_array_str_contains(choices, existing)) { >> APR_ARRAY_PUSH(proposals, const char*) = existing; >> } > > Shouldn't this "if" be moved before "if (proposals->nelts > 0)" above > so that we enter here (i.e. don't return NULL) when "existing" is both > a choice and a configured Protocols? > >> - >> + >> /* Select the most preferred protocol */ >> if (APLOGcdebug(c)) { >> ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, >> - "select protocol, proposals=%s preferences=%s", >> + "select protocol, proposals=%s preferences=%s >> configured=%s", >> apr_array_pstrcat(pool, proposals, ','), >> - apr_array_pstrcat(pool, prefs, ',')); >> + apr_array_pstrcat(pool, prefs, ','), >> + apr_array_pstrcat(pool, conf->protocols, ',')); >> } >> - for (i = 0; i < proposals->nelts; ++i) { >> - const char *p = APR_ARRAY_IDX(proposals, i, const char *); >> - if (conf->protocols->nelts > 0 >> - && !ap_array_str_contains(conf->protocols, p)) { >> - /* not a permitted protocol here */ >> - continue; >> - } >> - else if (!protocol >> - || (protocol_cmp(prefs, protocol, p) < 0)) { >> - /* none selected yet or this on has preference */ >> - protocol = p; >> + if (conf->protocols->nelts <= 0) { >> + /* nothing configured, by default, we only allow http/1.1 here. >> + * For now... >> + */ >> + return (ap_array_str_contains(proposals, AP_PROTOCOL_HTTP1)? >> + AP_PROTOCOL_HTTP1 : NULL); >> + } > > Couldn't we move this "if" at the beginning of the function? > Whatever the choices and proposals are, it looks immutable. > >> + else { >> + for (i = 0; i < proposals->nelts; ++i) { >> + const char *p = APR_ARRAY_IDX(proposals, i, const char *); >> + if (conf->protocols->nelts <= 0 && >> !strcmp(AP_PROTOCOL_HTTP1, p)) { > > "conf->protocols->nelts <= 0" already tested above (always false here). > >> + /* nothing configured, by default, we only allow >> http/1.1 here. >> + * For now... >> + */ >> + continue; >> + } >> + if (!ap_array_str_contains(conf->protocols, p)) { >> + /* not a configured protocol here */ >> + continue; >> + } >> + else if (!protocol >> + || (protocol_cmp(prefs, protocol, p) < 0)) { >> + /* none selected yet or this one has preference */ >> + protocol = p; >> + } >> } >> } >> } >> @@ -2037,7 +2061,7 @@ AP_DECLARE(const char *) ap_select_proto >> protocol? protocol : "(none)"); >> } >> >> - return protocol? protocol : existing; >> + return protocol; >> } > > > My comments above also attached as a patch (possibly better than words ;) ... > > Regards, > Yann. > <ap_select_protocol.patch>
<green/>bytes GmbH Hafenweg 16, 48155 Münster, Germany Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
