httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: [PATCH] Fix settings options with ProxyPassMatch
Date Tue, 29 Apr 2014 11:04:24 GMT

On Apr 24, 2014, at 8:57 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> Hi Jan,
> 
> sorry for the late.
> 
> On Tue, Apr 22, 2014 at 3:39 PM, Jan Kalu┼ża <jkaluza@redhat.com> 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...


Mime
View raw message