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...
