Fixed HttpHdr::Private/NoCache(v) implementations and optimized their API.

These calls now avoid assertions and extra trailing commas when called with
empty names. The API now allows calling with a String() object, but still
needs more polishing work.

Moved common code from Ftp::Server::setReply and Ftp::Relay::createHttpReply()
into Ftp::HttpReplyWrapper().

Also removed the last non-job callbak from Ftp::Client, polished and synced
new FTP comments with the modern client/server/gateway/relay terminology, as
well as minimized changes compared to trunk.

diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h
index a8d40bb1f58a11eaa907712c127fafffdc3341c1..b486be80611386556cdf09980454524ac2ee2707 100644 (file)
--- a/src/HttpHdrCc.h
+++ b/src/HttpHdrCc.h
@@ -75,8 +75,10 @@ public:
     //manipulation for Cache-Control: private header
     bool hasPrivate() const {return isSet(CC_PRIVATE);}
     const String &Private() const {return private_;}
-    void Private(const String &v = "") {
+    void Private(const String &v) {
         setMask(CC_PRIVATE,true);
+        if (!v.size())
+            return;
         // uses append for multi-line headers
         if (private_.size() > 0)
             private_.append(",");
@@ -87,10 +89,12 @@ public:
     //manipulation for Cache-Control: no-cache header
     bool hasNoCache() const {return isSet(CC_NO_CACHE);}
     const String &noCache() const {return no_cache;}
-    void noCache(String &v) {
+    void noCache(const String &v) {
         setMask(CC_NO_CACHE,true);
+        if (!v.size())
+            return;
         // uses append for multi-line headers
-        if (no_cache.size() > 0)
+        if (no_cache.size() > 0 && v.size() > 0)
             no_cache.append(",");
         no_cache.append(v);
     }

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index e42759665b266b9829498c3e33f66fc362c6a27b..0476a4a4db9ce56153e7d146858e9a4ae3c29843 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -730,20 +730,13 @@ Ftp::Client::connectDataChannel()
 
     debugs(9, 3, "connecting to " << conn->remote);
 
-    data.opener = commCbCall(9, 3, "Ftp::Client::dataChannelConnected",
-                             CommConnectCbPtrFun(Ftp::Client::dataChannelConnected, this));
+    typedef CommCbMemFunT<Client, CommConnectCbParams> Dialer;
+    data.opener = JobCallback(9, 3, Dialer, this, Ftp::Client::dataChannelConnected);
     Comm::ConnOpener *cs = new Comm::ConnOpener(conn, data.opener, Config.Timeout.connect);
     cs->setHost(data.host);
     AsyncJob::Start(cs);
 }
 
-void
-Ftp::Client::dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno, void *data)
-{
-    Client *ftpState = static_cast<Client *>(data);
-    ftpState->dataChannelConnected(conn, status, xerrno);
-}
-
 bool
 Ftp::Client::openListenSocket()
 {

diff --git a/src/clients/FtpClient.h b/src/clients/FtpClient.h
index 688f282b55adfc7278c50955ce71d1c59354af87..12f2e20f26344f50eafa124d347af337cc3f5b60 100644 (file)
--- a/src/clients/FtpClient.h
+++ b/src/clients/FtpClient.h
@@ -82,7 +82,7 @@ public:
     bool read_pending;
 };
 
-/// Base class for FTP Gateway and FTP Native client classes.
+/// FTP client functionality shared among FTP Gateway and Relay clients.
 class Client: public ::ServerStateData
 {
 public:
@@ -166,8 +166,7 @@ protected:
     void readControlReply(const CommIoCbParams &io);
     virtual void handleControlReply();
     void writeCommandCallback(const CommIoCbParams &io);
-    static CNCB dataChannelConnected;
-    virtual void dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno) = 0;
+    virtual void dataChannelConnected(const CommConnectCbParams &io) = 0;
     void dataRead(const CommIoCbParams &io);
     void dataComplete();
     AsyncCall::Pointer dataCloser();

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index 1edde3ef07ef3fba6fd39eeab2d4152de755f2d1..4daf7ae0ab5e6ccb833649f4fd4e60e6bc52d6c0 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -179,7 +179,7 @@ public:
     void setCurrentOffset(int64_t offset) { currentOffset = offset; }
     int64_t getCurrentOffset() const { return currentOffset; }
 
-    virtual void dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno);
+    virtual void dataChannelConnected(const CommConnectCbParams &io);
     static PF ftpDataWrite;
     virtual void timeout(const CommTimeoutCbParams &io);
     void ftpAcceptDataConnection(const CommAcceptCbParams &io);
