From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 5 Aug 2014 22:44:23 +0000 (-0600)
Subject: Polished for the official review, addressing several TODOs.
X-Git-Tag: SQUID_3_5_0_1~117^2~8
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e7ce227f301a8f1ab73487d19da899fde7c18858;p=thirdparty%2Fsquid.git

Polished for the official review, addressing several TODOs.

Use 1.1 version for FTP ports because FTP commands are sent on a
"persistent by default" connection, just like HTTP/1.1.

Cleaned up Ftp::CtrlChannel, Ftp::DataChannel, and Ftp::Client object
construction and destruction.

Do not insist on USER command when intercepting FTP. Interception support
may still not work for other reasons, but USER does not seem to be required
since Squid gets request destination from the intercepted connection info.
---

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 694dc7d371..0d7b2b7c8d 100644
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -740,8 +740,8 @@ HttpHeader::packInto(Packer * p, bool mask_sensitive_info) const
             continue;
         }
         switch (e->id) {
-        // TODO: When FTP gw command wrappers may end up in error pages and
-        // other sensitive places, hide HDR_FTP_ARGUMENTS for FTP PASS command.
+        // TODO: When native FTP commands may end up in error pages and other
+        // sensitive places, hide HDR_FTP_ARGUMENTS for the FTP PASS command.
         case HDR_AUTHORIZATION:
         case HDR_PROXY_AUTHORIZATION:
             packerAppend(p, e->name.rawBuf(), e->name.size());
diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index f0249595b2..8f4a0e3ed3 100644
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -45,6 +45,7 @@ class HttpHdrRange;
 class HttpHdrSc;
 class Packer;
 class StoreEntry;
+class SBuf;
 
 /* constant attributes of http header fields */
 
@@ -152,7 +153,7 @@ typedef enum {
     HDR_FRONT_END_HTTPS,                /**< MS Exchange custom header we may have to add */
     HDR_FTP_COMMAND,                    /**< Internal header for FTP command */
     HDR_FTP_ARGUMENTS,                  /**< Internal header for FTP command arguments */
-    HDR_FTP_PRE,                        /**< Custom: Contains leading FTP control response lines */
+    HDR_FTP_PRE,                        /**< Internal header containing leading FTP control response lines */
     HDR_FTP_STATUS,                     /**< Internal header for FTP reply status */
     HDR_FTP_REASON,                     /**< Internal header for FTP reply reason */
     HDR_OTHER,                          /**< internal tag value for "unknown" headers */
@@ -306,8 +307,8 @@ private:
 
 int httpHeaderParseQuotedString(const char *start, const int len, String *val);
 
-/// quotes string using RFC 2616 quoted-string rules
-String httpHeaderQuoteString(const char *raw);
+/// quotes string using RFC 7230 quoted-string rules
+SBuf httpHeaderQuoteString(const char *raw);
 
 int httpHeaderHasByNameListMember(const HttpHeader * hdr, const char *name, const char *member, const char separator);
 void httpHeaderUpdate(HttpHeader * old, const HttpHeader * fresh, const HttpHeaderMask * denied_mask);
diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc
index 4fdcb0dcb2..e1ac27ad6d 100644
--- a/src/HttpHeaderTools.cc
+++ b/src/HttpHeaderTools.cc
@@ -297,20 +297,18 @@ httpHeaderParseQuotedString(const char *start, const int len, String *val)
     return 1;
 }
 
-// TODO: Optimize using SBuf
-String
+SBuf
 httpHeaderQuoteString(const char *raw)
 {
     assert(raw);
 
-    // HTTPbis says Senders SHOULD NOT escape octets in quoted-strings that
-    // do not require escaping (i.e., except DQUOTE and the backslash octet).
+    // RFC 7230 says a "sender SHOULD NOT generate a quoted-pair in a
+    // quoted-string except where necessary" (i.e., DQUOTE and backslash)
     bool needInnerQuote = false;
     for (const char *s = raw; !needInnerQuote &&  *s; ++s)
         needInnerQuote = *s == '"' || *s == '\\';
 
-    static String quotedStr;
-    quotedStr.clean();
+    SBuf quotedStr;
     quotedStr.append('"');
 
     if (needInnerQuote) {
@@ -322,7 +320,7 @@ httpHeaderQuoteString(const char *raw)
     } else {
         quotedStr.append(raw);
     }
-    
+
     quotedStr.append('"');
     return quotedStr;
 }
diff --git a/src/SBuf.cc b/src/SBuf.cc
index dae2aa4431..3fd780b10c 100644
--- a/src/SBuf.cc
+++ b/src/SBuf.cc
@@ -237,6 +237,12 @@ SBuf::append(const char * S, size_type Ssize)
     return lowAppend(S, Ssize);
 }
 
+SBuf &
+SBuf::append(const char c)
+{
+    return lowAppend(&c, 1);
+}
+
 SBuf&
 SBuf::Printf(const char *fmt, ...)
 {
diff --git a/src/SBuf.h b/src/SBuf.h
index be1287a9fa..dec61489be 100644
--- a/src/SBuf.h
+++ b/src/SBuf.h
@@ -182,6 +182,9 @@ public:
      */
     SBuf& append(const SBuf & S);
 
+    /// Append a single character. The character may be NUL (\0).
+    SBuf& append(const char c);
+
     /** Append operation for C-style strings.
      *
      * Append the supplied c-string to the SBuf; extend storage
diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc
index da137358a4..76a5fec9fe 100644
--- a/src/anyp/PortCfg.cc
+++ b/src/anyp/PortCfg.cc
@@ -30,11 +30,12 @@ AnyP::PortCfg::PortCfg() :
         actAsOrigin(false),
         ignore_cc(false),
         connection_auth_disabled(false),
+        ftp_track_dirs(false),
         vport(0),
         disable_pmtu_discovery(0),
-        listenConn(),
+        listenConn()
 #if USE_OPENSSL
-        cert(NULL),
+        ,cert(NULL),
         key(NULL),
         version(0),
         cipher(NULL),
@@ -59,9 +60,8 @@ AnyP::PortCfg::PortCfg() :
         dhParams(),
         contextMethod(),
         sslContextFlags(0),
-        sslOptions(0),
+        sslOptions(0)
 #endif
-        ftp_track_dirs(false)
 {
     memset(&tcp_keepalive, 0, sizeof(tcp_keepalive));
 }
@@ -105,9 +105,9 @@ AnyP::PortCfg::clone() const
     b->vhost = vhost;
     b->vport = vport;
     b->connection_auth_disabled = connection_auth_disabled;
+    b->ftp_track_dirs = ftp_track_dirs;
     b->disable_pmtu_discovery = disable_pmtu_discovery;
     b->tcp_keepalive = tcp_keepalive;
-    b->ftp_track_dirs = ftp_track_dirs;
 
 #if 0
     // TODO: AYJ: 2009-07-18: for now SSL does not clone. Configure separate ports with IPs and SSL settings
@@ -200,7 +200,7 @@ AnyP::PortCfg::setTransport(const char *aProtocol)
         transport = AnyP::ProtocolVersion(AnyP::PROTO_HTTPS, 1,1);
 
     else if (strcasecmp("ftp", aProtocol) == 0)
-        transport = AnyP::ProtocolVersion(AnyP::PROTO_FTP, 1,0);
+        transport = AnyP::ProtocolVersion(AnyP::PROTO_FTP, 1,1);
 
     else
         fatalf("http(s)_port protocol=%s is not supported\n", aProtocol);
diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h
index 9e4c160ac6..bbabd18ad5 100644
--- a/src/anyp/PortCfg.h
+++ b/src/anyp/PortCfg.h
@@ -27,7 +27,7 @@ public:
     /**
      * Set this ports transport type from a string representation.
      * Unknown transport type representations will halt Squid.
-     * Supports: HTTP, HTTP/1.1, HTTPS, HTTPS/1.1.
+     * Supports: HTTP, HTTP/1.1, HTTPS, HTTPS/1.1, and FTP.
      */
     void setTransport(const char *aProtocol);
 
@@ -47,6 +47,8 @@ public:
 
     bool connection_auth_disabled; ///< Don't support connection oriented auth
 
+    bool ftp_track_dirs; ///< whether transactions should track FTP directories
+
     int vport;               ///< virtual port support. -1 if dynamic, >0 static
     int disable_pmtu_discovery;
 
@@ -94,8 +96,6 @@ public:
     long sslContextFlags; ///< flags modifying the use of SSL
     long sslOptions; ///< SSL engine options
 #endif
-
-    bool ftp_track_dirs; ///< whether ftp_port should track FTP directories
 };
 
 } // namespace AnyP
diff --git a/src/cf.data.pre b/src/cf.data.pre
index 1efdd5b0eb..6326e0e7be 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1907,7 +1907,7 @@ DOC_START
 
 	Options:
 
-	    ftp-track-dirs=on|off 
+	    ftp-track-dirs=on|off
 			Enables tracking of FTP directories by injecting extra
 			PWD commands and adjusting Request-URI (in wrapping
 			HTTP requests) to reflect the current FTP server
diff --git a/src/client_side.cc b/src/client_side.cc
index 7d044d1c60..bd82eccfc2 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -93,7 +93,6 @@
 #include "clientStream.h"
 #include "comm.h"
 #include "comm/Connection.h"
-#include "comm/ConnOpener.h"
 #include "comm/Loops.h"
 #include "comm/Read.h"
 #include "comm/TcpAcceptor.h"
@@ -106,7 +105,6 @@
 #include "FwdState.h"
 #include "globals.h"
 #include "http.h"
-#include "HttpHdrCc.h"
 #include "HttpHdrContRange.h"
 #include "HttpHeaderTools.h"
 #include "HttpReply.h"
@@ -114,7 +112,6 @@
 #include "ident/Config.h"
 #include "ident/Ident.h"
 #include "internal.h"
-#include "ip/tools.h"
 #include "ipc/FdNotes.h"
 #include "ipc/StartListening.h"
 #include "log/access_log.h"
@@ -156,7 +153,6 @@
 #include <climits>
 #include <cmath>
 #include <limits>
-#include <set>
 
 #if LINGERING_CLOSE
 #define comm_close comm_lingering_close
