On 04/25/2014 02:57 AM, Yann Ylavic wrote:
Hi Jan,
sorry for the late.
No problem :).
On Tue, Apr 22, 2014 at 3:39 PM, Jan Kaluža <[email protected]> wrote:
Hi again,
the patch has been here for some time already. I hesitate to commit it to
trunk without any review, because it changes the core code in mod_proxy and
I'm afraid that there could exist more corner-cases I'm not aware of.
On the other side (and to motivate someone with deeper knowledge of
mod_proxy :) ), it would be great to have UDS support also for
ProxyPassMatch and fix some old bugs related to setting options together
with ProxyPassMatch (like PR 43513 and PR 54973).
So, anyone who would have time to review this patch, please? :)
I slightly modified your patch with the following changes :
1. Use \-escaping instead of %-escaping for wildcard_match so to
consume less space and unescape quickly.
Hm, I'm not sure right now if that's right. The corner-case here is
following configuration:
ScriptAlias /cgi?bin/ "/usr/local/apache2/cgi-bin/"
ProxyPassMatch ^/x/(.*\.x)$ http://127.0.0.1/cgi?bin/$1 timeout=1
I'm not sure how valid this configuration is, but it leads to following
request:
"http://127.0.0.1/cgi%3Fbin/printenv.x";
while the proxy_worker has following escaped string:
"http://127.0.0.1/cgi\\?bin/*";
So the right proxy_worker is not matched in this case (not sure if
"right" is really "right" in this case :) ).
2. Fix the recursion in ap_proxy_wildcard_match(), which was calling
the legacy ap_strcmp_match(), and unescape according to 1.
2'. Shouldn't this function be made iterative (it is not final
recursive currently)?
3. Always try to get the worker with its de_socketfy()ed name, and
only if not found use ap_proxy_define[_wildcard]_worker() with the
configured URL (add_pass() and proxysection() were not consistent
about ap_proxy_get_worker()'s given name/URL).
+1 to this.
4. Don't match both the "normal" name with strcnmp() and the wildcard
name with strcmp_match() in ap_proxy_get_worker(), since the worker is
either wildcard or not (this enforcement is also added to add_pass()
and proxysection() so that it is not possible to declare the same
worker name once with a Match, again without, or vice versa).
4'. Does it makes sense to use the longest match_name or should we go
for the first matching wildcard worker (if any) wins?
Hm, I think it should be the longest match_name, if you mean that
"http://127.0.0.1/cgi-bin/*"; should be matched if there exist two
workers like "http://127.0.0.1/*"; and "http://127.0.0.1/cgi-bin/*";.
But if I'm right, in this case, there will be just one *shared* worker.
Can you write an example of two non-shared workers with matching
wirldcard name?
5. Rename ap_proxy_wildcard_match() as ap_proxy_strcmp_ematch() to
suggest it could go to server/util.ch (if/when the associated
ap_matchexp_escape() and ap_proxy_strcasecmp_ematch() are coded).
6. Move proxy_worker_shared { char
wildcard_name[PROXY_WORKER_MAX_NAME_SIZE]; } to proxy_worker { char
*match_name; } to save shm space and avoid length issues (I don't see
how this could be updated at runtime, for now, balancer-manager can't
declare Match workers).
6. Keep ap_proxy_escape_wildcard_url() static (private) since it is
not used outside proxy_util (now).
+1 to all above.
This is not very tested (yet), I'll come back if I detect issues, but
first I'd like your feedbacks on this modifications...
I've tested the patch and generally it works for me (except the things I
wrote above).
Patch attached.
Regards,
Yann.
Regards,
Jan Kaluza
