> 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
