Revert HttpMsg::parse API from MemBuf to c-string

It is simpler and more performant to incrementally convert the I/O
buffers to SBuf using c-string export to legacy code than MemBuf.

HttpMsg::parse API did not really make much use of MemBuf, so we can
remove that and avoid performance regressions later in ICAP I/O buffer
conversion.

diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc
index d995e3c5f3b7448ad68fc55a79761ab4cfe2f954..b69c044a1c26cdb5f7ad03572ff96fbe87dcbfc7 100644 (file)
--- a/src/HttpMsg.cc
+++ b/src/HttpMsg.cc
@@ -125,16 +125,13 @@ httpMsgIsolateStart(const char **parse_start, const char **blk_start, const char
 // zero return means need more data
 // positive return is the size of parsed headers
 bool
-HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error)
+HttpMsg::parse(const char *buf, const size_t sz, bool eof, Http::StatusCode *error)
 {
     assert(error);
     *error = Http::scNone;
 
-    // httpMsgParseStep() and debugging require 0-termination, unfortunately
-    buf->terminate(); // does not affect content size
-
     // find the end of headers
-    const size_t hdr_len = headersEnd(buf->content(), buf->contentSize());
+    const size_t hdr_len = headersEnd(buf, sz);
 
     // sanity check the start line to see if this is in fact an HTTP message
     if (!sanityCheckStartLine(buf, hdr_len, error)) {
@@ -146,15 +143,14 @@ HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error)
         return false;
     }
 
-    // TODO: move to httpReplyParseStep()
-    if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) {
+    if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && sz > Config.maxReplyHeaderSize)) {
         debugs(58, DBG_IMPORTANT, "HttpMsg::parse: Too large reply header (" << hdr_len << " > " << Config.maxReplyHeaderSize);
         *error = Http::scHeaderTooLarge;
         return false;
     }
 
     if (hdr_len <= 0) {
-        debugs(58, 3, "HttpMsg::parse: failed to find end of headers (eof: " << eof << ") in '" << buf->content() << "'");
+        debugs(58, 3, "HttpMsg::parse: failed to find end of headers (eof: " << eof << ") in '" << buf << "'");
 
         if (eof) // iff we have seen the end, this is an error
             *error = Http::scInvalidHeader;
@@ -162,22 +158,22 @@ HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error)
         return false;
     }
 
-    const int res = httpMsgParseStep(buf->content(), buf->contentSize(), eof);
+    const int res = httpMsgParseStep(buf, sz, eof);
 
     if (res < 0) { // error
-        debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers in '" << buf->content() << "'");
+        debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers in '" << buf << "'");
         *error = Http::scInvalidHeader;
         return false;
     }
 
     if (res == 0) {
-        debugs(58, 2, "HttpMsg::parse: strange, need more data near '" << buf->content() << "'");
+        debugs(58, 2, "HttpMsg::parse: strange, need more data near '" << buf << "'");
         *error = Http::scInvalidHeader;
         return false; // but this should not happen due to headersEnd() above
     }
 
     assert(res > 0);
-    debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) near '" << buf->content() << "'");
+    debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) near '" << buf << "'");
 
     if (hdr_sz != (int)hdr_len) {
         debugs(58, DBG_IMPORTANT, "internal HttpMsg::parse vs. headersEnd error: " <<

diff --git a/src/HttpMsg.h b/src/HttpMsg.h
index 309a82972de73257d72e6476320e1218f111d4f6..d85aa795b8445ea97c60d9824dd4188485a8d0f4 100644 (file)
--- a/src/HttpMsg.h
+++ b/src/HttpMsg.h
@@ -67,7 +67,7 @@ public:
     // returns true and sets hdr_sz on success
     // returns false and sets *error to zero when needs more data
     // returns false and sets *error to a positive Http::StatusCode on error
-    bool parse(MemBuf *buf, bool eol, Http::StatusCode *error);
+    bool parse(const char *buf, const size_t sz, bool eol, Http::StatusCode *error);
 
     bool parseCharBuf(const char *buf, ssize_t end);
 
@@ -89,7 +89,7 @@ protected:
      * \retval true   Status line has no serious problems.
      * \retval false  Status line has a serious problem. Correct response is indicated by error.
      */
-    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) = 0;
+    virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) = 0;
 
     virtual void packFirstLineInto(Packer * p, bool full_uri) const = 0;
 

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index d403616e4dfe0ebadda4d9baaa918ccaac66353f..3c12fcb08d7b4ba4d57e504dabd3d62ae45adf97 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -411,15 +411,15 @@ HttpReply::bodySize(const HttpRequestMethod& method) const
  * NP: not all error cases are detected yet. Some are left for detection later in parse.
  */
 bool
-HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error)
+HttpReply::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error)
 {
     // hack warning: using psize instead of size here due to type mismatches with MemBuf.
 
     // content is long enough to possibly hold a reply
     // 4 being magic size of a 3-digit number plus space delimiter
-    if ( buf->contentSize() < (protoPrefix.psize() + 4) ) {
+    if (hdr_len < (size_t)(protoPrefix.psize() + 4)) {
         if (hdr_len > 0) {
-            debugs(58, 3, HERE << "Too small reply header (" << hdr_len << " bytes)");
+            debugs(58, 3, "Too small reply header (" << hdr_len << " bytes)");
             *error = Http::scInvalidHeader;
         }
         return false;
@@ -428,13 +428,13 @@ HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusC
     int pos;
     // catch missing or mismatched protocol identifier
     // allow special-case for ICY protocol (non-HTTP identifier) in response to faked HTTP request.
-    if (strncmp(buf->content(), "ICY", 3) == 0) {
+    if (strncmp(buf, "ICY", 3) == 0) {
         protoPrefix = "ICY";
         pos = protoPrefix.psize();
     } else {
 
-        if (protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
-            debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol prefix (" << protoPrefix << ") in '" << buf->content() << "'");
+        if (protoPrefix.cmp(buf, protoPrefix.size()) != 0) {
+            debugs(58, 3, "missing protocol prefix (" << protoPrefix << ") in '" << buf << "'");
             *error = Http::scInvalidHeader;
             return false;
         }
@@ -443,21 +443,21 @@ HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusC
         pos = protoPrefix.psize();
 
         // skip arbitrary number of digits and a dot in the verion portion
-        while ( pos <= buf->contentSize() && (*(buf->content()+pos) == '.' || xisdigit(*(buf->content()+pos)) ) ) ++pos;
+        while ((size_t)pos <= hdr_len && (*(buf+pos) == '.' || xisdigit(*(buf+pos)) ) ) ++pos;
 
         // catch missing version info
         if (pos == protoPrefix.psize()) {
-            debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol version numbers (ie. " << protoPrefix << "/1.0) in '" << buf->content() << "'");
+            debugs(58, 3, "missing protocol version numbers (ie. " << protoPrefix << "/1.0) in '" << buf << "'");
             *error = Http::scInvalidHeader;
             return false;
         }
     }
 
     // skip arbitrary number of spaces...
-    while (pos <= buf->contentSize() && (char)*(buf->content()+pos) == ' ') ++pos;
+    while ((size_t)pos <= hdr_len && (char)*(buf+pos) == ' ') ++pos;
 
-    if (pos < buf->contentSize() && !xisdigit(*(buf->content()+pos))) {
-        debugs(58, 3, "HttpReply::sanityCheckStartLine: missing or invalid status number in '" << buf->content() << "'");
+    if ((size_t)pos < hdr_len && !xisdigit(*(buf+pos))) {
+        debugs(58, 3, "missing or invalid status number in '" << buf << "'");
         *error = Http::scInvalidHeader;
         return false;
     }

diff --git a/src/HttpReply.h b/src/HttpReply.h
index 05b51a346f013fe11d0702dfbc10153bcb9e0836..95aa93029b434531241d1438eba38bf864a7528a 100644 (file)
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -39,7 +39,7 @@ public:
      \retval false and sets *error to zero when needs more data
      \retval false and sets *error to a positive Http::StatusCode on error
      */
-    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error);
+    virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error);
 
     /** \par public, readable; never update these or their .hdr equivalents directly */
     time_t date;

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 459cae8558d79446ffb9fdf97e4b244c5631d771..4b4ea77af474d176aadcc9e5195080768a12ebb7 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -269,11 +269,11 @@ HttpRequest::inheritProperties(const HttpMsg *aMsg)
  * NP: Other errors are left for detection later in the parse.
  */
 bool
-HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error)
+HttpRequest::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error)
 {
     // content is long enough to possibly hold a reply
     // 2 being magic size of a 1-byte request method plus space delimiter
-    if ( buf->contentSize() < 2 ) {
+    if (hdr_len < 2) {
         // this is ony a real error if the headers apparently complete.
         if (hdr_len > 0) {
             debugs(58, 3, HERE << "Too large request header (" << hdr_len << " bytes)");
@@ -284,7 +284,7 @@ HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::Statu
 
     /* See if the request buffer starts with a non-whitespace HTTP request 'method'. */
     HttpRequestMethod m;
-    m.HttpRequestMethodXXX(buf->content());
+    m.HttpRequestMethodXXX(buf);
     if (m == Http::METHOD_NONE) {
         debugs(73, 3, "HttpRequest::sanityCheckStartLine: did not find HTTP request method");
         *error = Http::scInvalidHeader;

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 73f30653632ff017941a2818a4bb9cc03e14be8a..348e19825ac328e298927d9000c811d675a21951 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -257,7 +257,7 @@ private:
 protected:
     virtual void packFirstLineInto(Packer * p, bool full_uri) const;
 
-    virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error);
+    virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error);
 
     virtual void hdrCacheInit();
 

diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc
index d8a3b4106c381c74ff522c9df5a520ee552d74c1..a8375a7ee68a8be8235eb0be743679254f97e247 100644 (file)
--- a/src/adaptation/icap/ModXact.cc
+++ b/src/adaptation/icap/ModXact.cc
@@ -557,10 +557,10 @@ void Adaptation::Icap::ModXact::readMore()
         return;
     }
 
-    if (readBuf.hasSpace())
+    if (readBuf.spaceSize())
         scheduleRead();
     else
-        debugs(93,3,HERE << "nothing to do because !readBuf.hasSpace()");
+        debugs(93,3,HERE << "nothing to do because !readBuf.spaceSize()");
 }
 
 // comm module read a portion of the ICAP response for us
@@ -649,9 +649,8 @@ void Adaptation::Icap::ModXact::checkConsuming()
 
 void Adaptation::Icap::ModXact::parseMore()
 {
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" <<
-           status());
-    debugs(93, 5, HERE << "\n" << readBuf.content());
+    debugs(93, 5, "have " << readBuf.length() << " bytes to parse" << status());
+    debugs(93, 5, "\n" << readBuf);
 
     if (state.parsingHeaders())
         parseHeaders();
@@ -967,7 +966,8 @@ void Adaptation::Icap::ModXact::prepEchoing()
     // parse the buffer back
     Http::StatusCode error = Http::scNone;
 
-    Must(adapted.header->parse(&httpBuf, true, &error));
+    httpBuf.terminate(); // HttpMsg::parse requires nil-terminated buffer
+    Must(adapted.header->parse(httpBuf.content(), httpBuf.contentSize(), true, &error));
 
     if (HttpRequest *r = dynamic_cast<HttpRequest*>(adapted.header))
         urlCanonical(r); // parse does not set HttpRequest::canonical
@@ -1075,11 +1075,13 @@ void Adaptation::Icap::ModXact::parseHttpHead()
 bool Adaptation::Icap::ModXact::parseHead(HttpMsg *head)
 {
     Must(head);
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " head bytes to parse" <<
-           "; state: " << state.parsing);
+    debugs(93, 5, "have " << readBuf.length() << " head bytes to parse; state: " << state.parsing);
 
     Http::StatusCode error = Http::scNone;
-    const bool parsed = head->parse(&readBuf, commEof, &error);
+    // XXX: performance regression. c_str() data copies
+    // XXX: HttpMsg::parse requires a terminated string buffer
+    const char *tmpBuf = readBuf.c_str();
+    const bool parsed = head->parse(tmpBuf, readBuf.length(), commEof, &error);
     Must(parsed || !error); // success or need more data
 
     if (!parsed) { // need more data
@@ -1117,15 +1119,22 @@ void Adaptation::Icap::ModXact::parseBody()
     Must(state.parsing == State::psBody);
     Must(bodyParser);
 
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes to parse");
+    debugs(93, 5, "have " << readBuf.length() << " body bytes to parse");
 
     // the parser will throw on errors
     BodyPipeCheckout bpc(*adapted.body_pipe);
-    const bool parsed = bodyParser->parse(&readBuf, &bpc.buf);
+    // XXX: performance regression. SBuf-convert (or Parser-convert?) the chunked decoder.
+    MemBuf encodedData;
+    encodedData.init();
+    // NP: we must do this instead of pointing encodedData at the SBuf::rawContent
+    // because chunked decoder uses MemBuf::consume, which shuffles buffer bytes around.
+    encodedData.append(readBuf.rawContent(), readBuf.length());
+    const bool parsed = bodyParser->parse(&encodedData, &bpc.buf);
+    // XXX: httpChunkDecoder has consumed from MemBuf.
+    readBuf.consume(readBuf.length() - encodedData.contentSize());
     bpc.checkIn();
 
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes after " <<
-           "parse; parsed all: " << parsed);
+    debugs(93, 5, "have " << readBuf.length() << " body bytes after parsed all: " << parsed);
     replyHttpBodySize += adapted.body_pipe->buf().contentSize();
 
     // TODO: expose BodyPipe::putSize() to make this check simpler and clearer

diff --git a/src/adaptation/icap/OptXact.cc b/src/adaptation/icap/OptXact.cc
index 103b2968fbc94e23ab5c703ac223167b2689637e..5aa1225441b0e82de2110a4c757b7b1cbc2378b2 100644 (file)
--- a/src/adaptation/icap/OptXact.cc
+++ b/src/adaptation/icap/OptXact.cc
@@ -67,7 +67,8 @@ void Adaptation::Icap::OptXact::makeRequest(MemBuf &buf)
 
     // XXX: HttpRequest cannot fully parse ICAP Request-Line
     Http::StatusCode reqStatus;
-    Must(icapRequest->parse(&buf, true, &reqStatus) > 0);
+    buf.terminate(); // HttpMsg::parse requires terminated buffer
+    Must(icapRequest->parse(buf.content(), buf.contentSize(), true, &reqStatus) > 0);
 }
 
 void Adaptation::Icap::OptXact::handleCommWrote(size_t size)
@@ -99,9 +100,8 @@ void Adaptation::Icap::OptXact::handleCommRead(size_t)
 
 bool Adaptation::Icap::OptXact::parseResponse()
 {
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" <<
-           status());
-    debugs(93, 5, HERE << "\n" << readBuf.content());
+    debugs(93, 5, "have " << readBuf.length() << " bytes to parse" << status());
+    debugs(93, DBG_DATA, "\n" << readBuf);
 
     HttpReply::Pointer r(new HttpReply);
     r->protoPrefix = "ICAP/"; // TODO: make an IcapReply class?

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 8dee87fad138d63970bd036cbd61dfc6e91d4e74..6485ac2f3bf56706d262bc797da99c62c7c74996 100644 (file)
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -39,8 +39,6 @@ Adaptation::Icap::Xaction::Xaction(const char *aTypeName, Adaptation::Icap::Serv
     attempts(0),
     connection(NULL),
     theService(aService),
-    commBuf(NULL),
-    commBufSize(0),
     commEof(false),
     reuseConnection(true),
     isRetriable(true),
@@ -95,11 +93,6 @@ void Adaptation::Icap::Xaction::disableRepeats(const char *reason)
 void Adaptation::Icap::Xaction::start()
 {
     Adaptation::Initiate::start();
-
-    readBuf.init(SQUID_TCP_SO_RCVBUF, SQUID_TCP_SO_RCVBUF);
-    commBuf = (char*)memAllocBuf(SQUID_TCP_SO_RCVBUF, &commBufSize);
-    // make sure maximum readBuf space does not exceed commBuf size
-    Must(static_cast<size_t>(readBuf.potentialSpaceSize()) <= commBufSize);
 }
 
 static void
@@ -383,17 +376,14 @@ void Adaptation::Icap::Xaction::scheduleRead()
 {
     Must(haveConnection());
     Must(!reader);
-    Must(readBuf.hasSpace());
 
-    /*
-     * See comments in Adaptation::Icap::Xaction.h about why we use commBuf
-     * here instead of reading directly into readBuf.buf.
-     */
-    typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommIoCbParams> Dialer;
-    reader = JobCallback(93, 3,
-                         Dialer, this, Adaptation::Icap::Xaction::noteCommRead);
+    // TODO: tune this better to expected message sizes
+    readBuf.reserveCapacity(SQUID_TCP_SO_RCVBUF);
+    Must(readBuf.spaceSize());
 
-    comm_read(connection, commBuf, readBuf.spaceSize(), reader);
+    typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommIoCbParams> Dialer;
+    reader = JobCallback(93, 3, Dialer, this, Adaptation::Icap::Xaction::noteCommRead);
+    Comm::Read(connection, reader);
     updateTimeout();
 }
 
@@ -405,7 +395,30 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io)
 
     Must(io.flag == Comm::OK);
 
-    if (!io.size) {
+    CommIoCbParams rd(this); // will be expanded with ReadNow results
+    rd.conn = io.conn;
+    rd.size = readBuf.spaceSize();
+
+    switch (Comm::ReadNow(rd, readBuf)) { // XXX: SBuf convert readBuf
+    case Comm::INPROGRESS:
+        if (readBuf.isEmpty())
+            debugs(33, 2, io.conn << ": no data to process, " << xstrerr(rd.xerrno));
+        scheduleRead();
+        return;
+
+    case Comm::OK:
+        al.icap.bytesRead += rd.size;
+
+        updateTimeout();
+
+        debugs(93, 3, "read " << rd.size << " bytes");
+
+        disableRetries(); // because pconn did not fail
+
+        /* Continue to process previously read data */
+        break;
+
+    case Comm::ENDFILE: // close detected by 0-byte read
         commEof = true;
         reuseConnection = false;
 
@@ -415,21 +428,14 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io)
             mustStop("pconn race");
             return;
         }
-    } else {
-
-        al.icap.bytesRead+=io.size;
-
-        updateTimeout();
-
-        debugs(93, 3, HERE << "read " << io.size << " bytes");
 
-        /*
-         * See comments in Adaptation::Icap::Xaction.h about why we use commBuf
-         * here instead of reading directly into readBuf.buf.
-         */
+        break;
 
-        readBuf.append(commBuf, io.size);
-        disableRetries(); // because pconn did not fail
+        // case Comm::COMM_ERROR:
+    default: // no other flags should ever occur
+        debugs(11, 2, io.conn << ": read failure: " << xstrerr(rd.xerrno));
+        mustStop("unknown ICAP I/O read error");
+        return;
     }
 
     handleCommRead(io.size);
@@ -446,10 +452,12 @@ void Adaptation::Icap::Xaction::cancelRead()
 
 bool Adaptation::Icap::Xaction::parseHttpMsg(HttpMsg *msg)
 {
-    debugs(93, 5, HERE << "have " << readBuf.contentSize() << " head bytes to parse");
+    debugs(93, 5, "have " << readBuf.length() << " head bytes to parse");
 
     Http::StatusCode error = Http::scNone;
-    const bool parsed = msg->parse(&readBuf, commEof, &error);
+    // XXX: performance regression c_str() data copies
+    const char *buf = readBuf.c_str();
+    const bool parsed = msg->parse(buf, readBuf.length(), commEof, &error);
     Must(parsed || !error); // success or need more data
 
     if (!parsed) {  // need more data
@@ -465,7 +473,7 @@ bool Adaptation::Icap::Xaction::parseHttpMsg(HttpMsg *msg)
 bool Adaptation::Icap::Xaction::mayReadMore() const
 {
     return !doneReading() && // will read more data
-           readBuf.hasSpace();  // have space for more data
+           readBuf.spaceSize();  // have space for more data
 }
 
 bool Adaptation::Icap::Xaction::doneReading() const
@@ -530,11 +538,7 @@ void Adaptation::Icap::Xaction::swanSong()
 
     closeConnection(); // TODO: rename because we do not always close
 
-    if (!readBuf.isNull())
-        readBuf.clean();
-
-    if (commBuf)
-        memFreeBuf(commBufSize, commBuf);
+    readBuf.clear();
 
     tellQueryAborted();
 

diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h
index 644449521c94dc5f7d2f18ba82484ae1291b4dba..d5b812f6b64445b3a202681a380371aea1112e27 100644 (file)
--- a/src/adaptation/icap/Xaction.h
+++ b/src/adaptation/icap/Xaction.h
@@ -16,7 +16,9 @@
 #include "CommCalls.h"
 #include "HttpReply.h"
 #include "ipcache.h"
-#include "MemBuf.h"
+#include "SBuf.h"
+
+class MemBuf; // XXX: may not be needed when we are done SBuf'ing
 
 namespace Adaptation
 {
@@ -127,20 +129,7 @@ protected:
     Comm::ConnectionPointer connection;     ///< ICAP server connection
     Adaptation::Icap::ServiceRep::Pointer theService;
 
-    /*
-     * We have two read buffers.   We would prefer to read directly
-     * into the MemBuf, but since comm_read isn't MemBuf-aware, and
-     * uses event-delayed callbacks, it leaves the MemBuf in an
-     * inconsistent state.  There would be data in the buffer, but
-     * MemBuf.size won't be updated until the (delayed) callback
-     * occurs.   To avoid that situation we use a plain buffer
-     * (commBuf) and then copy (append) its contents to readBuf in
-     * the callback.  If comm_read ever becomes MemBuf-aware, we
-     * can eliminate commBuf and this extra buffer copy.
-     */
-    MemBuf readBuf;
-    char *commBuf;
-    size_t commBufSize;
+    SBuf readBuf;
     bool commEof;
     bool reuseConnection;
     bool isRetriable;  ///< can retry on persistent connection failures

diff --git a/src/http.cc b/src/http.cc
index 1757073d7f9faf47f88f068b85033a60c9a332fd..665b5057144cbfce98b7024c1d18737814b8820b 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -705,7 +705,8 @@ HttpStateData::processReplyHeader()
     Http::StatusCode error = Http::scNone;
 
     HttpReply *newrep = new HttpReply;
-    const bool parsed = newrep->parse(readBuf, eof, &error);
+    readBuf->terminate(); // XXX: HttpMsg::parse requires terminated string
+    const bool parsed = newrep->parse(readBuf->content(), readBuf->contentSize(), eof, &error);
 
     if (!parsed && readBuf->contentSize() > 5 && strncmp(readBuf->content(), "HTTP/", 5) != 0 && strncmp(readBuf->content(), "ICY", 3) != 0) {
         MemBuf *mb;
@@ -713,7 +714,8 @@ HttpStateData::processReplyHeader()
         tmprep->setHeaders(Http::scOkay, "Gatewaying", NULL, -1, -1, -1);
         tmprep->header.putExt("X-Transformed-From", "HTTP/0.9");
         mb = tmprep->pack();
-        newrep->parse(mb, eof, &error);
+        mb->terminate(); // XXX: HttpMsg::parse requires terminated string
+        newrep->parse(mb->content(), mb->contentSize(), eof, &error);
         delete mb;
         delete tmprep;
     } else {

diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc
index d1d1a77dc5cc1078ab51237b0de35a9c04339d40..8ae8d38863e5838d7efa09af6b38468724c95932 100644 (file)
--- a/src/tests/stub_HttpReply.cc
+++ b/src/tests/stub_HttpReply.cc
@@ -21,7 +21,7 @@ HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0),
     void HttpReply::packHeadersInto(Packer * p) const STUB
     void HttpReply::reset() STUB
     void httpBodyPackInto(const HttpBody * body, Packer * p) STUB
-    bool HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false)
+    bool HttpReply::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false)
     int HttpReply::httpMsgParseError() STUB_RETVAL(0)
     bool HttpReply::expectingBody(const HttpRequestMethod&, int64_t&) const STUB_RETVAL(false)
     bool HttpReply::parseFirstLine(const char *start, const char *end) STUB_RETVAL(false)

diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc
index fcaf21d62b92e298292b571daac08a6f078413b3..62ff173da5105e4860561fad6f0bfa85d1d13920 100644 (file)
--- a/src/tests/stub_HttpRequest.cc
+++ b/src/tests/stub_HttpRequest.cc
@@ -17,7 +17,7 @@ HttpRequest::HttpRequest() : HttpMsg(hoRequest) STUB
     HttpRequest::HttpRequest(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) : HttpMsg(hoRequest) STUB
     HttpRequest::~HttpRequest() STUB
     void HttpRequest::packFirstLineInto(Packer * p, bool full_uri) const STUB
-    bool HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false)
+    bool HttpRequest::sanityCheckStartLine(const char*buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false)
     void HttpRequest::hdrCacheInit() STUB
     void HttpRequest::reset() STUB
     bool HttpRequest::expectingBody(const HttpRequestMethod& unused, int64_t&) const STUB_RETVAL(false)

diff --git a/src/tests/testHttpReply.cc b/src/tests/testHttpReply.cc
index a0259090369244bddac9152136d7bf334d679905..8fcd9d6d293781471affae78b693d800cabfc446 100644 (file)
--- a/src/tests/testHttpReply.cc
+++ b/src/tests/testHttpReply.cc
@@ -50,14 +50,14 @@ testHttpReply::testSanityCheckFirstLine()
     // a valid status line
     input.append("HTTP/1.1 200 Okay\n\n", 19);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 1 && engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 1 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1    200  Okay     \n\n", 28);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 2 && engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 2 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -66,14 +66,14 @@ testHttpReply::testSanityCheckFirstLine()
     // invalid status line
     input.append("HTTP/1.1 999 Okay\n\n", 19);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 3 && !engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 3 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1    2000  Okay     \n\n", 29);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 4 && engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 4 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -82,7 +82,7 @@ testHttpReply::testSanityCheckFirstLine()
     // valid ICY protocol status line
     input.append("ICY 200 Okay\n\n", 14);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -93,14 +93,14 @@ testHttpReply::testSanityCheckFirstLine()
     // empty status line
     input.append("\n\n", 2);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 5 && !engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 5 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
 
     input.append("      \n\n", 8);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT( 6 && !engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT( 6 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
@@ -108,14 +108,14 @@ testHttpReply::testSanityCheckFirstLine()
     // status line with no message
     input.append("HTTP/1.1 200\n\n", 14); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1 200 \n\n", 15); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -123,42 +123,42 @@ testHttpReply::testSanityCheckFirstLine()
     // incomplete (short) status lines... not sane (yet), but no error either.
     input.append("H", 1);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/", 5);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1", 6);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1", 8);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1 ", 9); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1    20", 14);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -166,21 +166,21 @@ testHttpReply::testSanityCheckFirstLine()
     // status line with no status
     input.append("HTTP/1.1 \n\n", 11);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1     \n\n", 15);
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
 
     input.append("HTTP/1.1  Okay\n\n", 16); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
@@ -188,7 +188,7 @@ testHttpReply::testSanityCheckFirstLine()
     // status line with nul-byte
     input.append("HTTP/1.1" "\0" "200 Okay\n\n", 19); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;
@@ -196,7 +196,7 @@ testHttpReply::testSanityCheckFirstLine()
     // status line with negative status
     input.append("HTTP/1.1 -000\n\n", 15); /* real case seen */
     hdr_len = headersEnd(input.content(),input.contentSize());
-    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;

diff --git a/src/tests/testHttpRequest.cc b/src/tests/testHttpRequest.cc
index 336ae3265c9a307e27dfc942e97b1f1d2093ee0d..aacda2df555d19378e47ec490e05b59f2292ee9d 100644 (file)
--- a/src/tests/testHttpRequest.cc
+++ b/src/tests/testHttpRequest.cc
@@ -22,7 +22,7 @@ CPPUNIT_TEST_SUITE_REGISTRATION( testHttpRequest );
 class PrivateHttpRequest : public HttpRequest
 {
 public:
-    bool doSanityCheckStartLine(MemBuf *b, const size_t h, Http::StatusCode *e) { return sanityCheckStartLine(b,h,e); };
+    bool doSanityCheckStartLine(const char *b, const size_t h, Http::StatusCode *e) { return sanityCheckStartLine(b,h,e); };
 };
 
 /* init memory pools */
@@ -164,14 +164,14 @@ testHttpRequest::testSanityCheckStartLine()
     // a valid request line
     input.append("GET / HTTP/1.1\n\n", 16);
     hdr_len = headersEnd(input.content(), input.contentSize());
-    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("GET  /  HTTP/1.1\n\n", 18);
     hdr_len = headersEnd(input.content(), input.contentSize());
-    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -179,14 +179,14 @@ testHttpRequest::testSanityCheckStartLine()
     // strange but valid methods
     input.append(". / HTTP/1.1\n\n", 14);
     hdr_len = headersEnd(input.content(), input.contentSize());
-    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
 
     input.append("OPTIONS * HTTP/1.1\n\n", 20);
     hdr_len = headersEnd(input.content(), input.contentSize());
-    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scNone);
     input.reset();
     error = Http::scNone;
