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