Re: [MERGE] Connection pinning patch

On Tue, 2008-09-23 at 10:37 +1200, Amos Jeffries wrote:

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

Yes, I understand that. There are three other issues here (and I hope
Christos got it):

- It is not a good idea to name class members using "no" prefixes.
- "Negative" configuration option names are also a poor default.
- no_connection_auth data member should have bool type.

> > *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?

Sure, here is one example:

AsyncCall::Pointer call = asyncCall(33, 5, "ConnStateData::connStateClosed",
    Dialer(connState, &ConnStateData::connStateClosed));
comm_add_close_handler(newfd, call);

The first statement creates an async call with a given section,
debugging level, name, and dialer. The second statement gives that call
to Comm as a close handler callback.

You can find many more (most written by Christos) if you grep for
comm_add_close_handler and look for those calls that have "::" above
them.

The call can probably be declared as const unless you want to store it
to cancel later (which is guaranteed to work unlike canceling of
old-style callbacks).

HTH,

Alex.
Received on Mon Sep 22 2008 - 23:13:44 MDT