I would propose that if the below is NOT the cause, then the old version remain. There is a lot to be said for simplicity and clarity.
Plus, the whole reason for ap_proxy_port_of_scheme() was to avoid the sorts of special numbers the below "hides" in various locations. > On Nov 17, 2015, at 7:41 AM, Yann Ylavic <[email protected]> wrote: > > On Tue, Nov 17, 2015 at 1:34 PM, Jim Jagielski <[email protected]> wrote: >> >>> On Nov 16, 2015, at 12:38 PM, Yann Ylavic <[email protected]> wrote: >>> >>> + case 'h': >>> + if (strncasecmp(scheme + 1, "ttp", 4) == 0) { >> >> Should be 3, shouldn't it? > > Yes, I corrected this already in a previous message. > >> >>> + int s4 = ap_tolower(scheme[4]); >>> + if (!s4) { >>> + return APR_URI_HTTP_DEFAULT_PORT; >>> } >>> + else if (s4 == 's' && !scheme[5]) { >>> + return APR_URI_HTTPS_DEFAULT_PORT; >>> + } >>> } >> >> If we are really trying to optimize the heck out of this, certainly >> we should not ap_tolower s4 until we know it's not '\0'. > > Agreed, I have a final patch already which addresses this, just wanted > to see (quickly) if that was the real issue. > > The patch I'd propose to commit is something like: > > Index: modules/proxy/proxy_util.c > =================================================================== > --- modules/proxy/proxy_util.c (revision 1714617) > +++ modules/proxy/proxy_util.c (working copy) > @@ -3663,37 +3663,61 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade(apr_bucke > return OK; > } > > -/* Fill in unknown schemes from apr_uri_port_of_scheme() */ > > -typedef struct proxy_schemes_t { > - const char *name; > - apr_port_t default_port; > -} proxy_schemes_t ; > - > -static proxy_schemes_t pschemes[] = > +PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme) > { > - {"fcgi", 8000}, > - {"ajp", AJP13_DEF_PORT}, > - {"scgi", SCGI_DEF_PORT}, > - { NULL, 0xFFFF } /* unknown port */ > -}; > + if (!scheme) { > + return 0; > + } > > -PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(const char *scheme) > -{ > - if (scheme) { > - apr_port_t port; > - if ((port = apr_uri_port_of_scheme(scheme)) != 0) { > - return port; > - } else { > - proxy_schemes_t *pscheme; > - for (pscheme = pschemes; pscheme->name != NULL; ++pscheme) { > - if (strcasecmp(scheme, pscheme->name) == 0) { > - return pscheme->default_port; > - } > + /* Fill in unknown schemes from apr_uri_port_of_scheme(), > + * and try a fast path for common ones. > + */ > + switch (*scheme) { > + case 'a': > + case 'A': > + if (strcasecmp(scheme + 1, "jp") == 0) { > + return AJP13_DEF_PORT; > + } > + break; > + > + case 'f': > + case 'F': > + if (strcasecmp(scheme + 1, "cgi") == 0) { > + return AP_FCGI_DEF_PORT; > + } > + if (strcasecmp(scheme + 1, "tp") == 0) { > + return APR_URI_FTP_DEFAULT_PORT; > + } > + break; > + > + case 'h': > + case 'H': > + if (strncasecmp(scheme + 1, "ttp", 3) == 0) { > + if (!scheme[4]) { > + return APR_URI_HTTP_DEFAULT_PORT; > } > + else if (ap_tolower(scheme[4]) == 's' && !scheme[5]) { > + return APR_URI_HTTPS_DEFAULT_PORT; > + } > } > + break; > + > + case 'n': > + case 'N': > + if (strcasecmp(scheme + 1, "ntp") == 0) { > + return APR_URI_NNTP_DEFAULT_PORT; > + } > + break; > + > + case 's': > + case 'S': > + if (strcasecmp(scheme + 1, "cgi") == 0) { > + return SCGI_DEF_PORT; > + } > + break; > } > - return 0; > + return apr_uri_port_of_scheme(scheme); > } > > void proxy_util_register_hooks(apr_pool_t *p) > Index: include/util_fcgi.h > =================================================================== > --- include/util_fcgi.h (revision 1714617) > +++ include/util_fcgi.h (working copy) > @@ -72,6 +72,11 @@ typedef struct { > #define AP_FCGI_VERSION_1 1 > > /** > + * Default port for FastCGI backends. > + */ > +#define AP_FCGI_DEF_PORT 8000 > + > +/** > * Possible values for the type field of ap_fcgi_header > */ > #define AP_FCGI_BEGIN_REQUEST 1 > Index: modules/proxy/mod_proxy_ajp.c > =================================================================== > --- modules/proxy/mod_proxy_ajp.c (revision 1714617) > +++ modules/proxy/mod_proxy_ajp.c (working copy) > @@ -48,7 +48,7 @@ static int proxy_ajp_canon(request_rec *r, char *u > * do syntactic check. > * We break the URL into host, port, path, search > */ > - port = def_port = ap_proxy_port_of_scheme("ajp"); > + port = def_port = AJP13_DEF_PORT; > > err = ap_proxy_canon_netloc(r->pool, &url, NULL, NULL, &host, &port); > if (err) { > Index: modules/proxy/mod_proxy_fcgi.c > =================================================================== > --- modules/proxy/mod_proxy_fcgi.c (revision 1714617) > +++ modules/proxy/mod_proxy_fcgi.c (working copy) > @@ -46,7 +46,7 @@ static int proxy_fcgi_canon(request_rec *r, char * > return DECLINED; > } > > - port = def_port = ap_proxy_port_of_scheme("fcgi"); > + port = def_port = AP_FCGI_DEF_PORT; > > ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, > "canonicalising URL %s", url); > Index: modules/proxy/mod_proxy_http.c > =================================================================== > --- modules/proxy/mod_proxy_http.c (revision 1714617) > +++ modules/proxy/mod_proxy_http.c (working copy) > @@ -46,15 +46,16 @@ static int proxy_http_canon(request_rec *r, char * > if (strncasecmp(url, "http:", 5) == 0) { > url += 5; > scheme = "http"; > + port = def_port = APR_URI_HTTP_DEFAULT_PORT; > } > else if (strncasecmp(url, "https:", 6) == 0) { > url += 6; > scheme = "https"; > + port = def_port = APR_URI_HTTPS_DEFAULT_PORT; > } > else { > return DECLINED; > } > - port = def_port = ap_proxy_port_of_scheme(scheme); > > ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, > "HTTP: canonicalising URL %s", url); > -- > > where we both optimize ap_proxy_port_of_scheme() and avoid unnessary calls...
