From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 26 Aug 2013 17:46:48 +0000 (-0600)
Subject: Do not close FTP gw server control connection on more server-unrelated errors.
X-Git-Tag: SQUID_3_5_0_1~117^2~54
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=29f23abfda055fadd3871a5cb6d6014bdac81e27;p=thirdparty%2Fsquid.git

Do not close FTP gw server control connection on more server-unrelated errors.

FtpGatewayServer now forgets about its control connection to the server once
it is done communicating with that server. This allows us to preserve that
connection on FTP transaction errors (e.g., forbidden responses or broken
ICAP servers) that are not related to the server communication and can be
ignored as far as the next FTP gatewayed transaction is concerned.

Removed special DONE state because it is now identical to doneWithServer().

Fixed ServerChannel::forget() to prevent subsequent forgotten connection
closure.
---

diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc
index 23f9ecbe38..94e134e88f 100644
--- a/src/FtpGatewayServer.cc
+++ b/src/FtpGatewayServer.cc
@@ -32,10 +32,10 @@ protected:
 
     ConnStateData::FtpState clientState() const;
     void clientState(ConnStateData::FtpState newState);
+    virtual void serverComplete();
     virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
     virtual void handleControlReply();
     virtual void handleRequestBodyProducerAborted();
-    virtual bool doneWithServer() const;
     virtual bool mayReadVirginReplyBody() const;
     virtual void completeForwarding();
     void forwardReply();
@@ -59,7 +59,7 @@ protected:
         SENT_DATA_REQUEST,
         READING_DATA,
         UPLOADING_DATA,
-        DONE
+        END
     };
     typedef void (ServerStateData::*SM_FUNC)();
     static const SM_FUNC SM_FUNCS[];
@@ -89,7 +89,7 @@ const ServerStateData::SM_FUNC ServerStateData::SM_FUNCS[] = {
     &ServerStateData::readDataReply, // SENT_DATA_REQUEST
     &ServerStateData::readTransferDoneReply, // READING_DATA
     &ServerStateData::readReply, // UPLOADING_DATA
-    NULL // DONE
+    NULL // END
 };
 
 ServerStateData::ServerStateData(FwdState *const fwdState):
@@ -100,10 +100,7 @@ ServerStateData::ServerStateData(FwdState *const fwdState):
 
 ServerStateData::~ServerStateData()
 {
-    if (Comm::IsConnOpen(ctrl.conn)) {
-        fwd->unregister(ctrl.conn);
-        ctrl.forget();
-    }
+    closeServer(); // TODO: move to Server.cc?
 }
 
 void
@@ -119,6 +116,20 @@ ServerStateData::start()
         sendCommand();
 }
 
+/// Keep control connection for future requests, after we are done with it.
+/// Similar to COMPLETE_PERSISTENT_MSG handling in http.cc.
+void
+ServerStateData::serverComplete()
+{
+    if (Comm::IsConnOpen(ctrl.conn)) {
+        debugs(9, 5, "preserve FTP server FD " << ctrl.conn->fd);
+        fwd->unregister(ctrl.conn);
+        ctrl.forget();
+        // fwd->request->clientConnectionManager has this connection pinned
+    }
+    Ftp::ServerStateData::serverComplete();
+}
+
 ConnStateData::FtpState
 ServerStateData::clientState() const
 {
@@ -217,7 +228,7 @@ ServerStateData::handleControlReply()
     if (ctrl.message == NULL)
         return; // didn't get complete reply yet
 
-    assert(state < DONE);
+    assert(state < END);
     (this->*SM_FUNCS[state])();
 }
 
@@ -229,12 +240,6 @@ ServerStateData::handleRequestBodyProducerAborted()
     failed(ERR_READ_ERROR);
 }
 
-bool
-ServerStateData::doneWithServer() const
-{
-    return state == DONE || Ftp::ServerStateData::doneWithServer();
-}
-
 bool
 ServerStateData::mayReadVirginReplyBody() const
 {
@@ -253,7 +258,6 @@ ServerStateData::forwardReply()
     setVirginReply(reply);
     adaptOrFinalizeReply();
 
-    state = DONE;
     serverComplete();
 }
 
@@ -290,7 +294,6 @@ ServerStateData::proceedAfterPreliminaryReply()
 void
 ServerStateData::forwardError(err_type error, int xerrno)
 {
-    state = DONE;
     failed(error, xerrno);
 }
 
@@ -504,7 +507,6 @@ ServerStateData::readTransferDoneReply()
                " after reading data");
     }
 
-    state = DONE;
     serverComplete();
 }
 
diff --git a/src/FtpServer.cc b/src/FtpServer.cc
index 4c1ca4cd8a..995d221ea7 100644
--- a/src/FtpServer.cc
+++ b/src/FtpServer.cc
@@ -85,7 +85,7 @@ ServerChannel::forget()
 {
     if (Comm::IsConnOpen(conn))
         comm_remove_close_handler(conn->fd, closer);
-    closer = NULL;
+    clear();
 }
 
 void