From 43446566e3092dc7969e72f22b6c1362a4701784 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 7 Aug 2014 16:33:48 -0600 Subject: [PATCH] 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. --- src/HttpHdrCc.h | 10 +++++--- src/clients/FtpClient.cc | 11 ++------- src/clients/FtpClient.h | 5 ++-- src/clients/FtpGateway.cc | 16 ++++++------- src/clients/FtpRelay.cc | 50 +++++++++++++-------------------------- src/ftp/Elements.cc | 24 +++++++++++++++++++ src/ftp/Elements.h | 6 +++++ src/servers/FtpServer.cc | 28 +++++++--------------- src/servers/FtpServer.h | 2 +- 9 files changed, 75 insertions(+), 77 deletions(-) diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h index a8d40bb1f5..b486be8061 100644 --- 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 e42759665b..0476a4a4db 100644 --- 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 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(data); - ftpState->dataChannelConnected(conn, status, xerrno); -} - bool Ftp::Client::openListenSocket() { diff --git a/src/clients/FtpClient.h b/src/clients/FtpClient.h index 688f282b55..12f2e20f26 100644 --- 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 1edde3ef07..4daf7ae0ab 100644 --- 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 e4f7b7b757..780badeb4f 100644 --- 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 6e053706c8..8570d99cee 100644 --- 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 6d77e86de9..87f74e4a8d 100644 --- 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 37130c2a1c..51e6a4a394 100644 --- 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 ¶ms) 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 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 635321ba53..4f6f8a84af 100644 --- 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 -- 2.47.2