Re: svn commit: r1608762 - in /httpd/httpd/branches/2.4.x: ./ CHANGES modules/proxy/proxy_util.c
On Wed, Jul 9, 2014 at 7:35 AM, Marion Christophe JAILLET
christophe.jail...@wanadoo.fr wrote:
Just a few details :
1) Shouldn't we use 100-continue (lowercase c) instead, to more closely
match http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html, § 8.2.3 ?
This would also be consistent with the use of this string in
protocol.c
2) if of any use, in the fast path, strcmp could be used instead of
strcasecmp
It seems that HTTP (unquoted-)tokens are case insensitive:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20 :
Comparison of expectation values is case-insensitive for unquoted
tokens (including the 100-continue token), and is case-sensitive for
quoted-string expectation-extensions.
So we can probably use lowercase 100-continue to conform the rfc
wording, but the case sentive comparison looks invalid.
3) // fast path, should be /* fast path */
Thanks, will fix it.
4) in protocol.c, around line 1212 there is:
if (((expect = apr_table_get(r-headers_in, Expect)) != NULL)
(expect[0] != '\0')) {
/*
* The Expect header field was added to HTTP/1.1 after RFC 2068
* as a means to signal when a 100 response is desired and,
* unfortunately, to signal a poor man's mandatory extension that
* the server must understand or return 417 Expectation Failed.
*/
if (strcasecmp(expect, 100-continue) == 0) {
r-expecting_100 = 1;
}
this is not consistent with the code below. Should this be changed to
something like:
if (strcasecmp(expect, 100-continue) == 0 ||
ap_find_token(r-pool, expect, 100-Continue)) {
r-expecting_100 = 1;
}
?
+1, that's another issue though.
Re: svn commit: r1608762 - in /httpd/httpd/branches/2.4.x: ./ CHANGES modules/proxy/proxy_util.c
On Wed, Jul 9, 2014 at 12:56 PM, Yann Ylavic ylavic@gmail.com wrote: On Wed, Jul 9, 2014 at 7:35 AM, Marion Christophe JAILLET 3) // fast path, should be /* fast path */ Thanks, will fix it. Done in r1609100 (2.4.x) and r1609101 (trunk).
Re: svn commit: r1608762 - in /httpd/httpd/branches/2.4.x: ./ CHANGES modules/proxy/proxy_util.c
On 9 Jul 2014, at 11:56, Yann Ylavic ylavic@gmail.com wrote: On Wed, Jul 9, 2014 at 7:35 AM, Marion Christophe JAILLET christophe.jail...@wanadoo.fr wrote: Just a few details : 1) Shouldn't we use 100-continue (lowercase c) instead, to more closely match http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html, § 8.2.3 ? This would also be consistent with the use of this string in protocol.c 2) if of any use, in the fast path, strcmp could be used instead of strcasecmp It seems that HTTP (unquoted-)tokens are case insensitive: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20 : Comparison of expectation values is case-insensitive for unquoted tokens (including the 100-continue token), and is case-sensitive for quoted-string expectation-extensions. So we can probably use lowercase 100-continue to conform the rfc wording, but the case sentive comparison looks invalid. strcmp would only be used on the fast path - the slow path could cover the case insensitive case. Regards, Graham --
Re: svn commit: r1608762 - in /httpd/httpd/branches/2.4.x: ./ CHANGES modules/proxy/proxy_util.c
On Wed, Jul 9, 2014 at 2:12 PM, Graham Leggett minf...@sharp.fm wrote: On 9 Jul 2014, at 11:56, Yann Ylavic ylavic@gmail.com wrote: On Wed, Jul 9, 2014 at 7:35 AM, Marion Christophe JAILLET christophe.jail...@wanadoo.fr wrote: Just a few details : 1) Shouldn't we use 100-continue (lowercase c) instead, to more closely match http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html, § 8.2.3 ? This would also be consistent with the use of this string in protocol.c 2) if of any use, in the fast path, strcmp could be used instead of strcasecmp It seems that HTTP (unquoted-)tokens are case insensitive: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20 : Comparison of expectation values is case-insensitive for unquoted tokens (including the 100-continue token), and is case-sensitive for quoted-string expectation-extensions. So we can probably use lowercase 100-continue to conform the rfc wording, but the case sentive comparison looks invalid. strcmp would only be used on the fast path - the slow path could cover the case insensitive case. That would be a (little) faster path, but probably hit less often... Regards, Yann.
Re: [PATCH] Fix settings options with ProxyPassMatch
On 04/29/2014 03:51 PM, Jim Jagielski wrote: On Apr 29, 2014, at 8:41 AM, Jan Kaluža jkal...@redhat.com wrote: Because later we have to match the URL of request with some proxy_worker. If you configure ProxyPassMatch like this: ProxyPassMatch ^/test/(\d+)/foo.jpg http://x/$1/foo.jpg Then the proxy_worker name would be http://x/$1/foo.jpg;. If you receive request with URL http://x/something/foo.jpg;, ap_proxy_get_worker() will have to find out the worker with name http://x/$1/foo.jpg;. The question here is how it would do that? The answer used in the patch is we change the worker name to http://x/*/foo.jpg; and check if the URL (http://x/something/foo.jpg; in our case) matches that worker. If we store the original name with $N, we will have to find out different way how to match the worker (probably emulating wildcard pattern matching) It would be possible to store only the original name (with $N variables), store the flag that the proxy worker is using regex and change ap_proxy_strcmp_ematch() function to treat $N as *, but I don't see any real advantage here. In Yann's suggested patch we don't store match_name where it belongs; so we'd need to put it in shm, which means more memory. Instead, we store as is and add a simple char flag which sez if the stored name is a regex. Much savings. Hi, could you please check the patch I've attached to this email? It changes following parts of Yann's patch: 1. keep only single name of the worker stored in shared memory. 2. when ProxyPassMatch is used, wshared-is_name_matchable = 1. 3. if is_name_matchable == 1, ap_proxy_get_worker() uses ap_proxy_strcmp_ematch() which treats $N as '*'. I've tested this patch a bit and it seems to work for me. Regards, Jan Kaluza And I have no idea why storing with $1 - * somehow makes things easier or implies a different way how to match the worker. Finally, let's think about this deeper... Assume we do have ProxyPassMatch ^/test/(\d+)/foo.jpg http://x/$1/foo.jpg ProxyPassMatch ^/zippy/(\d+)/bar.jpg http://x/$1/omar/propjoe.gif is the intent/desire to have 2 workers or 1? A worker is, in some ways, simply a nickname for the socket related to a host and port. Maybe, in the interests of efficiency and speed, since regexes are slow as it is, a condition could be specified (a limitation, as it were), that when using PPM, only everything up to the 1st potential substitution is considered a unique worker. httpd-trunk-proxy-matchv2.patch Description: application/download
Re: [PATCH] Fix settings options with ProxyPassMatch
On Wed, Jul 9, 2014 at 3:03 PM, Jan Kaluža jkal...@redhat.com wrote: Hi, could you please check the patch I've attached to this email? Looks good to me. It changes following parts of Yann's patch: 1. keep only single name of the worker stored in shared memory. 2. when ProxyPassMatch is used, wshared-is_name_matchable = 1. 3. if is_name_matchable == 1, ap_proxy_get_worker() uses ap_proxy_strcmp_ematch() which treats $N as '*'. Much simpler, no need to store the pattern. Would it be possible to handle some escaping character (eg. \), so that $ can be expressed without being interpolated? (There is still the comment in ap_proxy_strcmp_ematch(), but not the code anymore). AFAICT, $ is a legitimate URL character that need not be %-escaped. Regards, Yann.
Re: [PATCH] Fix settings options with ProxyPassMatch
On Wed, Jul 9, 2014 at 4:14 PM, Yann Ylavic ylavic@gmail.com wrote: On Wed, Jul 9, 2014 at 3:03 PM, Jan Kaluža jkal...@redhat.com wrote: Hi, could you please check the patch I've attached to this email? Looks good to me. It changes following parts of Yann's patch: 1. keep only single name of the worker stored in shared memory. 2. when ProxyPassMatch is used, wshared-is_name_matchable = 1. 3. if is_name_matchable == 1, ap_proxy_get_worker() uses ap_proxy_strcmp_ematch() which treats $N as '*'. Much simpler, no need to store the pattern. Would it be possible to handle some escaping character (eg. \), so that $ can be expressed without being interpolated? (There is still the comment in ap_proxy_strcmp_ematch(), but not the code anymore). AFAICT, $ is a legitimate URL character that need not be %-escaped. I forgot proxysection(), why not handle the ap_proxy_define_match_worker() case there too?
Re: FYI: Looking for a release of 2.4.x soonish
On Tue, Jun 24, 2014 at 8:40 PM, Jim Jagielski j...@jagunet.com wrote: I'm hoping to encourage us to push out the next 2.4 release within the next coupla weeks, maybe after the July 4th US-based holiday. Maybe one more vote for the latest mod_deflate fix (PR 56196) so it has no know issue in 2.4.10?
