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
