Re: [MERGE] Connection pinning patch

> On Sat, 2008-09-20 at 20:48 +0300, Tsantilas Christos wrote:
>
>> This patch fixes the bug 1632
>> (http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
>> It is based on the original squid2.5 connection pinning patch developed
>> by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the
>> related squid 2.6 connection pinning code.
>
> Hi Christos,
>
> Thank you for attacking this problem. It looks like you got the
> basics of the solution done, but we need to polish a few things.
>
<snip>
>
> *B2* Please remove negation from the new member name:
>
>> + int no_connection_auth; /* Don't support connection oriented
>> auth */
>
> Call it conn_auth_prohibited or _disabled if you want to keep the same
> semantics. With a negative name, I find it difficult to quickly
> interpret the code below, and I am sure I am not alone:
>
>> + if (!request->flags.no_connection_auth) { // XXX: no no
>> authentication?
>
>
> *B2* Inconsistent and negative configuration option:
>
>> + no-connection-auth
>> + Prevent forwarding of Microsoft connection
>> oriented
>> + authentication (NTLM, Negotiate and Kerberos)
>
> Can this be changed to connection-auth=[off|on|no|yes] or similar? We
> can support the old style for Squid2 compatibility sake, but it is
> better to avoid negative name as the only option. This is especially
> annoying since we already have the right stuff for cache_peer:
>
>> + connection-auth[=on|off|auto]
>
> *B2* no_connection_auth should have bool type, right?
>

Christos and I have already covered this issue.
no-connection-auth is a bool http_port option for incomings.
connection-auth (with auto) is a tri-state cache_peer option for outgoings.

>
> *B2* Please use methods as comm callbacks handlers:
>
>>
>> +/* This is a handler normally called by comm_close() */
>> +static void
>> +clientPinnedConnectionClosed(int fd, void *data)
> ...
>> + comm_add_close_handler(pinning_fd, clientPinnedConnectionClosed,
>> this);
>
> This will improve debugging and overall code appearance.

For my education, could you specify the best way to do this?

Amos
Received on Mon Sep 22 2008 - 22:37:04 MDT