Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-24 Thread Jim Jagielski 

> On Nov 23, 2015, at 3:00 PM, Marion & Christophe JAILLET 
>  wrote:
> 
> Hi,
> 
> 1 typo below.
> 
> Moreover, this kind of patch is a good candidate for backport as it 
> introduces many small differences between 2.4 and trunk.
> Without a backport, backporting future patches may become a nightmare.
> 

++1. I always anticipated it being backported.

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-24 Thread Yann Ylavic 
On Tue, Nov 24, 2015 at 5:28 AM, William A Rowe Jr  wrote:
>
> I didn't have a chance to review the patch, but all of the instances that 1.
> map to an RFC protocol string comparison and 2. occur in 2.4 code base
> *should* be coalesced into one patch.  I'm +1 for every such change.  The
> rest of the RFC token comparisons can be added individually for later
> merging with their associated bug or enhancement backports.

The goal of the changes I committed (and reverted) so far is to
address the cases where we really expect ASCII comparison (ie. don't
want locale to make us think the compared string matches whereas it is
not, eg. "Connection: clôse").
This mainly concerns header names, header values (known tokens), schemes, ...
Hopefully I reverted the other/abusive changes already.

There remain these changes to mod_proxy and mod_rewrite (revert patch
attached, not applied, yet?), which I think makes sense for
performance reasons, but are not strictly required since the compared
strings come from the configuration file (though mod_proxy params may
be configured via the balancer manager).
I whish I could let them applied because of the (huge/costly)
if-else-if-else-if-... pattern, and we do compare real (ours) tokens
here...
Index: modules/mappers/mod_rewrite.c
===
--- modules/mappers/mod_rewrite.c	(revision 1716159)
+++ modules/mappers/mod_rewrite.c	(working copy)
@@ -3477,13 +3477,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
 switch (*key++) {
 case 'b':
 case 'B':
-if (!*key || !ap_casecmpstr(key, "ackrefescaping")) {
+if (!*key || !strcasecmp(key, "ackrefescaping")) {
 cfg->flags |= RULEFLAG_ESCAPEBACKREF;
 if (val && *val) { 
 cfg->escapes = val;
 }
 }
-else if (!ap_casecmpstr(key, "NP") || !ap_casecmpstr(key, "ackrefernoplus")) { 
+else if (!strcasecmp(key, "NP") || !strcasecmp(key, "ackrefernoplus")) { 
 cfg->flags |= RULEFLAG_ESCAPENOPLUS;
 }
 else {
@@ -3492,11 +3492,11 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'c':
 case 'C':
-if (!*key || !ap_casecmpstr(key, "hain")) {   /* chain */
+if (!*key || !strcasecmp(key, "hain")) {   /* chain */
 cfg->flags |= RULEFLAG_CHAIN;
 }
 else if (((*key == 'O' || *key == 'o') && !key[1])
- || !ap_casecmpstr(key, "ookie")) {   /* cookie */
+ || !strcasecmp(key, "ookie")) {   /* cookie */
 data_item *cp = cfg->cookie;
 
 if (!cp) {
@@ -3519,13 +3519,13 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'd':
 case 'D':
-if (!*key || !ap_casecmpstr(key, "PI") || !ap_casecmpstr(key,"iscardpath")) {
+if (!*key || !strcasecmp(key, "PI") || !strcasecmp(key,"iscardpath")) {
 cfg->flags |= (RULEFLAG_DISCARDPATHINFO);
 }
 break;
 case 'e':
 case 'E':
-if (!*key || !ap_casecmpstr(key, "nv")) { /* env */
+if (!*key || !strcasecmp(key, "nv")) { /* env */
 data_item *cp = cfg->env;
 
 if (!cp) {
@@ -3542,7 +3542,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 cp->next = NULL;
 cp->data = val;
 }
-else if (!ap_casecmpstr(key, "nd")) {/* end */
+else if (!strcasecmp(key, "nd")) {/* end */
 cfg->flags |= RULEFLAG_END;
 }
 else {
@@ -3552,7 +3552,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'f':
 case 'F':
-if (!*key || !ap_casecmpstr(key, "orbidden")) {   /* forbidden */
+if (!*key || !strcasecmp(key, "orbidden")) {   /* forbidden */
 cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
 cfg->forced_responsecode = HTTP_FORBIDDEN;
 }
@@ -3563,7 +3563,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'g':
 case 'G':
-if (!*key || !ap_casecmpstr(key, "one")) {/* gone */
+if (!*key || !strcasecmp(key, "one")) {/* gone */
 cfg->flags |= (RULEFLAG_STATUS | RULEFLAG_NOSUB);
 cfg->forced_responsecode = HTTP_GONE;
 }
@@ -3574,7 +3574,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 
 case 'h':
 case 'H':
-if (!*key || !ap_casecmpstr(key, "andler")) { /* handler */
+if (!*key || !strcasecmp(key, "andler")) { /* handler */
 cfg->forced_handler = val;
 }
 else {
@@ -3583,7 +3583,7 @@ static const char *cmd_rewriterule_setflag(apr_poo
 break;
 case 'l':
 case 'L':
-if (!*key || !ap_casecmpstr(key, "ast")) {/* last */
+if (!*key || !strcasecmp(key,

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-23 Thread Ruediger Pluem 


On 11/23/2015 05:46 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Mon Nov 23 16:46:01 2015
> New Revision: 1715876
> 
> URL: http://svn.apache.org/viewvc?rev=1715876=rev
> Log:
> Use new ap_casecmpstr[n]() functions where appropriate (not exhaustive).
> 
> Modified:
> httpd/httpd/trunk/modules/cache/cache_util.c
> httpd/httpd/trunk/modules/filters/mod_deflate.c
> httpd/httpd/trunk/modules/filters/mod_include.c
> httpd/httpd/trunk/modules/filters/mod_proxy_html.c
> httpd/httpd/trunk/modules/generators/mod_autoindex.c
> httpd/httpd/trunk/modules/generators/mod_cgi.c
> httpd/httpd/trunk/modules/generators/mod_cgid.c
> httpd/httpd/trunk/modules/http/byterange_filter.c
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/modules/http2/h2_from_h1.c
> httpd/httpd/trunk/modules/loggers/mod_log_config.c
> httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> httpd/httpd/trunk/modules/metadata/mod_cern_meta.c
> httpd/httpd/trunk/modules/metadata/mod_headers.c
> httpd/httpd/trunk/modules/proxy/ajp_header.c
> httpd/httpd/trunk/modules/proxy/mod_proxy.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_scgi.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/mod_serf.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
> httpd/httpd/trunk/modules/test/mod_policy.c
> httpd/httpd/trunk/server/mpm_unix.c
> httpd/httpd/trunk/server/protocol.c
> httpd/httpd/trunk/server/util.c
> httpd/httpd/trunk/server/util_expr_eval.c
> httpd/httpd/trunk/server/util_script.c
> 
> Modified: httpd/httpd/trunk/modules/cache/cache_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=1715876=1715875=1715876=diff
> ==
> --- httpd/httpd/trunk/modules/cache/cache_util.c (original)
> +++ httpd/httpd/trunk/modules/cache/cache_util.c Mon Nov 23 16:46:01 2015

> @@ -1066,59 +1045,46 @@ int ap_cache_control(request_rec *r, cac
>  cc->max_stale = 1;
>  cc->max_stale_value = -1;
>  }
> -break;
>  }
> -else if (!strncasecmp(token, "min-fresh", 9)) {
> +else if (!ap_casecmpstrn(token, "min-fresh", 9)) {
>  if (token[9] == '=') {
>  cc->min_fresh = 1;
>  cc->min_fresh_value = apr_atoi64(token + 10);
>  }
> -break;
> -}
> -else if (!strcasecmp(token, "must-revalidate")) {
> -cc->must_revalidate = 1;
>  }
>  break;
>  }
>  case 'o':
>  case 'O': {
> -if (!strcasecmp(token, "only-if-cached")) {
> +if (!ap_casecmpstr(token, "only-if-cached")) {
>  cc->only_if_cached = 1;
>  }
>  break;
>  }
> -case 'p':
> -case 'P': {
> -/* handle most common quickest cases... */
> -if (!strcmp(token, "private")) {
> -cc->private = 1;
> -}
> -/* ...then try slowest cases */
> -else if (!strcasecmp(token, "public")) {
> +case 'p': {

Why removing the uppercase 'P' case here?

Regards

Rüdiger

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-23 Thread Marion & Christophe JAILLET 

Hi,

1 typo below.

Moreover, this kind of patch is a good candidate for backport as it 
introduces many small differences between 2.4 and trunk.

Without a backport, backporting future patches may become a nightmare.

I would find useful to split it into several pieces.
The first one should apply cleanly to 2.4.x to ease backport.
Other parts should be splitted in "as many piece as necessary" for 
potential future backport.


CJ


Le 23/11/2015 17:46, yla...@apache.org a écrit :

Author: ylavic
Date: Mon Nov 23 16:46:01 2015
New Revision: 1715876

URL: http://svn.apache.org/viewvc?rev=1715876=rev
Log:
Use new ap_casecmpstr[n]() functions where appropriate (not exhaustive).

Modified:

Modified: httpd/httpd/trunk/modules/cache/cache_util.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_util.c?rev=1715876=1715875=1715876=diff
==
--- httpd/httpd/trunk/modules/cache/cache_util.c (original)
+++ httpd/httpd/trunk/modules/cache/cache_util.c Mon Nov 23 16:46:01 2015

[...]

@@ -1066,59 +1045,46 @@ int ap_cache_control(request_rec *r, cac
  cc->max_stale = 1;
  cc->max_stale_value = -1;
  }
-break;
  }
-else if (!strncasecmp(token, "min-fresh", 9)) {
+else if (!ap_casecmpstrn(token, "min-fresh", 9)) {
  if (token[9] == '=') {
  cc->min_fresh = 1;
  cc->min_fresh_value = apr_atoi64(token + 10);
  }
-break;
Thanks for that. I was about to commit the same kind of patch (remove 
useless break).



-}
-else if (!strcasecmp(token, "must-revalidate")) {
-cc->must_revalidate = 1;
  }
  break;
  }
  case 'o':
  case 'O': {
-if (!strcasecmp(token, "only-if-cached")) {
+if (!ap_casecmpstr(token, "only-if-cached")) {
  cc->only_if_cached = 1;
  }
  break;
  }
-case 'p':
-case 'P': {
-/* handle most common quickest cases... */
-if (!strcmp(token, "private")) {
-cc->private = 1;
-}
-/* ...then try slowest cases */
-else if (!strcasecmp(token, "public")) {
+case 'p': {

case 'P' removed?

Best regards,
CJ

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-23 Thread Yann Ylavic 
On Mon, Nov 23, 2015 at 8:37 PM, Ruediger Pluem  wrote:
>
> On 11/23/2015 05:46 PM, yla...@apache.org wrote:
>>
>> -case 'p':
>> -case 'P': {
>> -/* handle most common quickest cases... */
>> -if (!strcmp(token, "private")) {
>> -cc->private = 1;
>> -}
>> -/* ...then try slowest cases */
>> -else if (!strcasecmp(token, "public")) {
>> +case 'p': {
>
> Why removing the uppercase 'P' case here?

Not intentional, fixed in r1715938.
Thanks for noticing.

Regards,
Yann.

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-23 Thread Yann Ylavic 
On Mon, Nov 23, 2015 at 9:00 PM, Marion & Christophe JAILLET
 wrote:
> Hi,
>
> 1 typo below.

Fixed in r1715938, thanks.

>
> Moreover, this kind of patch is a good candidate for backport as it
> introduces many small differences between 2.4 and trunk.
> Without a backport, backporting future patches may become a nightmare.

Agreed.

>
> I would find useful to split it into several pieces.
> The first one should apply cleanly to 2.4.x to ease backport.
> Other parts should be splitted in "as many piece as necessary" for potential
> future backport.

I'll try to merge with svn, and resolve conflicts...

Regards,
Yann.

Re: svn commit: r1715876 - in /httpd/httpd/trunk: modules/cache/ modules/filters/ modules/generators/ modules/http/ modules/http2/ modules/loggers/ modules/mappers/ modules/metadata/ modules/proxy/ mo

2015-11-23 Thread William A Rowe Jr 
On Nov 23, 2015 14:12, "Yann Ylavic"  wrote:
>
> On Mon, Nov 23, 2015 at 9:00 PM, Marion & Christophe JAILLET
>  wrote:
> >
> > Moreover, this kind of patch is a good candidate for backport as it
> > introduces many small differences between 2.4 and trunk.
> > Without a backport, backporting future patches may become a nightmare.
>
> Agreed.
>
> >
> > I would find useful to split it into several pieces.
> > The first one should apply cleanly to 2.4.x to ease backport.
> > Other parts should be splitted in "as many piece as necessary" for
potential
> > future backport.
>
> I'll try to merge with svn, and resolve conflicts...

Moreover,

I didn't have a chance to review the patch, but all of the instances that
1. map to an RFC protocol string comparison and 2. occur in 2.4 code base
*should* be coalesced into one patch.  I'm +1 for every such change.  The
rest of the RFC token comparisons can be added individually for later
merging with their associated bug or enhancement backports.

I'm strongly -1 for using this function to 'optimize' what is otherwise a
non-spec string comparison.  There is no excuse for us to defeat good
implementations of strcasecmp for the benefit of platforms with shoddy
implementations.

Jim proposed this as an optimization but I reject that, and wholeheartedly
support his suggested use of a predictable ASCII-only folding comparison
when following any spec that defines token strings in terms of ASCII alpha
chars.

We aren't writing a clib but we do have to be mindful of i18n side-effects
of the clib we are running within.