On Apr 24, 2014, at 8:57 PM, Yann Ylavic <[email protected]> wrote:
Hi Jan,
sorry for the late.
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.
Wouldn't this cause confusion/issue with normal escaping?
2. Fix the recursion in ap_proxy_wildcard_match(), which was calling
the legacy ap_strcmp_match(), and unescape according to 1.
+1
2'. Shouldn't this function be made iterative (it is not final
recursive currently)?
+1
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).
Makes sense.
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?
Longest match name, imo.
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).
Why?
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).
Future effort, so -1 on the moving.
6. Keep ap_proxy_escape_wildcard_url() static (private) since it is
not used outside proxy_util (now).
Sounds good.
This is not very tested (yet), I'll come back if I detect issues, but
first I'd like your feedbacks on this modifications...
Patch attached.
Just a comment: I am really concerned that with such a major change,
we will likely break things in ways we are unaware of... We should
focus on fixing the actual problem w/o also combining it with
other "fixes" that are tangentially involved. For example, maybe
a simply flag when creating the worker that sez "worker name is a regex"
is the way to approach it, and thus ap_proxy_get_worker() could
add additional checks for finding the "right" worker when it
knows it needs to allow for regex substitutions. That creates
a additional logic path which is self-contained and isolated, instead
of monkeying around with changing current paths...
