Re: [PATCH] [MINOR] join findproxy() and findproxy_mode() into more generic findproxy()

Hi Krzysztof,

On Fri, Nov 06, 2009 at 03:26:23PM +0100, Krzysztof Piotr Oledzki wrote:
> >From 573bb3f6dd97e675a48de09f0b07089b70be9fc8 Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole#ans.pl>
> Date: Fri, 6 Nov 2009 15:19:33 +0100
> Subject: [MINOR] join findproxy() and findproxy_mode() into more generic findproxy()
>
> This patch joins two functions into a one. The new one expects a
> bit mask of allowed modes or 0 if any mode is acceptable, so now
> it is possible to remove hardcoded condition allowing switching
> from tcp to http.

Well, I don't find this much satisfying for the same reason I proposed Alex to split the function in two : findproxy_mode() emits an alert when it does not find something, restricting it to some very specific uses. At many places where we need to find a proxy we have used a while() loop to avoid calling it just because of that alert (eg: in the CLI).

I'd prefer that findproxy() just does what its name implies, find a proxy or return NULL when none is found, and then that the caller can handle the error condition gracefully, sometimes by displaying an appropriate error. We can even have a dedicated function to return a hint in case of error.

So most likely we should progressively convert all users of findproxy_mode() to use findproxy() instead then handle the NULL by themselves.

After all, I think that the original findproxy() function dates back when it was permitted to have two proxies with the same name, reason why we pass the required mode to it. Now we can just lookup the proxy name and check the mode afterwards.

What's your opinion ?

Regards,
Willy Received on 2009/11/09 06:52