@@ -972,11 +972,11 @@ Ftp::Gateway::parseListing()
     line = (char *)memAllocate(MEM_4K_BUF);
     ++end;
     s = sbuf;
-    s += strspn(s, Ftp::crlf);
+    s += strspn(s, crlf);
 
-    for (; s < end; s += strcspn(s, Ftp::crlf), s += strspn(s, Ftp::crlf)) {
+    for (; s < end; s += strcspn(s, crlf), s += strspn(s, crlf)) {
         debugs(9, 7, HERE << "s = {" << s << "}");
-        linelen = strcspn(s, Ftp::crlf) + 1;
+        linelen = strcspn(s, crlf) + 1;
 
         if (linelen < 2)
             break;
@@ -1831,16 +1831,16 @@ ftpReadPasv(Ftp::Gateway * ftpState)
 }
 
 void
-Ftp::Gateway::dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno)
+Ftp::Gateway::dataChannelConnected(const CommConnectCbParams &io)
 {
     debugs(9, 3, HERE);
     data.opener = NULL;
 
-    if (err != Comm::OK) {
+    if (io.flag != Comm::OK) {
         debugs(9, 2, HERE << "Failed to connect. Retrying via another method.");
 
         // ABORT on timeouts. server may be waiting on a broken TCP link.
-        if (err == Comm::TIMEOUT)
+        if (io.xerrno == Comm::TIMEOUT)
             writeCommand("ABOR");
 
         // try another connection attempt with some other method
@@ -1848,7 +1848,7 @@ Ftp::Gateway::dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Fl
         return;
     }
 
-    data.opened(conn, dataCloser());
+    data.opened(io.conn, dataCloser());
     ftpRestOrList(this);
 }
 

diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index e4f7b7b75734637917cd6bd90b982eb505fbed8a..780badeb4f7a57ae832f2b1725b3ed75e4037e3d 100644 (file)
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -9,6 +9,7 @@
 #include "client_side.h"
 #include "clients/forward.h"
 #include "clients/FtpClient.h"
+#include "ftp/Elements.h"
 #include "ftp/Parsing.h"
 #include "HttpHdrCc.h"
 #include "HttpRequest.h"
@@ -38,7 +39,7 @@ protected:
 
     /* 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);
+    virtual void dataChannelConnected(const CommConnectCbParams &io);
 
     /* ServerStateData API */
     virtual void serverComplete();
@@ -54,7 +55,7 @@ protected:
     void forwardReply();
     void forwardError(err_type error = ERR_NONE, int xerrno = 0);
     void failedErrorMessage(err_type error, int xerrno);
-    HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int clen = 0);
+    HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int64_t clen = 0);
     void handleDataRequest();
     void startDataDownload();
     void startDataUpload();
@@ -386,31 +387,14 @@ Ftp::Relay::forwardError(err_type error, int xerrno)
 }
 
 HttpReply *