@@ -1462,7 +1458,6 @@ void
 clientSocketRecipient(clientStreamNode * node, ClientHttpRequest * http,
                       HttpReply * rep, StoreIOBuffer receivedData)
 {
-    debugs(33,7, HERE << "rep->content_length=" << (rep ? rep->content_length : -2) << " receivedData.length=" << receivedData.length);
     /* Test preconditions */
     assert(node != NULL);
     PROF_start(clientSocketRecipient);
@@ -1532,8 +1527,7 @@ ConnStateData::readNextRequest()
     typedef CommCbMemFunT<ConnStateData, CommTimeoutCbParams> TimeoutDialer;
     AsyncCall::Pointer timeoutCall = JobCallback(33, 5,
                                      TimeoutDialer, this, ConnStateData::requestTimeout);
-    const int timeout = idleTimeout();
-    commSetConnTimeout(clientConnection, timeout, timeoutCall);
+    commSetConnTimeout(clientConnection, idleTimeout(), timeoutCall);
 
     readSomeData();
     /** Please don't do anything with the FD past here! */
@@ -2638,8 +2632,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
     /* compile headers */
     /* we should skip request line! */
     /* XXX should actually know the damned buffer size here */
-    if (http_ver.major >= 1 &&
-        !request->parseHeader(HttpParserHdrBuf(hp), HttpParserHdrSz(hp))) {
+    if (http_ver.major >= 1 && !request->parseHeader(HttpParserHdrBuf(hp), HttpParserHdrSz(hp))) {
         clientStreamNode *node = context->getClientReplyContext();
         debugs(33, 5, "Failed to parse request headers:\n" << HttpParserHdrBuf(hp));
         conn->quitAfterError(request.getRaw());
@@ -2804,7 +2797,7 @@ doFtpAndHttp:
         request->body_pipe = conn->expectRequestBody(
                                  chunked ? -1 : request->content_length);
 
-        if (!notedUseOfBuffer) {
+        if (!isFtp) {
             // consume header early so that body pipe gets just the body
             connNoteUseOfBuffer(conn, http->req_sz);
             notedUseOfBuffer = true;
@@ -2832,7 +2825,7 @@ doFtpAndHttp:
                 goto finish;
 
             if (!request->body_pipe->productionEnded()) {
-                debugs(33, 5, HERE << "need more request body");
+                debugs(33, 5, "need more request body");
                 context->mayUseConnection(true);
                 assert(conn->flags.readMore);
             }
@@ -3236,7 +3229,7 @@ clientLifetimeTimeout(const CommTimeoutCbParams &io)
         io.conn->close();
 }
 
-ConnStateData::ConnStateData(const MasterXaction::Pointer &xact):
+ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) :
         AsyncJob("ConnStateData"), // kids overwrite
 #if USE_OPENSSL
         sslBumpMode(Ssl::bumpEnd),
@@ -3353,7 +3346,7 @@ ConnStateData::start()
     // kids must extend to actually start doing something (e.g., reading)
 }
 
-/** Handle a new connection on HTTP socket. */
+/** Handle a new connection on an HTTP socket. */
 void
 httpAccept(const CommAcceptCbParams &params)
 {
@@ -3364,22 +3357,21 @@ httpAccept(const CommAcceptCbParams &params)
 
     if (params.flag != Comm::OK) {
         // Its possible the call was still queued when the client disconnected
-        debugs(33, 2, "httpAccept: " << s->listenConn << ": accept failure: " << xstrerr(params.xerrno));
+        debugs(33, 2, s->listenConn << ": accept failure: " << xstrerr(params.xerrno));
         return;
     }
 
-    debugs(33, 4, HERE << params.conn << ": accepted");
+    debugs(33, 4, params.conn << ": accepted");
     fd_note(params.conn->fd, "client http connect");
 
-    if (s->tcp_keepalive.enabled) {
+    if (s->tcp_keepalive.enabled)
         commSetTcpKeepalive(params.conn->fd, s->tcp_keepalive.idle, s->tcp_keepalive.interval, s->tcp_keepalive.timeout);
-    }
 
-    ++ incoming_sockets_accepted;
+    ++incoming_sockets_accepted;
 
     // Socket is ready, setup the connection manager to start using it
     ConnStateData *connState = Http::NewServer(xact);
-    AsyncJob::Start(connState);
+    AsyncJob::Start(connState); // usually async-calls readSomeData()
 }
 
 #if USE_OPENSSL
@@ -3643,7 +3635,7 @@ httpsAccept(const CommAcceptCbParams &params)
 
     // Socket is ready, setup the connection manager to start using it
     ConnStateData *connState = Https::NewServer(xact);
-    AsyncJob::Start(connState); // will eventually call postHttpsAccept()
+    AsyncJob::Start(connState); // usually async-calls postHttpsAccept()
 }
 
 void
@@ -3654,7 +3646,7 @@ ConnStateData::postHttpsAccept()
     const AnyP::PortCfgPointer s = port;
 
     if (s->flags.tunnelSslBumping) {
-        debugs(33, 5, "httpsAccept: accept transparent connection: " << clientConnection);
+        debugs(33, 5, "accept transparent connection: " << clientConnection);
 
         if (!Config.accessList.ssl_bump) {
             httpsSslBumpAccessCheckDone(ACCESS_DENIED, connState);
@@ -4123,15 +4115,18 @@ clientStartListeningOn(AnyP::PortCfgPointer &port, const RefCount< CommCbFunPtrC
     // Fill out a Comm::Connection which IPC will open as a listener for us
     port->listenConn = new Comm::Connection;
     port->listenConn->local = port->s;
-    port->listenConn->flags = COMM_NONBLOCKING | (port->flags.tproxyIntercept ? COMM_TRANSPARENT : 0) |
-                              (port->flags.natIntercept ? COMM_INTERCEPTION : 0);
+    port->listenConn->flags =
+        COMM_NONBLOCKING |
+        (port->flags.tproxyIntercept ? COMM_TRANSPARENT : 0) |
+        (port->flags.natIntercept ? COMM_INTERCEPTION : 0);
 
     // route new connections to subCall
     typedef CommCbFunPtrCallT<CommAcceptCbPtrFun> AcceptCall;
     Subscription::Pointer sub = new CallSubscription<AcceptCall>(subCall);
-    AsyncCall::Pointer listenCall = asyncCall(33, 2, "clientListenerConnectionOpened",
-                                    ListeningStartedDialer(&clientListenerConnectionOpened,
-                                                           port, fdNote, sub));
+    AsyncCall::Pointer listenCall =
+        asyncCall(33, 2, "clientListenerConnectionOpened",
+                  ListeningStartedDialer(&clientListenerConnectionOpened,
+                                         port, fdNote, sub));
     Ipc::StartListening(SOCK_STREAM, IPPROTO_TCP, port->listenConn, fdNote, listenCall);
 
     assert(NHttpSockets < MAXTCPLISTENPORTS);
@@ -4174,11 +4169,11 @@ clientOpenListenSockets(void)
     Ftp::StartListening();
 
     if (NHttpSockets < 1)
-        fatal("No HTTP, HTTPS or FTP ports configured");
+        fatal("No HTTP, HTTPS, or FTP ports configured");
 }
 
 void
-clientConnectionsClose(void)
+clientConnectionsClose()
 {
     for (AnyP::PortCfgPointer s = HttpPortList; s != NULL; s = s->next) {
         if (s->listenConn != NULL) {
@@ -4447,7 +4442,7 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
 void
 ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth, bool monitor)
 {
-    if (!Comm::IsConnOpen(pinning.serverConnection) || 
+    if (!Comm::IsConnOpen(pinning.serverConnection) ||
         pinning.serverConnection->fd != pinServer->fd)
         pinNewConnection(pinServer, request, aPeer, auth);
 
@@ -4497,8 +4492,8 @@ ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRe
     comm_add_close_handler(pinning.serverConnection->fd, pinning.closeHandler);
 }
 
-/// [re]start monitoring pinned connection for server closures so that we can
-/// propagate them to an _idle_ client pinned to the server
+/// [re]start monitoring pinned connection for peer closures so that we can
+/// propagate them to an _idle_ client pinned to that peer
 void
 ConnStateData::startPinnedConnectionMonitoring()
 {
diff --git a/src/client_side.h b/src/client_side.h
index cadb4d0b0e..21a2a1b776 100644
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -271,7 +271,7 @@ public:
         int port;               /* port of pinned connection */
         bool pinned;             /* this connection was pinned */
         bool auth;               /* pinned for www authentication */
-        bool reading;   ///< we are monitoring for server connection closure
+        bool reading;   ///< we are monitoring for peer connection closure
         bool zeroReply; ///< server closed w/o response (ERR_ZERO_SIZE_OBJECT)
         CachePeer *peer;             /* CachePeer the connection goes via */
         AsyncCall::Pointer readHandler; ///< detects serverConnection closure
@@ -304,12 +304,12 @@ public:
     bool handleReadData();
     bool handleRequestBodyData();
 
-    /// forward future client requests using the given server connection
-    /// optionally, monitor pinned server connection for server-side closures
+    /// Forward future client requests using the given server connection.
+    /// Optionally, monitor pinned server connection for server-side closures.
     void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor = true);
-    /// undo pinConnection() and, optionally, close the pinned connection
+    /// Undo pinConnection() and, optionally, close the pinned connection.
     void unpinConnection(const bool andClose);
-    /// returns validated pinnned server connection (and stops its monitoring)
+    /// Returns validated pinnned server connection (and stops its monitoring).
     Comm::ConnectionPointer borrowPinnedConnection(HttpRequest *request, const CachePeer *aPeer);
     /**
      * Checks if there is pinning info if it is valid. It can close the server side connection
@@ -417,8 +417,10 @@ protected:
     void startPinnedConnectionMonitoring();
     void clientPinnedConnectionRead(const CommIoCbParams &io);
 
-    // TODO: document
+    /// parse input buffer prefix into a single transfer protocol request
     virtual ClientSocketContext *parseOneRequest(Http::ProtocolVersion &ver) = 0;
+
+    /// start processing a freshly parsed request
     virtual void processParsedRequest(ClientSocketContext *context, const Http::ProtocolVersion &ver) = 0;
 
     /// returning N allows a pipeline of 1+N requests (see pipeline_prefetch)
@@ -430,7 +432,6 @@ protected:
 protected:
     int connFinishedWithConn(int size);
     void clientAfterReadingRequests();
-
     bool concurrentRequestQueueFilled() const;
 
     void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth);
@@ -469,11 +470,14 @@ const char *findTrailingHTTPVersion(const char *uriAndHTTPVersion, const char *e
 
 int varyEvaluateMatch(StoreEntry * entry, HttpRequest * req);
 
+/// accept requests to a given port and inform subCall about them
 void clientStartListeningOn(AnyP::PortCfgPointer &port, const RefCount< CommCbFunPtrCallT<CommAcceptCbPtrFun> > &subCall, const Ipc::FdNoteId noteId);
 
 void clientOpenListenSockets(void);
 void clientConnectionsClose(void);
 void httpRequestFree(void *);
+
+/// decide whether to expect multiple requests on the corresponding connection
 void clientSetKeepaliveFlag(ClientHttpRequest *http);
 
 /* misplaced declaratrions of Stream callbacks provided/used by client side */
diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index 166c0f2d62..e42759665b 100644
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -59,6 +59,8 @@ escapeIAC(const char *buf)
     return ret;
 }
 
+/* Ftp::Channel */
+
 /// configures the channel with a descriptor and registers a close handler
 void
 Ftp::Channel::opened(const Comm::ConnectionPointer &newConn,
@@ -104,24 +106,46 @@ Ftp::Channel::clear()
     closer = NULL;
 }
 
-Ftp::Client::Client(FwdState *fwdState):
-    AsyncJob("Ftp::Client"), ::ServerStateData(fwdState)
+/* Ftp::CtrlChannel */
+
+Ftp::CtrlChannel::CtrlChannel():
+        buf(NULL),
+        size(0),
+        offset(0),
+        message(NULL),
+        last_command(NULL),
+        last_reply(NULL),
+        replycode(0)
 {
-    ++statCounter.server.all.requests;
-    ++statCounter.server.ftp.requests;
+    buf = static_cast<char*>(memAllocBuf(4096, &size));
+}
 
-    ctrl.last_command = xstrdup("Connect to server");
-    ctrl.buf = static_cast<char *>(memAllocBuf(4096, &ctrl.size));
-    ctrl.offset = 0;
+Ftp::CtrlChannel::~CtrlChannel()
+{
+    memFreeBuf(size, buf);
+    if (message)
+        wordlistDestroy(&message);
+    safe_free(last_command);
+    safe_free(last_reply);
+}
 
-    typedef CommCbMemFunT<Client, CommCloseCbParams> Dialer;
-    const AsyncCall::Pointer closer = JobCallback(9, 5, Dialer, this,
-                                                  Ftp::Client::ctrlClosed);
-    ctrl.opened(fwdState->serverConnection(), closer);
+/* Ftp::DataChannel */
+
+Ftp::DataChannel::DataChannel():
+        readBuf(NULL),
+        host(NULL),
+        port(0),
+        read_pending(false)
+{
+}
+
+Ftp::DataChannel::~DataChannel()
+{
+    delete readBuf;
 }
 
 void
-Ftp::Client::DataChannel::addr(const Ip::Address &import)
+Ftp::DataChannel::addr(const Ip::Address &import)
 {
      static char addrBuf[MAX_IPSTRLEN];
      import.toStr(addrBuf, sizeof(addrBuf));
@@ -130,6 +154,29 @@ Ftp::Client::DataChannel::addr(const Ip::Address &import)
      port = import.port();
 }
 
+/* Ftp::Client */
+
+Ftp::Client::Client(FwdState *fwdState):
+        AsyncJob("Ftp::Client"),
+        ::ServerStateData(fwdState),
+        ctrl(),
+        data(),
+        state(BEGIN),
+        old_request(NULL),
+        old_reply(NULL),
+        shortenReadTimeout(false)
+{
+    ++statCounter.server.all.requests;
+    ++statCounter.server.ftp.requests;
+
+    ctrl.last_command = xstrdup("Connect to server");
+
+    typedef CommCbMemFunT<Client, CommCloseCbParams> Dialer;
+    const AsyncCall::Pointer closer = JobCallback(9, 5, Dialer, this,
+                                                  Ftp::Client::ctrlClosed);
+    ctrl.opened(fwdState->serverConnection(), closer);
+}
+
 Ftp::Client::~Client()
 {
     if (data.opener != NULL) {
@@ -138,26 +185,8 @@ Ftp::Client::~Client()
     }
     data.close();
 
-    if (ctrl.buf) {
-        memFreeBuf(ctrl.size, ctrl.buf);
-        ctrl.buf = NULL;
-    }
-    if (ctrl.message)
-        wordlistDestroy(&ctrl.message);
-    safe_free(ctrl.last_command);
-    safe_free(ctrl.last_reply);
-
-    if (data.readBuf) {
-        if (!data.readBuf->isNull())
-            data.readBuf->clean();
-
-        delete data.readBuf;
-    }
-
     safe_free(old_request);
-
     safe_free(old_reply);
-
     fwd = NULL; // refcounted
 }
 
@@ -183,24 +212,24 @@ void
 Ftp::Client::closeServer()
 {
     if (Comm::IsConnOpen(ctrl.conn)) {
-        debugs(9,3, HERE << "closing FTP server FD " << ctrl.conn->fd << ", this " << this);
+        debugs(9, 3, "closing FTP server FD " << ctrl.conn->fd << ", this " << this);
         fwd->unregister(ctrl.conn);
         ctrl.close();
     }
 
     if (Comm::IsConnOpen(data.conn)) {
-        debugs(9,3, HERE << "closing FTP data FD " << data.conn->fd << ", this " << this);
+        debugs(9, 3, "closing FTP data FD " << data.conn->fd << ", this " << this);
         data.close();
     }
 
-    debugs(9,3, HERE << "FTP ctrl and data connections closed. this " << this);
+    debugs(9, 3, "FTP ctrl and data connections closed. this " << this);
 }
 
 /**
  * Did we close all FTP server connection(s)?
  *
- \retval true	Both server control and data channels are closed. And not waiting for a new data connection to open.
- \retval false	Either control channel or data is still active.
+ \retval true  Both server control and data channels are closed. And not waiting for a new data connection to open.
+ \retval false Either control channel or data is still active.
  */
 bool
 Ftp::Client::doneWithServer() const
@@ -211,7 +240,7 @@ Ftp::Client::doneWithServer() const
 void
 Ftp::Client::failed(err_type error, int xerrno)
 {
-    debugs(9,3,HERE << "entry-null=" << (entry?entry->isEmpty():0) << ", entry=" << entry);
+    debugs(9, 3, "entry-null=" << (entry?entry->isEmpty():0) << ", entry=" << entry);
 
     const char *command, *reply;
     const Http::StatusCode httpStatus = failedHttpStatus(error);
@@ -263,7 +292,7 @@ Ftp::Client::failedHttpStatus(err_type &error)
 void
 Ftp::Client::scheduleReadControlReply(int buffered_ok)
 {
-    debugs(9, 3, HERE << ctrl.conn);
+    debugs(9, 3, ctrl.conn);
 
     if (buffered_ok && ctrl.offset > 0) {
         /* We've already read some reply data */
@@ -277,9 +306,14 @@ Ftp::Client::scheduleReadControlReply(int buffered_ok)
             commUnsetConnTimeout(data.conn);
         }
 
+        const time_t tout = shortenReadTimeout ?
+                            min(Config.Timeout.connect, Config.Timeout.read):
+                            Config.Timeout.read;
+        shortenReadTimeout = false; // we only need to do this once, after PASV
+
         typedef CommCbMemFunT<Client, CommTimeoutCbParams> TimeoutDialer;
         AsyncCall::Pointer timeoutCall = JobCallback(9, 5, TimeoutDialer, this, Ftp::Client::timeout);
-        commSetConnTimeout(ctrl.conn, Config.Timeout.read, timeoutCall);
+        commSetConnTimeout(ctrl.conn, tout, timeoutCall);
 
         typedef CommCbMemFunT<Client, CommIoCbParams> Dialer;
         AsyncCall::Pointer reader = JobCallback(9, 5, Dialer, this, Ftp::Client::readControlReply);
@@ -290,7 +324,7 @@ Ftp::Client::scheduleReadControlReply(int buffered_ok)
 void
 Ftp::Client::readControlReply(const CommIoCbParams &io)
 {
-    debugs(9, 3, HERE << "FD " << io.fd << ", Read " << io.size << " bytes");
+    debugs(9, 3, "FD " << io.fd << ", Read " << io.size << " bytes");
 
     if (io.size > 0) {
         kb_incr(&(statCounter.server.all.kbytes_in), io.size);
@@ -313,7 +347,7 @@ Ftp::Client::readControlReply(const CommIoCbParams &io)
 
     if (io.flag != Comm::OK) {
         debugs(50, ignoreErrno(io.xerrno) ? 3 : DBG_IMPORTANT,
-               "ftpReadControlReply: read error: " << xstrerr(io.xerrno));
+               "FTP control reply read error: " << xstrerr(io.xerrno));
 
         if (ignoreErrno(io.xerrno)) {
             scheduleReadControlReply(0);
@@ -347,7 +381,7 @@ Ftp::Client::readControlReply(const CommIoCbParams &io)
 void
 Ftp::Client::handleControlReply()
 {
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     size_t bytes_used = 0;
     wordlistDestroy(&ctrl.message);
@@ -356,12 +390,12 @@ Ftp::Client::handleControlReply()
         /* didn't get complete reply yet */
 
         if (ctrl.offset == ctrl.size) {
-            ctrl.buf = (char *)memReallocBuf(ctrl.buf, ctrl.size << 1, &ctrl.size);
+            ctrl.buf = static_cast<char*>(memReallocBuf(ctrl.buf, ctrl.size << 1, &ctrl.size));
         }
 
         scheduleReadControlReply(0);
         return;
-    } 
+    }
 
     assert(ctrl.message); // the entire FTP server response, line by line
     assert(ctrl.replycode >= 0); // FTP status code (from the last line)
@@ -377,7 +411,7 @@ Ftp::Client::handleControlReply()
         memmove(ctrl.buf, ctrl.buf + bytes_used, ctrl.offset);
     }
 
-    debugs(9, 3, HERE << "state=" << state << ", code=" << ctrl.replycode);
+    debugs(9, 3, "state=" << state << ", code=" << ctrl.replycode);
 }
 
 bool
@@ -385,7 +419,7 @@ Ftp::Client::handlePasvReply(Ip::Address &srvAddr)
 {
     int code = ctrl.replycode;
     char *buf;
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     if (code != 227) {
         debugs(9, 2, "PASV not supported by remote end");
@@ -394,7 +428,7 @@ Ftp::Client::handlePasvReply(Ip::Address &srvAddr)
 
     /*  227 Entering Passive Mode (h1,h2,h3,h4,p1,p2).  */
     /*  ANSI sez [^0-9] is undefined, it breaks on Watcom cc */
-    debugs(9, 5, HERE << "scanning: " << ctrl.last_reply);
+    debugs(9, 5, "scanning: " << ctrl.last_reply);
 
     buf = ctrl.last_reply + strcspn(ctrl.last_reply, "0123456789");
 
@@ -416,7 +450,7 @@ Ftp::Client::handleEpsvReply(Ip::Address &remoteAddr)
 {
     int code = ctrl.replycode;
     char *buf;
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     if (code != 229 && code != 522) {
         if (code == 200) {
@@ -431,13 +465,14 @@ Ftp::Client::handleEpsvReply(Ip::Address &remoteAddr)
     }
 
     if (code == 522) {
-        /* server response with list of supported methods   */
-        /*   522 Network protocol not supported, use (1)    */
-        /*   522 Network protocol not supported, use (1,2)  */
-        /*   522 Network protocol not supported, use (2)  */
-        /* TODO: handle the (1,2) case. We might get it back after EPSV ALL
-         * which means close data + control without self-destructing and re-open from scratch. */
-        debugs(9, 5, HERE << "scanning: " << ctrl.last_reply);
+        /* Peer responded with a list of supported methods:
+         *   522 Network protocol not supported, use (1)
+         *   522 Network protocol not supported, use (1,2)
+         *   522 Network protocol not supported, use (2)
+         * TODO: Handle the (1,2) case which may happen after EPSV ALL. Close
+         * data + control without self-destructing and re-open from scratch.
+         */
+        debugs(9, 5, "scanning: " << ctrl.last_reply);
         buf = ctrl.last_reply;
         while (buf != NULL && *buf != '\0' && *buf != '\n' && *buf != '(')
             ++buf;
@@ -516,7 +551,7 @@ Ftp::Client::handleEpsvReply(Ip::Address &remoteAddr)
     return true;
 }
 
-// The server-side EPRT and PORT commands are not yet implemented.
+// FTP clients do not support EPRT and PORT commands yet.
 // The Ftp::Client::sendEprt() will fail because of the unimplemented
 // openListenSocket() or sendPort() methods
 bool
@@ -528,7 +563,7 @@ Ftp::Client::sendEprt()
         return sendPort();
     }
 
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     if (!openListenSocket()) {
         failed(ERR_FTP_FAILURE, 0);
@@ -537,7 +572,7 @@ Ftp::Client::sendEprt()
 
     debugs(9, 3, "Listening for FTP data connection with FD " << data.conn);
     if (!Comm::IsConnOpen(data.conn)) {
-        /* XXX Need to set error message */
+        // TODO: Set error message.
         failed(ERR_FTP_FAILURE, 0);
         return false;
     }
@@ -568,7 +603,7 @@ Ftp::Client::sendPort()
 bool
 Ftp::Client::sendPassive()
 {
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     /** \par
       * Checks for EPSV ALL special conditions:
@@ -608,7 +643,7 @@ Ftp::Client::sendPassive()
     switch (state) {
     case SENT_EPSV_ALL: /* EPSV ALL resulted in a bad response. Try ther EPSV methods. */
         if (ctrl.conn->local.isIPv6()) {
-            debugs(9, 5, HERE << "FTP Channel is IPv6 (" << ctrl.conn->remote << ") attempting EPSV 2 after EPSV ALL has failed.");
+            debugs(9, 5, "FTP Channel is IPv6 (" << ctrl.conn->remote << ") attempting EPSV 2 after EPSV ALL has failed.");
             mb.Printf("EPSV 2%s", Ftp::crlf);
             state = SENT_EPSV_2;
             break;
@@ -617,7 +652,7 @@ Ftp::Client::sendPassive()
 
     case SENT_EPSV_2: /* EPSV IPv6 failed. Try EPSV IPv4 */
         if (ctrl.conn->local.isIPv4()) {
-            debugs(9, 5, HERE << "FTP Channel is IPv4 (" << ctrl.conn->remote << ") attempting EPSV 1 after EPSV ALL has failed.");
+            debugs(9, 5, "FTP Channel is IPv4 (" << ctrl.conn->remote << ") attempting EPSV 1 after EPSV ALL has failed.");
             mb.Printf("EPSV 1%s", Ftp::crlf);
             state = SENT_EPSV_1;
             break;
@@ -629,7 +664,7 @@ Ftp::Client::sendPassive()
         // else fall through to skip EPSV 1
 
     case SENT_EPSV_1: /* EPSV options exhausted. Try PASV now. */
-        debugs(9, 5, HERE << "FTP Channel (" << ctrl.conn->remote << ") rejects EPSV connection attempts. Trying PASV instead.");
+        debugs(9, 5, "FTP Channel (" << ctrl.conn->remote << ") rejects EPSV connection attempts. Trying PASV instead.");
         mb.Printf("PASV%s", Ftp::crlf);
         state = SENT_PASV;
         break;
@@ -641,28 +676,28 @@ Ftp::Client::sendPassive()
             doEpsv = (checklist.fastCheck() == ACCESS_ALLOWED);
         }
         if (!doEpsv) {
-            debugs(9, 5, HERE << "EPSV support manually disabled. Sending PASV for FTP Channel (" << ctrl.conn->remote <<")");
+            debugs(9, 5, "EPSV support manually disabled. Sending PASV for FTP Channel (" << ctrl.conn->remote <<")");
             mb.Printf("PASV%s", Ftp::crlf);
             state = SENT_PASV;
         } else if (Config.Ftp.epsv_all) {
-            debugs(9, 5, HERE << "EPSV ALL manually enabled. Attempting with FTP Channel (" << ctrl.conn->remote <<")");
+            debugs(9, 5, "EPSV ALL manually enabled. Attempting with FTP Channel (" << ctrl.conn->remote <<")");
             mb.Printf("EPSV ALL%s", Ftp::crlf);
             state = SENT_EPSV_ALL;
         } else {
             if (ctrl.conn->local.isIPv6()) {
-                debugs(9, 5, HERE << "FTP Channel (" << ctrl.conn->remote << "). Sending default EPSV 2");
+                debugs(9, 5, "FTP Channel (" << ctrl.conn->remote << "). Sending default EPSV 2");
                 mb.Printf("EPSV 2%s", Ftp::crlf);
                 state = SENT_EPSV_2;
             }
             if (ctrl.conn->local.isIPv4()) {
-                debugs(9, 5, HERE << "Channel (" << ctrl.conn->remote <<"). Sending default EPSV 1");
+                debugs(9, 5, "Channel (" << ctrl.conn->remote <<"). Sending default EPSV 1");
                 mb.Printf("EPSV 1%s", Ftp::crlf);
                 state = SENT_EPSV_1;
             }
         }
         break;
     }
-	}
+    }
 
     if (ctrl.message)
         wordlistDestroy(&ctrl.message);
@@ -671,17 +706,7 @@ Ftp::Client::sendPassive()
 
     writeCommand(mb.content());
 
-    /*
-     * ugly hack for ftp servers like ftp.netscape.com that sometimes
-     * dont acknowledge PASV commands. Use connect timeout to be faster then read timeout (minutes).
-     */
-    /* XXX: resurrect or remove
-    typedef CommCbMemFunT<FtpStateData, CommTimeoutCbParams> TimeoutDialer;
-    AsyncCall::Pointer timeoutCall =  JobCallback(9, 5,
-                                      TimeoutDialer, this, FtpStateData::timeout);
-    commSetConnTimeout(ctrl.conn, Config.Timeout.connect, timeoutCall);
-    */
-
+    shortenReadTimeout = true;
     return true;
 }
 
@@ -703,9 +728,9 @@ Ftp::Client::connectDataChannel()
     conn->tos = ctrl.conn->tos;
     conn->nfmark = ctrl.conn->nfmark;
 
-    debugs(9, 3, HERE << "connecting to " << conn->remote);
+    debugs(9, 3, "connecting to " << conn->remote);
 
-    data.opener = commCbCall(9,3, "Ftp::Client::dataChannelConnected",
+    data.opener = commCbCall(9, 3, "Ftp::Client::dataChannelConnected",
                              CommConnectCbPtrFun(Ftp::Client::dataChannelConnected, this));
     Comm::ConnOpener *cs = new Comm::ConnOpener(conn, data.opener, Config.Timeout.connect);
     cs->setHost(data.host);
@@ -737,7 +762,7 @@ Ftp::Client::dataCloser()
 void
 Ftp::Client::dataClosed(const CommCloseCbParams &io)
 {
-    debugs(9, 4, HERE);
+    debugs(9, 4, status());
     if (data.listenConn != NULL) {
         data.listenConn->close();
         data.listenConn = NULL;
@@ -765,7 +790,7 @@ Ftp::Client::writeCommand(const char *buf)
     ctrl.last_command = ebuf;
 
     if (!Comm::IsConnOpen(ctrl.conn)) {
-        debugs(9, 2, HERE << "cannot send to closing ctrl " << ctrl.conn);
+        debugs(9, 2, "cannot send to closing ctrl " << ctrl.conn);
         // TODO: assert(ctrl.closer != NULL);
         return;
     }
@@ -782,7 +807,7 @@ void
 Ftp::Client::writeCommandCallback(const CommIoCbParams &io)
 {
 
-    debugs(9, 5, HERE << "wrote " << io.size << " bytes");
+    debugs(9, 5, "wrote " << io.size << " bytes");
 
     if (io.size > 0) {
         fd_bytes(io.fd, io.size, FD_WRITE);
@@ -794,7 +819,7 @@ Ftp::Client::writeCommandCallback(const CommIoCbParams &io)
         return;
 
     if (io.flag) {
-        debugs(9, DBG_IMPORTANT, "ftpWriteCommandCallback: " << io.conn << ": " << xstrerr(io.xerrno));
+        debugs(9, DBG_IMPORTANT, "FTP command write error: " << io.conn << ": " << xstrerr(io.xerrno));
         failed(ERR_WRITE_ERROR, io.xerrno);
         /* failed closes ctrl.conn and frees ftpState */
         return;
@@ -805,7 +830,7 @@ Ftp::Client::writeCommandCallback(const CommIoCbParams &io)
 void
 Ftp::Client::ctrlClosed(const CommCloseCbParams &io)
 {
-    debugs(9, 4, HERE);
+    debugs(9, 4, status());
     ctrl.clear();
     mustStop("Ftp::Client::ctrlClosed");
 }
@@ -813,7 +838,7 @@ Ftp::Client::ctrlClosed(const CommCloseCbParams &io)
 void
 Ftp::Client::timeout(const CommTimeoutCbParams &io)
 {
-    debugs(9, 4, HERE << io.conn << ": '" << entry->url() << "'" );
+    debugs(9, 4, io.conn << ": '" << entry->url() << "'" );
 
     if (abortOnBadEntry("entry went bad while waiting for a timeout"))
         return;
@@ -842,9 +867,9 @@ Ftp::Client::maybeReadVirginBody()
 
     const int read_sz = replyBodySpace(*data.readBuf, 0);
 
-    debugs(11,9, HERE << "FTP may read up to " << read_sz << " bytes");
+    debugs(11,9, "FTP may read up to " << read_sz << " bytes");
 
-    if (read_sz < 2)	// see http.cc
+    if (read_sz < 2) // see http.cc
         return;
 
     data.read_pending = true;
@@ -854,7 +879,7 @@ Ftp::Client::maybeReadVirginBody()
                                       TimeoutDialer, this, Ftp::Client::timeout);
     commSetConnTimeout(data.conn, Config.Timeout.read, timeoutCall);
 
-    debugs(9,5,HERE << "queueing read on FD " << data.conn->fd);
+    debugs(9,5,"queueing read on FD " << data.conn->fd);
 
     typedef CommCbMemFunT<Client, CommIoCbParams> Dialer;
     entry->delayAwareRead(data.conn, data.readBuf->space(), read_sz,
@@ -869,7 +894,7 @@ Ftp::Client::dataRead(const CommIoCbParams &io)
 
     data.read_pending = false;
 
-    debugs(9, 3, HERE << "FD " << io.fd << " Read " << io.size << " bytes");
+    debugs(9, 3, "FD " << io.fd << " Read " << io.size << " bytes");
 
     if (io.size > 0) {
         kb_incr(&(statCounter.server.all.kbytes_in), io.size);
@@ -903,7 +928,7 @@ Ftp::Client::dataRead(const CommIoCbParams &io)
 
     if (io.flag != Comm::OK) {
         debugs(50, ignoreErrno(io.xerrno) ? 3 : DBG_IMPORTANT,
-               HERE << "read error: " << xstrerr(io.xerrno));
+               "FTP data read error: " << xstrerr(io.xerrno));
 
         if (ignoreErrno(io.xerrno)) {
             maybeReadVirginBody();
@@ -913,7 +938,7 @@ Ftp::Client::dataRead(const CommIoCbParams &io)
             return;
         }
     } else if (io.size == 0) {
-        debugs(9,3, HERE << "Calling dataComplete() because io.size == 0");
+        debugs(9, 3, "Calling dataComplete() because io.size == 0");
         /*
          * DPW 2007-04-23
          * Dangerous curves ahead.  This call to dataComplete was
@@ -933,7 +958,7 @@ Ftp::Client::dataRead(const CommIoCbParams &io)
 void
 Ftp::Client::dataComplete()
 {
-    debugs(9, 3,HERE);
+    debugs(9, 3,status());
 
     /* Connection closed; transfer done. */
 
@@ -963,12 +988,12 @@ Ftp::Client::dataComplete()
  * Quickly abort the transaction
  *
  \todo destruction should be sufficient as the destructor should cleanup,
- *	including canceling close handlers
+ * including canceling close handlers
  */
 void
 Ftp::Client::abortTransaction(const char *reason)
 {
-    debugs(9, 3, HERE << "aborting transaction for " << reason <<
+    debugs(9, 3, "aborting transaction for " << reason <<
            "; FD " << (ctrl.conn!=NULL?ctrl.conn->fd:-1) << ", Data FD " << (data.conn!=NULL?data.conn->fd:-1) << ", this " << this);
     if (Comm::IsConnOpen(ctrl.conn)) {
         ctrl.conn->close();
@@ -1009,7 +1034,7 @@ void
 Ftp::Client::doneSendingRequestBody()
 {
     ::ServerStateData::doneSendingRequestBody();
-    debugs(9,3, HERE);
+    debugs(9, 3, status());
     dataComplete();
     /* NP: RFC 959  3.3.  DATA CONNECTION MANAGEMENT
      * if transfer type is 'stream' call dataComplete()
@@ -1031,7 +1056,7 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
     wordlist *list;
     wordlist **tail = &head;
     size_t linelen;
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
     /*
      * We need a NULL-terminated buffer for scanning, ick
      */
@@ -1045,15 +1070,15 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
 
     usable = end - sbuf;
 
-    debugs(9, 3, HERE << "usable = " << usable);
+    debugs(9, 3, "usable = " << usable);
 
     if (usable == 0) {
-        debugs(9, 3, HERE << "didn't find end of line");
+        debugs(9, 3, "didn't find end of line");
         safe_free(sbuf);
         return false;
     }
 
-    debugs(9, 3, HERE << len << " bytes to play with");
+    debugs(9, 3, len << " bytes to play with");
     ++end;
     s = sbuf;
     s += strspn(s, crlf);
@@ -1062,7 +1087,7 @@ Ftp::Client::parseControlReply(size_t &bytesUsed)
         if (complete)
             break;
 
-        debugs(9, 5, HERE << "s = {" << s << "}");
+        debugs(9, 5, "s = {" << s << "}");
 
         linelen = strcspn(s, crlf) + 1;
 
diff --git a/src/clients/FtpClient.h b/src/clients/FtpClient.h
index ab90b3dbbf..688f282b55 100644
--- a/src/clients/FtpClient.h
+++ b/src/clients/FtpClient.h
@@ -13,8 +13,8 @@ namespace Ftp {
 
 extern const char *const crlf;
 
-/// common code for FTP server control and data channels
-/// does not own the channel descriptor, which is managed by Ftp::Client
+/// Common code for FTP server control and data channels.
+/// Does not own the channel descriptor, which is managed by Ftp::Client.
 class Channel
 {
 public:
@@ -44,6 +44,44 @@ private:
     AsyncCall::Pointer closer; ///< Comm close handler callback
 };
 
+/// FTP channel for control commands.
+/// This channel is opened once per transaction.
+class CtrlChannel: public Ftp::Channel
+{
+public:
+    CtrlChannel();
+    ~CtrlChannel();
+
+    char *buf;
+    size_t size;
+    size_t offset;
+    wordlist *message;
+    char *last_command;
+    char *last_reply;
+    int replycode;
+
+private:
+    CtrlChannel(const CtrlChannel &); // not implemented
+    CtrlChannel &operator =(const CtrlChannel &); // not implemented
+};
+
+/// FTP channel for data exchanges.
+/// This channel may be opened/closed a few times.
+class DataChannel: public Ftp::Channel
+{
+public:
+    DataChannel();
+    ~DataChannel();
+
+    void addr(const Ip::Address &addr); ///< import host and port
+
+public:
+    MemBuf *readBuf;
+    char *host;
+    unsigned short port;
+    bool read_pending;
+};
+
 /// Base class for FTP Gateway and FTP Native client classes.
 class Client: public ::ServerStateData
 {
@@ -74,27 +112,8 @@ public:
     bool openListenSocket();
     void switchTimeoutToDataChannel();
 
-    // \todo: optimize ctrl and data structs member order, to minimize size
-    /// FTP control channel info; the channel is opened once per transaction
-    struct CtrlChannel: public Ftp::Channel {
-        char *buf;
-        size_t size;
-        size_t offset;
-        wordlist *message;
-        char *last_command;
-        char *last_reply;
-        int replycode;
-    } ctrl;
-
-    /// FTP data channel info; the channel may be opened/closed a few times
-    struct DataChannel: public Ftp::Channel {
-        MemBuf *readBuf;
-        char *host;
-        unsigned short port;
-        bool read_pending;
-
-        void addr(const Ip::Address &addr); ///< import host and port
-    } data;
+    CtrlChannel ctrl; ///< FTP control channel state
+    DataChannel data; ///< FTP data channel state
 
     enum {
         BEGIN,
@@ -162,6 +181,10 @@ protected:
 private:
     bool parseControlReply(size_t &bytesUsed);
 
+    /// XXX: An old hack for FTP servers like ftp.netscape.com that may not
+    /// respond to PASV. Use faster connect timeout instead of read timeout.
+    bool shortenReadTimeout;
+
     CBDATA_CLASS2(Client);
 };
 
diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index ecab798771..1edde3ef07 100644
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -119,7 +119,7 @@ typedef void (StateMethod)(Ftp::Gateway *);
 
 /// \ingroup ServerProtocolFTPInternal
 /// FTP Gateway: An FTP client that takes an HTTP request with an ftp:// URI,
-/// converts it into one or more FTP commands, and then 
+/// converts it into one or more FTP commands, and then
 /// converts one or more FTP responses into the final HTTP response.
 class Gateway : public Ftp::Client
 {
@@ -389,8 +389,8 @@ Ftp::Gateway::~Gateway()
     debugs(9, 3, HERE << entry->url()  );
 
     if (Comm::IsConnOpen(ctrl.conn)) {
-        debugs(9, DBG_IMPORTANT, HERE << "Internal bug: Ftp::ServerStateData "
-               "left open control channel " << ctrl.conn);
+        debugs(9, DBG_IMPORTANT, "Internal bug: FTP Gateway left open " <<
+               "control channel " << ctrl.conn);
     }
 
     if (reply_hdr) {
@@ -509,7 +509,7 @@ Ftp::Gateway::timeout(const CommTimeoutCbParams &io)
     if (SENT_PASV == state) {
         /* stupid ftp.netscape.com, of FTP server behind stupid firewall rules */
         flags.pasv_supported = false;
-        debugs(9, DBG_IMPORTANT, HERE << "timeout in SENT_PASV state");
+        debugs(9, DBG_IMPORTANT, "FTP Gateway timeout in SENT_PASV state");
 
         // cancel the data connection setup.
         if (data.opener != NULL) {
@@ -1009,7 +1009,7 @@ Ftp::Gateway::parseListing()
 void
 Ftp::Gateway::processReplyBody()
 {
-    debugs(9, 3, HERE << "Ftp::Gateway::processReplyBody starting.");
+    debugs(9, 3, status());
 
     if (request->method == Http::METHOD_HEAD && (flags.isdir || theSize != -1)) {
         serverComplete();
@@ -1032,7 +1032,7 @@ Ftp::Gateway::processReplyBody()
 #if USE_ADAPTATION
 
     if (adaptationAccessCheckPending) {
-        debugs(9,3, HERE << "returning from Ftp::Gateway::processReplyBody due to adaptationAccessCheckPending");
+        debugs(9, 3, "returning from Ftp::Gateway::processReplyBody due to adaptationAccessCheckPending");
         return;
     }
 
@@ -1214,9 +1214,7 @@ Ftp::Gateway::start()
     buildTitleUrl();
     debugs(9, 5, HERE << "FD " << ctrl.conn->fd << " : host=" << request->GetHost() <<
            ", path=" << request->urlpath << ", user=" << user << ", passwd=" << password);
-
     state = BEGIN;
-
     Ftp::Client::start();
 }
 
@@ -2553,8 +2551,11 @@ Ftp::Gateway::failedHttpStatus(err_type &error)
 {
     if (error == ERR_NONE) {
         switch (state) {
+
         case SENT_USER:
+
         case SENT_PASS:
+
             if (ctrl.replycode > 500) {
                 error = ERR_FTP_FORBIDDEN;
                 return password_url ? Http::scForbidden : Http::scUnauthorized;
@@ -2563,13 +2564,16 @@ Ftp::Gateway::failedHttpStatus(err_type &error)
                 return Http::scServiceUnavailable;
             }
             break;
+
         case SENT_CWD:
+
         case SENT_RETR:
             if (ctrl.replycode == 550) {
                 error = ERR_FTP_NOT_FOUND;
                 return Http::scNotFound;
             }
             break;
+
         default:
             break;
         }
diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index fda91a8979..4163686870 100644
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -20,7 +20,7 @@
 
 namespace Ftp {
 
-/// An FTP client receiving native FTP commands from our FTP server 
+/// An FTP client receiving native FTP commands from our FTP server
 /// (Ftp::Server), forwarding them to the next FTP hop,
 /// and then relaying FTP replies back to our FTP server.
 class Relay: public Ftp::Client
@@ -32,11 +32,12 @@ public:
 protected:
     const Ftp::MasterState &master() const;
     Ftp::MasterState &updateMaster();
-    Ftp::ServerState clientState() const;
-    void clientState(Ftp::ServerState newState);
+    Ftp::ServerState serverState() const { return master().serverState; }
+    void serverState(const Ftp::ServerState newState);
 
     /* Ftp::Client API */
     virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
+    virtual void dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno);
 
     /* ServerStateData API */
     virtual void serverComplete();
@@ -78,7 +79,6 @@ protected:
     void readCwdOrCdupReply();
     void readUserOrPassReply();
 
-    virtual void dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno);
     void scheduleReadControlReply();
 
     bool forwardingCompleted; ///< completeForwarding() has been called
@@ -101,9 +101,9 @@ const Ftp::Relay::SM_FUNC Ftp::Relay::SM_FUNCS[] = {
     &Ftp::Relay::readGreeting, // BEGIN
     &Ftp::Relay::readUserOrPassReply, // SENT_USER
     &Ftp::Relay::readUserOrPassReply, // SENT_PASS
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_TYPE
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_MDTM
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_SIZE
+    NULL,/* &Ftp::Relay::readReply */ // SENT_TYPE
+    NULL,/* &Ftp::Relay::readReply */ // SENT_MDTM
+    NULL,/* &Ftp::Relay::readReply */ // SENT_SIZE
     NULL, // SENT_EPRT
     NULL, // SENT_PORT
     &Ftp::Relay::readEpsvReply, // SENT_EPSV_ALL
@@ -111,17 +111,17 @@ const Ftp::Relay::SM_FUNC Ftp::Relay::SM_FUNCS[] = {
     &Ftp::Relay::readEpsvReply, // SENT_EPSV_2
     &Ftp::Relay::readPasvReply, // SENT_PASV
     &Ftp::Relay::readCwdOrCdupReply,  // SENT_CWD
-    NULL,/*&Ftp::Relay::readDataReply,*/ // SENT_LIST
-    NULL,/*&Ftp::Relay::readDataReply,*/ // SENT_NLST
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_REST
-    NULL,/*&Ftp::Relay::readDataReply*/ // SENT_RETR
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_STOR
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_QUIT
+    NULL,/* &Ftp::Relay::readDataReply, */ // SENT_LIST
+    NULL,/* &Ftp::Relay::readDataReply, */ // SENT_NLST
+    NULL,/* &Ftp::Relay::readReply */ // SENT_REST
+    NULL,/* &Ftp::Relay::readDataReply */ // SENT_RETR
+    NULL,/* &Ftp::Relay::readReply */ // SENT_STOR
+    NULL,/* &Ftp::Relay::readReply */ // SENT_QUIT
     &Ftp::Relay::readTransferDoneReply, // READING_DATA
     &Ftp::Relay::readReply, // WRITING_DATA
-    NULL,/*&Ftp::Relay::readReply*/ // SENT_MKDIR
+    NULL,/* &Ftp::Relay::readReply */ // SENT_MKDIR
     &Ftp::Relay::readFeatReply, // SENT_FEAT
-    NULL,/*&Ftp::Relay::readPwdReply*/ // SENT_PWD
+    NULL,/* &Ftp::Relay::readPwdReply */ // SENT_PWD
     &Ftp::Relay::readCwdOrCdupReply, // SENT_CDUP
     &Ftp::Relay::readDataReply,// SENT_DATA_REQUEST
     &Ftp::Relay::readReply, // SENT_COMMAND
@@ -159,8 +159,8 @@ Ftp::Relay::start()
     if (!master().clientReadGreeting)
         Ftp::Client::start();
     else
-    if (clientState() == fssHandleDataRequest ||
-        clientState() == fssHandleUploadRequest)
+    if (serverState() == fssHandleDataRequest ||
+        serverState() == fssHandleUploadRequest)
         handleDataRequest();
     else
         sendCommand();
@@ -191,6 +191,8 @@ Ftp::Relay::serverComplete()
     Ftp::Client::serverComplete();
 }
 
+/// Safely returns the master state,
+/// with safety checks in case the Ftp::Server side of the master xact is gone.
 Ftp::MasterState &
 Ftp::Relay::updateMaster()
 {
@@ -206,22 +208,17 @@ Ftp::Relay::updateMaster()
     return Master;
 }
 
+/// A const variant of updateMaster().
 const Ftp::MasterState &
 Ftp::Relay::master() const
 {
-    return const_cast<Ftp::Relay*>(this)->updateMaster();
-}
-
-Ftp::ServerState
-Ftp::Relay::clientState() const
-{
-    return master().serverState;
+    return const_cast<Ftp::Relay*>(this)->updateMaster(); // avoid code dupe
 }
 
+/// Changes server state and debugs about that important event.
 void
-Ftp::Relay::clientState(Ftp::ServerState newState)
+Ftp::Relay::serverState(const Ftp::ServerState newState)
 {
-    // XXX: s/client/server/g
     Ftp::ServerState &cltState = updateMaster().serverState;
     debugs(9, 3, "client state was " << cltState << " now: " << newState);
     cltState = newState;
@@ -229,7 +226,7 @@ Ftp::Relay::clientState(Ftp::ServerState newState)
 
 /**
  * Ensure we do not double-complete on the forward entry.
- * We complete forwarding when the response adaptation is over 
+ * We complete forwarding when the response adaptation is over
  * (but we may still be waiting for 226 from the FTP server) and
  * also when we get that 226 from the server (and adaptation is done).
  *
@@ -249,7 +246,7 @@ void
 Ftp::Relay::failed(err_type error, int xerrno)
 {
     if (!doneWithServer())
-        clientState(fssError);
+        serverState(fssError);
 
     // TODO: we need to customize ErrorState instead
     if (entry->isEmpty())
@@ -271,7 +268,7 @@ Ftp::Relay::failedErrorMessage(err_type error, int xerrno)
 void
 Ftp::Relay::processReplyBody()
 {
-    debugs(9, 3, HERE << "starting");
+    debugs(9, 3, status());
 
     if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
         /*
@@ -285,7 +282,7 @@ Ftp::Relay::processReplyBody()
 #if USE_ADAPTATION
 
     if (adaptationAccessCheckPending) {
-        debugs(9,3, HERE << "returning due to adaptationAccessCheckPending");
+        debugs(9, 3, "returning due to adaptationAccessCheckPending");
         return;
     }
 
@@ -293,7 +290,7 @@ Ftp::Relay::processReplyBody()
 
     if (data.readBuf != NULL && data.readBuf->hasContent()) {
         const mb_size_t csize = data.readBuf->contentSize();
-        debugs(9, 5, HERE << "writing " << csize << " bytes to the reply");
+        debugs(9, 5, "writing " << csize << " bytes to the reply");
         addVirginReplyBody(data.readBuf->content(), csize);
         data.readBuf->consume(csize);
     }
@@ -353,7 +350,7 @@ Ftp::Relay::forwardReply()
 void
 Ftp::Relay::forwardPreliminaryReply(const PreliminaryCb cb)
 {
-    debugs(9, 5, HERE << "Forwarding preliminary reply to client");
+    debugs(9, 5, "forwarding preliminary reply to client");
 
     // we must prevent concurrent ConnStateData::sendControlMsg() calls
     Must(thePreliminaryCb == NULL);
@@ -373,7 +370,7 @@ Ftp::Relay::forwardPreliminaryReply(const PreliminaryCb cb)
 void
 Ftp::Relay::proceedAfterPreliminaryReply()
 {
-    debugs(9, 5, HERE << "Proceeding after preliminary reply to client");
+    debugs(9, 5, "proceeding after preliminary reply to client");
 
     Must(thePreliminaryCb != NULL);
     const PreliminaryCb cb = thePreliminaryCb;
@@ -404,7 +401,7 @@ Ftp::Relay::createHttpReply(const Http::StatusCode httpStatus, const int clen)
 
     if (ctrl.message) {
         for (wordlist *W = ctrl.message; W && W->next; W = W->next)
-            header.putStr(HDR_FTP_PRE, httpHeaderQuoteString(W->key).termedBuf());
+            header.putStr(HDR_FTP_PRE, httpHeaderQuoteString(W->key).c_str());
     }
     if (ctrl.replycode > 0)
         header.putInt(HDR_FTP_STATUS, ctrl.replycode);
@@ -428,7 +425,7 @@ Ftp::Relay::startDataDownload()
 {
     assert(Comm::IsConnOpen(data.conn));
 
-    debugs(9, 3, HERE << "begin data transfer from " << data.conn->remote <<
+    debugs(9, 3, "begin data transfer from " << data.conn->remote <<
            " (" << data.conn->local << ")");
 
     HttpReply *const reply = createHttpReply(Http::scOkay, -1);
@@ -445,7 +442,7 @@ Ftp::Relay::startDataUpload()
 {
     assert(Comm::IsConnOpen(data.conn));
 
-    debugs(9, 3, HERE << "begin data transfer to " << data.conn->remote <<
+    debugs(9, 3, "begin data transfer to " << data.conn->remote <<
            " (" << data.conn->local << ")");
 
     if (!startRequestBodyFlow()) { // register to receive body data
@@ -464,8 +461,8 @@ Ftp::Relay::readGreeting()
     switch (ctrl.replycode) {
     case 220:
         updateMaster().clientReadGreeting = true;
-        if (clientState() == fssBegin)
-            clientState(fssConnected);
+        if (serverState() == fssBegin)
+            serverState(fssConnected);
 
         // Do not forward server greeting to the client because our client
         // side code has greeted the client already. Also, a greeting may
@@ -499,14 +496,14 @@ Ftp::Relay::sendCommand()
     const String &params = header.findEntry(HDR_FTP_ARGUMENTS)->value;
 
     if (params.size() > 0)
-        debugs(9, 5, HERE << "command: " << cmd << ", parameters: " << params);
+        debugs(9, 5, "command: " << cmd << ", parameters: " << params);
     else
-        debugs(9, 5, HERE << "command: " << cmd << ", no parameters");
+        debugs(9, 5, "command: " << cmd << ", no parameters");
 
-    if (clientState() == fssHandlePasv ||
-        clientState() == fssHandleEpsv ||
-        clientState() == fssHandleEprt ||
-        clientState() == fssHandlePort) {
+    if (serverState() == fssHandlePasv ||
+        serverState() == fssHandleEpsv ||
+        serverState() == fssHandleEprt ||
+        serverState() == fssHandlePort) {
         sendPassive();
         return;
     }
@@ -521,21 +518,21 @@ Ftp::Relay::sendCommand()
     writeCommand(mb.content());
 
     state =
-        clientState() == fssHandleCdup ? SENT_CDUP :
-        clientState() == fssHandleCwd ? SENT_CWD :
-        clientState() == fssHandleFeat ? SENT_FEAT :
-        clientState() == fssHandleDataRequest ? SENT_DATA_REQUEST :
-        clientState() == fssHandleUploadRequest ? SENT_DATA_REQUEST :
-        clientState() == fssConnected ? SENT_USER :
-        clientState() == fssHandlePass ? SENT_PASS :
+        serverState() == fssHandleCdup ? SENT_CDUP :
+        serverState() == fssHandleCwd ? SENT_CWD :
+        serverState() == fssHandleFeat ? SENT_FEAT :
+        serverState() == fssHandleDataRequest ? SENT_DATA_REQUEST :
+        serverState() == fssHandleUploadRequest ? SENT_DATA_REQUEST :
+        serverState() == fssConnected ? SENT_USER :
+        serverState() == fssHandlePass ? SENT_PASS :
         SENT_COMMAND;
 }
 
 void
 Ftp::Relay::readReply()
 {
-    assert(clientState() == fssConnected ||
-           clientState() == fssHandleUploadRequest);
+    assert(serverState() == fssConnected ||
+           serverState() == fssHandleUploadRequest);
 
     if (100 <= ctrl.replycode && ctrl.replycode < 200)
         forwardPreliminaryReply(&Ftp::Relay::scheduleReadControlReply);
@@ -546,7 +543,7 @@ Ftp::Relay::readReply()
 void
 Ftp::Relay::readFeatReply()
 {
-    assert(clientState() == fssHandleFeat);
+    assert(serverState() == fssHandleFeat);
 
     if (100 <= ctrl.replycode && ctrl.replycode < 200)
         return; // ignore preliminary replies
@@ -557,7 +554,7 @@ Ftp::Relay::readFeatReply()
 void
 Ftp::Relay::readPasvReply()
 {
-    assert(clientState() == fssHandlePasv || clientState() == fssHandleEpsv || clientState() == fssHandlePort || clientState() == fssHandleEprt);
+    assert(serverState() == fssHandlePasv || serverState() == fssHandleEpsv || serverState() == fssHandlePort || serverState() == fssHandleEprt);
 
     if (100 <= ctrl.replycode && ctrl.replycode < 200)
         return; // ignore preliminary replies
@@ -586,13 +583,13 @@ Ftp::Relay::readEpsvReply()
 void
 Ftp::Relay::readDataReply()
 {
-    assert(clientState() == fssHandleDataRequest ||
-           clientState() == fssHandleUploadRequest);
+    assert(serverState() == fssHandleDataRequest ||
+           serverState() == fssHandleUploadRequest);
 
     if (ctrl.replycode == 125 || ctrl.replycode == 150) {
-        if (clientState() == fssHandleDataRequest)
+        if (serverState() == fssHandleDataRequest)
             forwardPreliminaryReply(&Ftp::Relay::startDataDownload);
-        else // clientState() == fssHandleUploadRequest
+        else // serverState() == fssHandleUploadRequest
             forwardPreliminaryReply(&Ftp::Relay::startDataUpload);
     } else
         forwardReply();
@@ -604,7 +601,7 @@ Ftp::Relay::startDirTracking()
     if (!fwd->request->clientConnectionManager->port->ftp_track_dirs)
         return false;
 
-    debugs(9, 5, "Start directory tracking");
+    debugs(9, 5, "start directory tracking");
     savedReply.message = ctrl.message;
     savedReply.lastCommand = ctrl.last_command;
     savedReply.lastReply = ctrl.last_reply;
@@ -621,7 +618,7 @@ Ftp::Relay::startDirTracking()
 void
 Ftp::Relay::stopDirTracking()
 {
-    debugs(9, 5, "Got code from pwd: " << ctrl.replycode << ", msg: " << ctrl.last_reply);
+    debugs(9, 5, "got code from pwd: " << ctrl.replycode << ", msg: " << ctrl.last_reply);
 
     if (ctrl.replycode == 257)
         updateMaster().workingDir = Ftp::UnescapeDoubleQuoted(ctrl.last_reply);
@@ -643,10 +640,10 @@ Ftp::Relay::stopDirTracking()
 void
 Ftp::Relay::readCwdOrCdupReply()
 {
-    assert(clientState() == fssHandleCwd ||
-           clientState() == fssHandleCdup);
+    assert(serverState() == fssHandleCwd ||
+           serverState() == fssHandleCdup);
 
-    debugs(9, 5, HERE << "Got code " << ctrl.replycode << ", msg: " << ctrl.last_reply);
+    debugs(9, 5, "got code " << ctrl.replycode << ", msg: " << ctrl.last_reply);
 
     if (100 <= ctrl.replycode && ctrl.replycode < 200)
         return;
@@ -678,11 +675,11 @@ Ftp::Relay::readUserOrPassReply()
 void
 Ftp::Relay::readTransferDoneReply()
 {
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
 
     if (ctrl.replycode != 226 && ctrl.replycode != 250) {
-        debugs(9, DBG_IMPORTANT, HERE << "Got code " << ctrl.replycode <<
-               " after reading data");
+        debugs(9, DBG_IMPORTANT, "got FTP code " << ctrl.replycode <<
+               " after reading response data");
     }
 
     serverComplete();
@@ -691,16 +688,16 @@ Ftp::Relay::readTransferDoneReply()
 void
 Ftp::Relay::dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno)
 {
-    debugs(9, 3, HERE);
+    debugs(9, 3, status());
     data.opener = NULL;
 
     if (err != Comm::OK) {
-        debugs(9, 2, HERE << "Failed to connect FTP server data channel.");
+        debugs(9, 2, "failed to connect FTP server data channel");
         forwardError(ERR_CONNECT_FAIL, xerrno);
         return;
     }
 
-    debugs(9, 2, HERE << "Connected FTP server data channel: " << conn);
+    debugs(9, 2, "connected FTP server data channel: " << conn);
 
     data.opened(conn, dataCloser());
 
diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 5c38fe2e3d..effd90e8cd 100644
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -29,6 +29,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Ftp, Server);
 namespace Ftp {
 static void PrintReply(MemBuf &mb, const HttpReply *reply, const char *const prefix = "");
 static bool SupportedCommand(const String &name);
+static bool CommandHasPathParameter(const String &cmd);
 };
 
 Ftp::Server::Server(const MasterXaction::Pointer &xact):
@@ -76,7 +77,7 @@ Ftp::Server::start()
         clientConnection->local.toUrl(buf, MAX_IPSTRLEN);
         host = buf;
         calcUri();
-        debugs(33, 5, HERE << "FTP transparent URL: " << uri);
+        debugs(33, 5, "FTP transparent URL: " << uri);
     }
 
     writeEarlyReply(220, "Service ready");
@@ -93,7 +94,7 @@ Ftp::Server::maybeReadUploadData()
     if (availSpace <= 0)
         return;
 
-    debugs(33, 4, HERE << dataConn << ": reading FTP data...");
+    debugs(33, 4, dataConn << ": reading FTP data...");
 
     typedef CommCbMemFunT<Server, CommIoCbParams> Dialer;
     reader = JobCallback(33, 5, Dialer, this, Ftp::Server::readUploadData);
@@ -152,7 +153,7 @@ Ftp::Server::processParsedRequest(ClientSocketContext *context, const Http::Prot
 void
 Ftp::Server::readUploadData(const CommIoCbParams &io)
 {
-    debugs(33,5,HERE << io.conn << " size " << io.size);
+    debugs(33, 5, io.conn << " size " << io.size);
     Must(reader != NULL);
     reader = NULL;
 
@@ -169,13 +170,13 @@ Ftp::Server::readUploadData(const CommIoCbParams &io)
             uploadAvailSize += io.size;
             shovelUploadData();
         } else if (io.size == 0) {
-            debugs(33, 5, HERE << io.conn << " closed");
+            debugs(33, 5, io.conn << " closed");
             closeDataConnection();
             if (uploadAvailSize <= 0)
                 finishDechunkingRequest(true);
         }
     } else { // not Comm::Flags::OK or unexpected read
-        debugs(33, 5, HERE << io.conn << " closed");
+        debugs(33, 5, io.conn << " closed");
         closeDataConnection();
         finishDechunkingRequest(false);
     }
@@ -188,7 +189,7 @@ Ftp::Server::shovelUploadData()
 {
     assert(bodyPipe != NULL);
 
-    debugs(33,5, HERE << "handling FTP request data for " << clientConnection);
+    debugs(33, 5, "handling FTP request data for " << clientConnection);
     const size_t putSize = bodyPipe->putMoreData(uploadBuf,
                                                  uploadAvailSize);
     if (putSize > 0) {
@@ -227,11 +228,11 @@ Ftp::Server::AcceptCtrlConnection(const CommAcceptCbParams &params)
 
     if (params.flag != Comm::OK) {
         // Its possible the call was still queued when the client disconnected
-        debugs(33, 2, "ftpAccept: " << s->listenConn << ": accept failure: " << xstrerr(params.xerrno));
+        debugs(33, 2, s->listenConn << ": FTP accept failure: " << xstrerr(params.xerrno));
         return;
     }
 
-    debugs(33, 4, HERE << params.conn << ": accepted");
+    debugs(33, 4, params.conn << ": accepted");
     fd_note(params.conn->fd, "client ftp connect");
 
     if (s->tcp_keepalive.enabled)
@@ -297,14 +298,21 @@ Ftp::Server::clientPinnedConnectionClosed(const CommCloseCbParams &io)
     ConnStateData::clientPinnedConnectionClosed(io);
 
     // if the server control connection is gone, reset state to login again
-    // TODO: merge with similar code in ftpHandleUserRequest()
-    debugs(33, 5, "will need to re-login due to FTP server closure");
-    master.clientReadGreeting = false;
-    changeState(fssBegin, "server closure");
+    resetLogin("control connection closure");
+
     // XXX: Not enough. Gateway::ServerStateData::sendCommand() will not
     // re-login because clientState() is not ConnStateData::FTP_CONNECTED.
 }
 
+/// clear client and server login-related state after the old login is gone
+void
+Ftp::Server::resetLogin(const char *reason)
+{
+    debugs(33, 5, "will need to re-login due to " << reason);
+    master.clientReadGreeting = false;
+    changeState(fssBegin, reason);
+}
+
 /// computes uri member from host and, if tracked, working dir with file name
 void
 Ftp::Server::calcUri(const char *file)
@@ -415,7 +423,7 @@ Ftp::Server::closeDataConnection()
     }
 
     if (Comm::IsConnOpen(dataListenConn)) {
-        debugs(33, 5, HERE << "FTP closing client data listen socket: " <<
+        debugs(33, 5, "FTP closing client data listen socket: " <<
                *dataListenConn);
         dataListenConn->close();
     }
@@ -428,7 +436,7 @@ Ftp::Server::closeDataConnection()
     }
 
     if (Comm::IsConnOpen(dataConn)) {
-        debugs(33, 5, HERE << "FTP closing client data connection: " <<
+        debugs(33, 5, "FTP closing client data connection: " <<
                *dataConn);
         dataConn->close();
     }
@@ -440,7 +448,7 @@ Ftp::Server::closeDataConnection()
 void
 Ftp::Server::writeEarlyReply(const int code, const char *msg)
 {
-    debugs(33, 7, HERE << code << ' ' << msg);
+    debugs(33, 7, code << ' ' << msg);
     assert(99 < code && code < 1000);
 
     MemBuf mb;
@@ -471,7 +479,7 @@ Ftp::Server::writeReply(MemBuf &mb)
 void
 Ftp::Server::writeCustomReply(const int code, const char *msg, const HttpReply *reply)
 {
-    debugs(33, 7, HERE << code << ' ' << msg);
+    debugs(33, 7, code << ' ' << msg);
     assert(99 < code && code < 1000);
 
     const bool sendDetails = reply != NULL &&
@@ -506,7 +514,7 @@ Ftp::Server::changeState(const ServerState newState, const char *reason)
 
 /// whether the given FTP command has a pathname parameter
 static bool
-ftpHasPathParameter(const String &cmd)
+Ftp::CommandHasPathParameter(const String &cmd)
 {
     static const char *pathCommandsStr[]= {"CWD","SMNT", "RETR", "STOR", "APPE",
                                            "RNFR", "RNTO", "DELE", "RMD", "MKD",
@@ -536,7 +544,7 @@ Ftp::Server::parseOneRequest(Http::ProtocolVersion &ver)
     }
 
     if (eor == NULL) {
-        debugs(33, 5, HERE << "Incomplete request, waiting for end of request");
+        debugs(33, 5, "Incomplete request, waiting for end of request");
         return NULL;
     }
 
@@ -546,7 +554,7 @@ Ftp::Server::parseOneRequest(Http::ProtocolVersion &ver)
     const char *boc = inBuf; // beginning of command
     while (boc < eor && isspace(*boc)) ++boc;
     if (boc >= eor) {
-        debugs(33, 5, HERE << "Empty request, ignoring");
+        debugs(33, 5, "Empty request, ignoring");
         consumeInput(req_sz);
         return NULL;
     }
@@ -565,7 +573,7 @@ Ftp::Server::parseOneRequest(Http::ProtocolVersion &ver)
     } else
         bop = NULL;
 
-    debugs(33, 7, HERE << "Parsed FTP command " << boc << " with " <<
+    debugs(33, 7, "Parsed FTP command " << boc << " with " <<
            (bop == NULL ? "no " : "") << "parameters" <<
            (bop != NULL ? ": " : "") << bop);
 
@@ -575,17 +583,20 @@ Ftp::Server::parseOneRequest(Http::ProtocolVersion &ver)
 
     consumeInput(req_sz);
 
-    if (!master.clientReadGreeting) {
-        // the first command must be USER
-        if (!pinning.pinned && cmd.caseCmp("USER") != 0) {
-            writeEarlyReply(530, "Must login first");
-            return NULL;
+    // interception cases do not need USER to calculate the uri
+    if (!transparent()) {
+        if (!master.clientReadGreeting) {
+            // the first command must be USER
+            if (!pinning.pinned && cmd.caseCmp("USER") != 0) {
+                writeEarlyReply(530, "Must login first");
+                return NULL;
+            }
         }
-    }
 
-    // We need to process USER request now because it sets ftp server Hostname.
-    if (cmd.caseCmp("USER") == 0 && !handleUserRequest(cmd, params))
-        return NULL;
+        // process USER request now because it sets FTP peer host name
+        if (cmd.caseCmp("USER") == 0 && !handleUserRequest(cmd, params))
+            return NULL;
+    }
 
     if (!Ftp::SupportedCommand(cmd)) {
         writeEarlyReply(502, "Unknown or unsupported command");
@@ -596,13 +607,13 @@ Ftp::Server::parseOneRequest(Http::ProtocolVersion &ver)
         !cmd.caseCmp("APPE") || !cmd.caseCmp("STOR") || !cmd.caseCmp("STOU") ?
         Http::METHOD_PUT : Http::METHOD_GET;
 
-    const char *aPath = params.size() > 0 && ftpHasPathParameter(cmd) ?
+    const char *aPath = params.size() > 0 && CommandHasPathParameter(cmd) ?
         params.termedBuf() : NULL;
     calcUri(aPath);
     char *newUri = xstrdup(uri.termedBuf());
     HttpRequest *const request = HttpRequest::CreateFromUrlAndMethod(newUri, method);
     if (!request) {
-        debugs(33, 5, HERE << "Invalid FTP URL: " << uri);
+        debugs(33, 5, "Invalid FTP URL: " << uri);
         writeEarlyReply(501, "Invalid host");
         uri.clean();
         safe_free(newUri);
@@ -841,7 +852,7 @@ Ftp::Server::handleDataReply(const HttpReply *reply, StoreIOBuffer data)
         return;
     }
 
-    debugs(33, 7, HERE << data.length);
+    debugs(33, 7, data.length);
 
     if (data.length <= 0) {
         replyDataWritingCheckpoint(); // skip the actual write call
@@ -867,7 +878,7 @@ Ftp::Server::wroteReplyData(const CommIoCbParams &io)
         return;
 
     if (io.flag != Comm::OK) {
-        debugs(33, 3, HERE << "FTP reply data writing failed: " <<
+        debugs(33, 3, "FTP reply data writing failed: " <<
                xstrerr(io.xerrno));
         closeDataConnection();
         writeCustomReply(426, "Data connection error; transfer aborted");
@@ -888,15 +899,15 @@ Ftp::Server::replyDataWritingCheckpoint() {
         getCurrentContext()->pullData();
         return;
     case STREAM_COMPLETE:
-        debugs(33, 3, HERE << "FTP reply data transfer successfully complete");
+        debugs(33, 3, "FTP reply data transfer successfully complete");
         writeCustomReply(226, "Transfer complete");
         break;
     case STREAM_UNPLANNED_COMPLETE:
-        debugs(33, 3, HERE << "FTP reply data transfer failed: STREAM_UNPLANNED_COMPLETE");
+        debugs(33, 3, "FTP reply data transfer failed: STREAM_UNPLANNED_COMPLETE");
         writeCustomReply(451, "Server error; transfer aborted");
         break;
     case STREAM_FAILED:
-        debugs(33, 3, HERE << "FTP reply data transfer failed: STREAM_FAILED");
+        debugs(33, 3, "FTP reply data transfer failed: STREAM_FAILED");
         writeCustomReply(451, "Server error; transfer aborted");
         break;
     default:
@@ -1037,7 +1048,7 @@ Ftp::Server::writeForwardedReplyAndCall(const HttpReply *reply, AsyncCall::Point
     Must(header.has(HDR_FTP_STATUS));
     Must(header.has(HDR_FTP_REASON));
     const int scode = header.getInt(HDR_FTP_STATUS);
-    debugs(33, 7, HERE << "scode: " << scode);
+    debugs(33, 7, "scode: " << scode);
 
     // Status 125 or 150 implies upload or data request, but we still check
     // the state in case the server is buggy.
@@ -1224,13 +1235,15 @@ Ftp::Server::handleUserRequest(const String &cmd, String &params)
         return false;
     }
 
+    // find the [end of] user name
     const String::size_type eou = params.rfind('@');
     if (eou == String::npos || eou + 1 >= params.size()) {
         writeEarlyReply(501, "Missing host");
         return false;
     }
+    // const String login = params.substr(0, eou);
 
-    const String login = params.substr(0, eou);
+    // Determine the intended destination.
     host = params.substr(eou + 1, params.size());
     // If we can parse it as raw IPv6 address, then surround with "[]".
     // Otherwise (domain, IPv4, [bracketed] IPv6, garbage, etc), use as is.
@@ -1254,13 +1267,12 @@ Ftp::Server::handleUserRequest(const String &cmd, String &params)
     if (!master.clientReadGreeting) {
         debugs(11, 3, "set URI to " << uri);
     } else if (oldUri.caseCmp(uri) == 0) {
-        debugs(11, 5, "keep URI as " << oldUri);
+        debugs(11, 5, "kept URI as " << oldUri);
     } else {
         debugs(11, 3, "reset URI from " << oldUri << " to " << uri);
         closeDataConnection();
-        master.clientReadGreeting = false;
         unpinConnection(true); // close control connection to peer
-        changeState(fssBegin, "URI reset");
+        resetLogin("URI reset");
     }
 
     params.cut(eou);
@@ -1271,7 +1283,7 @@ Ftp::Server::handleUserRequest(const String &cmd, String &params)
 bool
 Ftp::Server::handleFeatRequest(String &cmd, String &params)
 {
-    changeState(fssHandleFeat, "ftpHandleFeatRequest");
+    changeState(fssHandleFeat, "handleFeatRequest");
     return true;
 }
 
@@ -1288,7 +1300,7 @@ Ftp::Server::handlePasvRequest(String &cmd, String &params)
         return false;
     }
 
-    changeState(fssHandlePasv, "ftpHandlePasvRequest");
+    changeState(fssHandlePasv, "handlePasvRequest");
     // no need to fake PASV request via setDataCommand() in true PASV case
     return true;
 }
@@ -1354,7 +1366,7 @@ Ftp::Server::handlePortRequest(String &cmd, String &params)
     if (!createDataConnection(cltAddr))
         return false;
 
-    changeState(fssHandlePort, "ftpHandlePortRequest");
+    changeState(fssHandlePort, "handlePortRequest");
     setDataCommand();
     return true; // forward our fake PASV request
 }
@@ -1365,7 +1377,7 @@ Ftp::Server::handleDataRequest(String &cmd, String &params)
     if (!checkDataConnPre())
         return false;
 
-    changeState(fssHandleDataRequest, "ftpHandleDataRequest");
+    changeState(fssHandleDataRequest, "handleDataRequest");
 
     return true;
 }
@@ -1376,7 +1388,7 @@ Ftp::Server::handleUploadRequest(String &cmd, String &params)
     if (!checkDataConnPre())
         return false;
 
-    changeState(fssHandleUploadRequest, "ftpHandleDataRequest");
+    changeState(fssHandleUploadRequest, "handleDataRequest");
 
     return true;
 }
@@ -1405,7 +1417,7 @@ Ftp::Server::handleEprtRequest(String &cmd, String &params)
     if (!createDataConnection(cltAddr))
         return false;
 
-    changeState(fssHandleEprt, "ftpHandleEprtRequest");
+    changeState(fssHandleEprt, "handleEprtRequest");
     setDataCommand();
     return true; // forward our fake PASV request
 }
@@ -1430,7 +1442,7 @@ Ftp::Server::handleEpsvRequest(String &cmd, String &params)
         return false;
     }
 
-    changeState(fssHandleEpsv, "ftpHandleEpsvRequest");
+    changeState(fssHandleEpsv, "handleEpsvRequest");
     setDataCommand();
     return true; // forward our fake PASV request
 }
@@ -1438,21 +1450,21 @@ Ftp::Server::handleEpsvRequest(String &cmd, String &params)
 bool
 Ftp::Server::handleCwdRequest(String &cmd, String &params)
 {
-    changeState(fssHandleCwd, "ftpHandleCwdRequest");
+    changeState(fssHandleCwd, "handleCwdRequest");
     return true;
 }
 
 bool
 Ftp::Server::handlePassRequest(String &cmd, String &params)
 {
-    changeState(fssHandlePass, "ftpHandlePassRequest");
+    changeState(fssHandlePass, "handlePassRequest");
     return true;
 }
 
 bool
 Ftp::Server::handleCdupRequest(String &cmd, String &params)
 {
-    changeState(fssHandleCdup, "ftpHandleCdupRequest");
+    changeState(fssHandleCdup, "handleCdupRequest");
     return true;
 }
 
diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h
index f659895db2..9550c04517 100644
--- a/src/servers/FtpServer.h
+++ b/src/servers/FtpServer.h
@@ -128,6 +128,7 @@ protected:
 private:
     void doProcessRequest();
     void shovelUploadData();
+    void resetLogin(const char *reason);
 
     String uri; ///< a URI reconstructed from various FTP message details
     String host; ///< intended dest. of a transparently intercepted FTP conn
diff --git a/src/tools.cc b/src/tools.cc
index 91f4b2d963..b775b47d51 100644
--- a/src/tools.cc
+++ b/src/tools.cc
@@ -102,7 +102,7 @@ releaseServerSockets(void)
 {
     // Release the main ports as early as possible
 
-    // clear http_port, https_port and ftp_port lists
+    // clear http_port, https_port, and ftp_port lists
     clientConnectionsClose();
 
     // clear icp_port's