@@ -203,7 +203,7 @@ testHttpRequest::testSanityCheckStartLine()
 
     input.append("      \n\n", 8);
     hdr_len = headersEnd(input.content(), input.contentSize());
-    CPPUNIT_ASSERT(!engine.doSanityCheckStartLine(&input, hdr_len, &error) );
+    CPPUNIT_ASSERT(!engine.doSanityCheckStartLine(input.content(), hdr_len, &error) );
     CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader);
     input.reset();
     error = Http::scNone;

diff --git a/src/tunnel.cc b/src/tunnel.cc
index fec72681adc3a8c78eb69a678570b5d7b5c7b120..4dda5d2fea3b2e49ab37ede5bacf7202f132af9a 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -399,7 +399,8 @@ TunnelStateData::handleConnectResponse(const size_t chunkSize)
     HttpReply rep;
     Http::StatusCode parseErr = Http::scNone;
     const bool eof = !chunkSize;
-    const bool parsed = rep.parse(connectRespBuf, eof, &parseErr);
+    connectRespBuf->terminate(); // HttpMsg::parse requires terminated string
+    const bool parsed = rep.parse(connectRespBuf->content(), connectRespBuf->contentSize(), eof, &parseErr);
     if (!parsed) {
         if (parseErr > 0) { // unrecoverable parsing error
             server.logicError("malformed CONNECT response from peer");