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.
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1701026)
+++ server/protocol.c (working copy)
@@ -1951,7 +1951,7 @@ AP_DECLARE(void) ap_send_interim_response(request_
* Compare two protocol identifier. Result is similar to strcmp():
* 0 gives same precedence, >0 means proto1 is preferred.
*/
-static int protocol_cmp(apr_array_header_t *preferences,
+static int protocol_cmp(const apr_array_header_t *preferences,
const char *proto1,
const char *proto2)
{
@@ -1979,11 +1979,11 @@ AP_DECLARE(const char *) ap_get_protocol(conn_rec
AP_DECLARE(const char *) ap_select_protocol(conn_rec *c, request_rec *r,
server_rec *s,
- apr_array_header_t *choices)
+ const apr_array_header_t *choices)
{
apr_pool_t *pool = r? r->pool : c->pool;
core_server_config *conf = ap_get_core_module_config(s->module_config);
- const char *protocol = NULL, *existing = ap_get_protocol(c);;
+ const char *protocol = NULL, *existing;
apr_array_header_t *proposals;
if (APLOGcdebug(c)) {
@@ -1990,21 +1990,40 @@ AP_DECLARE(const char *) ap_select_protocol(conn_r
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, choices?
- apr_array_pstrcat(pool, choices, ',') : "NULL",
+ p, apr_array_pstrcat(pool, choices, ','),
s->server_hostname);
}
-
- proposals = apr_array_make(pool, choices? choices->nelts+1 : 5,
- sizeof(char *));
+
+ if (conf->protocols->nelts <= 0) {
+ /* nothing configured, by default, we only allow http/1.1 here.
+ * For now...
+ */
+ if (ap_array_str_contains(choices, AP_PROTOCOL_HTTP1)) {
+ return AP_PROTOCOL_HTTP1;
+ }
+ else {
+ return NULL;
+ }
+ }
+
+ proposals = apr_array_make(pool, choices->nelts + 1, sizeof(char *));
ap_run_protocol_propose(c, r, s, choices, proposals);
-
+
+ /* If the existing protocol has not been proposed, but is a choice,
+ * add it to the proposals implicitly.
+ */
+ existing = ap_get_protocol(c);
+ if (!ap_array_str_contains(proposals, existing)
+ && ap_array_str_contains(choices, existing)) {
+ APR_ARRAY_PUSH(proposals, const char*) = existing;
+ }
+
if (proposals->nelts > 0) {
int i;
- apr_array_header_t *prefs = NULL;
-
+ const 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) {
+ if (conf->protocols_honor_order == 0 && choices->nelts > 0) {
prefs = choices;
}
else {
@@ -2011,15 +2030,6 @@ AP_DECLARE(const char *) ap_select_protocol(conn_r
prefs = conf->protocols;
}
- /* If the existing protocol has not been proposed, but is a choice,
- * add it to the proposals implicitly.
- */
- if (choices
- && !ap_array_str_contains(proposals, existing)
- && ap_array_str_contains(choices, existing)) {
- APR_ARRAY_PUSH(proposals, const char*) = existing;
- }
-
/* Select the most preferred protocol */
if (APLOGcdebug(c)) {
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
@@ -2028,32 +2038,17 @@ AP_DECLARE(const char *) ap_select_protocol(conn_r
apr_array_pstrcat(pool, prefs, ','),
apr_array_pstrcat(pool, conf->protocols, ','));
}
- 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);
- }
- 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)) {
- /* 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;
- }
+ for (i = 0; i < proposals->nelts; ++i) {
+ const char *p = APR_ARRAY_IDX(proposals, i, const char *);
+ 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;
+ }
}
}
if (APLOGcdebug(c)) {
Index: include/http_protocol.h
===================================================================
--- include/http_protocol.h (revision 1701026)
+++ include/http_protocol.h (working copy)
@@ -800,7 +800,7 @@ AP_DECLARE_HOOK(const char *,protocol_get,(const c
*/
AP_DECLARE(const char *) ap_select_protocol(conn_rec *c, request_rec *r,
server_rec *s,
- apr_array_header_t *choices);
+ const apr_array_header_t *choices);
/**
* Perform the actual protocol switch. The protocol given must have been
