Re: [MERGE] Connection pinning patch

Alex Rousskov wrote:
> 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.
>
> *B1* Client side cleanup
>
>> + if (pinning.fd >= 0)
>> + comm_close(pinning.fd);
>
> Please move cleanup to swanSong. A job destructor is not a good place to
> cleanup things because in the destructor
>
OK.

> - you should not use virtual functions (which may be needed during
> cleanup and logging)
> - you must not throw (which may happen when you do complicated cleanup)
>
> More work is needed to move old code where it belongs, but the new code
> should go into the right place.
>
>
> *B1* You already know about s/debug/debugs/ changes from Amos :-)

OK
>
>
> *B2* Asymmetric unlock in HttpRequest:
>
>> + if(pinned_connection)
>> + cbdataReferenceDone(pinned_connection);
>
> HttpRequest never locks the pointer, yet it unlocks it during cleanup.
> This worries me. Can you move the locking inside HttpRequest, perhaps by
> adding a pinned_connection getter and setter methods?

Ok I will add the setter and getter methods.

>
>
> *B3* Excessive cleanup:
>
>> + if(pinned_connection)
>> + cbdataReferenceDone(pinned_connection);
>> + pinned_connection = NULL;
>
> The last line is extra because cbdataReferenceDone sets the pointer to
> NULL. Same here:
>
>> + if (conn->pinning.peer) {
>> + cbdataReferenceDone(conn->pinning.peer);
>> + conn->pinning.peer = NULL;
>> + }
>
> And elsewhere (please grep for more):
>
>> + cbdataReferenceDone(pinning.peer);
>> + pinning.peer = NULL;
>
OK

>
> *B1* Docs missing:
>
>> +++ src/HttpRequest.h 2008-06-23 18:02:37 +0000
>> @@ -148,6 +148,8 @@
>>
>> String extacl_log; /* String to be used for access.log purposes */
>>
>> + ConnStateData *pinned_connection;
>
> Please document the pinned_connection member. Requests are used in many
> contexts, on many sides... Which connection do we point to?
>
OK
>
> *B3* inheritVirginReplyProperties is getting out of hand. There is
> already a TODO item to move it to HttpRequest. Now, you have created
> another one for HttpReply. Both should be moved, with a pure virtual
> method added to HttpMsg. However, do not do this for this project if you
> do not feel comfortable.
>
No problem, a virtual method inheritProperties can implemented
>
> *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?
>
I will keep the connection_auth_disabled if there is not any problem
>
> *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]

No problem

>
>
> *B2* no_connection_auth should have bool type, right?
Yes

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

OK

>
>
> *B1* The ConnStateData::getPinnedInfo seems to have serious
> side-effects like closing file descriptors and removing close handlers.
> This is not a simple constant getter! Please rename and mention that the
> call may close the descriptor in the method description.
>
Maybe it is better to split in two methods one method which check pinned
info state and if it is required close the invalid pinning connection
and an other one which simply returns the pinned info....

>
>> /**
>> + * If the current ConnStateData has pinned connection returns the socket descriptor of
>> + * pinned connection and the peer object if exists
>> +*/
>> +int ConnStateData::getPinnedInfo(const HttpRequest * request, struct peer * &peer){
> ...
>> + if (request && pinning.port != request->port){
>> + comm_close(pinning.fd);
>> + return -1;
>> + }
>
> *B1* The "peer" output parameter should be set to NULL when
> ConnStateData::getPinnedInfo fails. You can do it at the start of the
> method.

OK
>
>
> *B1* Scary pointer copying. Can, for example, *peer go away during
> reconfigure and such? Please ask Henrik if you are not sure whether this
> is safe:
>
>> + /*
>> + Maybe the peer should be a cbdataReference of pinning.peer (and the caller
>> + use cbdataReferenceDone).
>> + Now the peer is a single pointer...
>> + */
>> + peer = pinning.peer;
>
I remember I had check it. I add the comment here because I believe it
is safer to cbdataReference inside this function and the caller should
call the cbdataReferenceDone when does not need the code, but because I
did not find any problem I left it as is...
I will check it again.

>
> *B2* Something potentially wrong with descriptor management here:
>
>> + int pinning_fd = pinning.fd;
>> + if (pinning_fd < 0)
>> + return -1;
>> +
>> + if (pinning.auth && request && strcasecmp(pinning.host, request->GetHost()) != 0) {
>> + comm_close(pinning_fd);
>> + return -1;
>> + }
>
> If the if-statement condition is true, the pinning.fd is pointing is a
> closing descriptor. Is that what you want?

Or just the pinning.fd is not set. I think in both cases returns the
correct value....

>
>> + auth = pinning.auth;
>> + if (peer != pinning.peer){
>> + comm_close(pinning_fd);
>> + return -1;
>> + }
>
> Same here. I suggest removing pinning_fd local and updating pinning.fd if needed.
The pinning.fd will set in clientPinnedConnectionClosed comm_close
handler. I think it is better to not touch it in this function....

>
>
> * Please check the above few comments for both
> ConnStateData::getPinnedInfo and ConnStateData::getPinnedConnection as
> they are very similar.

OK

>
>
> *B3* Polish docs
>
>> + I was not able to find documentation about the use of the header
>> + "Connection: Proxy-Support" and how other http proxies use this header.
>> + The only I found is a Henrik's comment:
>> +
>> + "[...] However, this alone is not sufficient. You must also add a
>> + "Connection: Proxy-support" header to mark the extension header as a
>> + hop-by-hop header to protect from other proxies inbetween this proxy
>> + and the client.
>
> I would replace the above with
>
> We send "[Proxy-]Connection: Proxy-Support" header to mark
> Proxy-Support as a hop-by-hop header for intermediaries that do not
> understand the semantics of this header. The RFC should have included
> this recommendation.

OK
>
>
> *B3* Constant method missing const?
>
>> +/**
>> + * returns true if the peer can support connection pinning
>> +*/
>> +bool HttpStateData::peerSupportsConnectionPinning()

I will check...

>
>
> *B3* If you do not have a strong preference, please use
> /// single line comments
> for single line comments
OK
>
>
> *B3* Rename peerGetPinned to peerSelectPinned as it is not a getter
> method. And yes, I know you were following the naming for other peer
> methods. That is why this is a level-3 comment which you may ignore.
>
>> + * peerGetPinned
>> + *
>> + * Selects a pinned connection

OK
>
>
> *B3* spellcheck:
>
> + unsigned int pinned:1; /* Request seont on a pinned connection */
>
OK

Regards,
    Christos

>
> Thank you,
>
> Alex.
> bb:reject
>
>
>
Received on Mon Sep 22 2008 - 22:34:50 MDT