-Ftp::Relay::createHttpReply(const Http::StatusCode httpStatus, const int clen)
-{
-    HttpReply *const reply = new HttpReply;
-    reply->sline.set(Http::ProtocolVersion(1, 1), httpStatus);
-    HttpHeader &header = reply->header;
-    header.putTime(HDR_DATE, squid_curtime);
-    {
-        HttpHdrCc cc;
-        cc.Private();
-        header.putCc(&cc);
-    }
-    if (clen >= 0)
-        header.putInt64(HDR_CONTENT_LENGTH, clen);
-
+Ftp::Relay::createHttpReply(const Http::StatusCode httpStatus, const int64_t clen)
+{
+    HttpReply *const reply = Ftp::HttpReplyWrapper(ctrl.replycode, ctrl.last_reply, httpStatus, clen);
     if (ctrl.message) {
         for (wordlist *W = ctrl.message; W && W->next; W = W->next)
-            header.putStr(HDR_FTP_PRE, httpHeaderQuoteString(W->key).c_str());
+            reply->header.putStr(HDR_FTP_PRE, httpHeaderQuoteString(W->key).c_str());
+        // no hdrCacheInit() is needed for after HDR_FTP_PRE addition
     }
-    if (ctrl.replycode > 0)
-        header.putInt(HDR_FTP_STATUS, ctrl.replycode);
-    if (ctrl.last_reply)
-        header.putStr(HDR_FTP_REASON, ctrl.last_reply);
-
-    reply->hdrCacheInit();
-
     return reply;
 }
 
@@ -465,9 +449,9 @@ Ftp::Relay::readGreeting()
         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
-        // confuse a client that has changed the gateway destination mid-air.
+        // Do not forward server greeting to the user because our FTP Server
+        // has greeted the user already. Also, an original origin greeting may
+        // confuse a user that has changed the origin mid-air.
 
         start();
         break;
@@ -486,7 +470,7 @@ void
 Ftp::Relay::sendCommand()
 {
     if (!fwd->request->header.has(HDR_FTP_COMMAND)) {
-        abortTransaction("Internal error: FTP gateway request with no command");
+        abortTransaction("Internal error: FTP relay request with no command");
         return;
     }
 
@@ -686,20 +670,20 @@ Ftp::Relay::readTransferDoneReply()
 }
 
 void
-Ftp::Relay::dataChannelConnected(const Comm::ConnectionPointer &conn, Comm::Flag err, int xerrno)
+Ftp::Relay::dataChannelConnected(const CommConnectCbParams &io)
 {
     debugs(9, 3, status());
     data.opener = NULL;
 
-    if (err != Comm::OK) {
+    if (io.flag != Comm::OK) {
         debugs(9, 2, "failed to connect FTP server data channel");
-        forwardError(ERR_CONNECT_FAIL, xerrno);
+        forwardError(ERR_CONNECT_FAIL, io.xerrno);
         return;
     }
 
-    debugs(9, 2, "connected FTP server data channel: " << conn);
+    debugs(9, 2, "connected FTP server data channel: " << io.conn);
 
-    data.opened(conn, dataCloser());
+    data.opened(io.conn, dataCloser());
 
     sendCommand();
 }

diff --git a/src/ftp/Elements.cc b/src/ftp/Elements.cc
index 6e053706c892073e767a666a0715a7f0cbcbc859..8570d99ceec7a1124dcb82584b258956f7aaa720 100644 (file)
--- a/src/ftp/Elements.cc
+++ b/src/ftp/Elements.cc
@@ -4,8 +4,32 @@
 
 #include "squid.h"
 #include "ftp/Elements.h"
+#include "HttpHdrCc.h"
+#include "HttpReply.h"
 #include "SBuf.h"
 
+HttpReply *
+Ftp::HttpReplyWrapper(const int ftpStatus, const char *ftpReason, const Http::StatusCode httpStatus, const int64_t clen)
+{
+    HttpReply *const reply = new HttpReply;
+    reply->sline.set(Http::ProtocolVersion(1, 1), httpStatus);
+    HttpHeader &header = reply->header;
+    header.putTime(HDR_DATE, squid_curtime);
+    {
+        HttpHdrCc cc;
+        cc.Private(String());
+        header.putCc(&cc);
+    }
+    if (ftpStatus > 0)
+        header.putInt(HDR_FTP_STATUS, ftpStatus);
+    if (ftpReason)
+        header.putStr(HDR_FTP_REASON, ftpReason);
+    if (clen >= 0)
+        header.putInt64(HDR_CONTENT_LENGTH, clen);
+    reply->hdrCacheInit();
+    return reply;
+}
+
 const SBuf &
 Ftp::cmdAppe()
 {

diff --git a/src/ftp/Elements.h b/src/ftp/Elements.h
index 6d77e86de909250e0bfe2f8cb4d99dcb768a30bc..87f74e4a8d920f10a186cf7239992b0817290054 100644 (file)
--- a/src/ftp/Elements.h
+++ b/src/ftp/Elements.h
@@ -1,10 +1,16 @@
 #ifndef SQUID_FTP_ELEMENTS_H
 #define SQUID_FTP_ELEMENTS_H
 
+#include "http/StatusCode.h"
+
 class SBuf;
+class HttpReply;
 
 namespace Ftp {
 
+/// Create an internal HttpReply structure to house FTP control response info.
+HttpReply *HttpReplyWrapper(const int ftpStatus, const char *ftpReason, const Http::StatusCode httpStatus, const int64_t clen);
+
 /* FTP Commands used by Squid. ALLCAPS case. Safe for static initializaton. */
 const SBuf &cmdAppe();
 const SBuf &cmdAuth();

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 37130c2a1c57d65898220a2e2a476b07b014420f..51e6a4a3942ebfefa1acee5593a3e90042a7c44e 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -306,8 +306,8 @@ Ftp::Server::clientPinnedConnectionClosed(const CommCloseCbParams &io)
     // if the server control connection is gone, reset state to login again
     resetLogin("control connection closure");
 
-    // XXX: Not enough. Gateway::ServerStateData::sendCommand() will not
-    // re-login because clientState() is not ConnStateData::FTP_CONNECTED.
+    // XXX: Reseting is not enough. FtpRelay::sendCommand() will not re-login
+    // because FtpRelay::serverState() is not going to be fssConnected.
 }
 
 /// clear client and server login-related state after the old login is gone
@@ -1278,7 +1278,7 @@ Ftp::Server::handleUserRequest(const SBuf &cmd, SBuf &params)
     if (master.clientReadGreeting)
         oldUri = uri;
 
-    master.workingDir = NULL;
+    master.workingDir.clear();
     calcUri(NULL);
 
     if (!master.clientReadGreeting) {
@@ -1574,19 +1574,7 @@ Ftp::Server::setReply(const int code, const char *msg)
     assert(http != NULL);
     assert(http->storeEntry() == NULL);
 
-    HttpReply *const reply = new HttpReply;
-    reply->sline.set(Http::ProtocolVersion(1, 1), Http::scNoContent);
-    HttpHeader &header = reply->header;
-    header.putTime(HDR_DATE, squid_curtime);
-    {
-        HttpHdrCc cc;
-        cc.Private();
-        header.putCc(&cc);
-    }
-    header.putInt64(HDR_CONTENT_LENGTH, 0);
-    header.putInt(HDR_FTP_STATUS, code);
-    header.putStr(HDR_FTP_REASON, msg);
-    reply->hdrCacheInit();
+    HttpReply *const reply = Ftp::HttpReplyWrapper(code, msg, Http::scNoContent, 0);
 
     setLogUri(http, urlCanonicalClean(http->request));
 
@@ -1602,16 +1590,16 @@ Ftp::Server::setReply(const int code, const char *msg)
     http->storeEntry()->replaceHttpReply(reply);
 }
 
-/// Whether Squid FTP gateway supports a given feature (e.g., a command).
+/// Whether Squid FTP Relay supports a named feature (e.g., a command).
 static bool
 Ftp::SupportedCommand(const SBuf &name)
 {
     static std::set<SBuf> BlackList;
     if (BlackList.empty()) {
-        /* Add FTP commands that Squid cannot gateway correctly */
+        /* Add FTP commands that Squid cannot relay correctly. */
 
-        // we probably do not support AUTH TLS.* and AUTH SSL,
-        // but let's disclaim all AUTH support to KISS, for now
+        // We probably do not support AUTH TLS.* and AUTH SSL,
+        // but let's disclaim all AUTH support to KISS, for now.
         BlackList.insert(cmdAuth());
     }
 

diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h
index 635321ba534b9d2ba61ef78c638e7e99b0d9213f..4f6f8a84af3bcd72f95561d1f0c99459ead69457 100644 (file)
--- a/src/servers/FtpServer.h
+++ b/src/servers/FtpServer.h
@@ -32,7 +32,7 @@ class MasterState
 {
 public:
     Ip::Address clientDataAddr; ///< address of our FTP client data connection
-    SBuf workingDir;
+    SBuf workingDir; ///< estimated current working directory for URI formation
     ServerState serverState; ///< what our FTP server is doing
     bool clientReadGreeting; ///< whether our FTP client read their FTP server greeting