From 7024fb734a59409889e53df2257b3fc817809fb4 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 31 Mar 2021 02:44:42 +0000
Subject: [PATCH] Handle more Range requests (#790)

Also removed some effectively unused code.
---
 src/HttpHdrRange.cc        | 17 -------------
 src/HttpHeaderRange.h      |  5 ++--
 src/client_side.cc         |  4 ++--
 src/client_side_request.cc | 27 ++++++++++++++++++---
 src/client_side_request.h  |  7 +++++-
 src/http/Stream.cc         | 49 ++++++--------------------------------
 6 files changed, 41 insertions(+), 68 deletions(-)

diff --git a/src/HttpHdrRange.cc b/src/HttpHdrRange.cc
index e7179dc..5849144 100644
--- a/src/HttpHdrRange.cc
+++ b/src/HttpHdrRange.cc
@@ -526,23 +526,6 @@ HttpHdrRange::offsetLimitExceeded(const int64_t limit) const
     return true;
 }

-bool
-HttpHdrRange::contains(const HttpHdrRangeSpec& r) const
-{
-    assert(r.length >= 0);
-    HttpHdrRangeSpec::HttpRange rrange(r.offset, r.offset + r.length);
-
-    for (const_iterator i = begin(); i != end(); ++i) {
-        HttpHdrRangeSpec::HttpRange irange((*i)->offset, (*i)->offset + (*i)->length);
-        HttpHdrRangeSpec::HttpRange intersection = rrange.intersection(irange);
-
-        if (intersection.start == irange.start && intersection.size() == irange.size())
-            return true;
-    }
-
-    return false;
-}
-
 const HttpHdrRangeSpec *
 HttpHdrRangeIter::currentSpec() const
 {
diff --git a/src/HttpHeaderRange.h b/src/HttpHeaderRange.h
index 2103b64..352e749 100644
--- a/src/HttpHeaderRange.h
+++ b/src/HttpHeaderRange.h
@@ -78,7 +78,6 @@ public:
     int64_t firstOffset() const;
     int64_t lowestOffset(int64_t) const;
     bool offsetLimitExceeded(const int64_t limit) const;
-    bool contains(const HttpHdrRangeSpec& r) const;
     std::vector<HttpHdrRangeSpec *> specs;

 private:
@@ -100,9 +99,9 @@ public:
     void updateSpec();
     int64_t debt() const;
     void debt(int64_t);
-    int64_t debt_size;      /* bytes left to send from the current spec */
+    int64_t debt_size = 0;  /* bytes left to send from the current spec */
     String boundary;        /* boundary for multipart responses */
-    bool valid;
+    bool valid = false;
 };

 #endif /* SQUID_HTTPHEADERRANGE_H */
diff --git a/src/client_side.cc b/src/client_side.cc
index 120c39c..9516166 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -724,8 +724,8 @@ clientPackRangeHdr(const HttpReply * rep, const HttpHdrRangeSpec * spec, String
  * warning: assumes that HTTP headers for individual ranges at the
  *          time of the actuall assembly will be exactly the same as
  *          the headers when clientMRangeCLen() is called */
-int
-ClientHttpRequest::mRangeCLen()
+int64_t
+ClientHttpRequest::mRangeCLen() const
 {
     int64_t clen = 0;
     MemBuf mb;
diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index a37f8d4..3c83e9e 100644
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1094,9 +1094,6 @@ clientInterpretRequestHeaders(ClientHttpRequest * http)
              * iter up at this point.
              */
             node->readBuffer.offset = request->range->lowestOffset(0);
-            http->range_iter.pos = request->range->begin();
-            http->range_iter.end = request->range->end();
-            http->range_iter.valid = true;
         }
     }

@@ -1954,6 +1951,30 @@ ClientHttpRequest::setErrorUri(const char *aUri)
 #include "client_side_request.cci"
 #endif

+// XXX: This should not be a _request_ method. Move range_iter elsewhere.
+int64_t
+ClientHttpRequest::prepPartialResponseGeneration()
+{
+    assert(request);
+    assert(request->range);
+
+    range_iter.pos = request->range->begin();
+    range_iter.end = request->range->end();
+    range_iter.debt_size = 0;
+    const auto multipart = request->range->specs.size() > 1;
+    if (multipart)
+        range_iter.boundary = rangeBoundaryStr();
+    range_iter.valid = true; // TODO: Remove.
+    range_iter.updateSpec(); // TODO: Refactor to initialize rather than update.
+
+    assert(range_iter.pos != range_iter.end);
+    const auto &firstRange = *range_iter.pos;
+    assert(firstRange);
+    out.offset = firstRange->offset;
+
+    return multipart ? mRangeCLen() : firstRange->length;
+}
+
 #if USE_ADAPTATION
 /// Initiate an asynchronous adaptation transaction which will call us back.
 void
diff --git a/src/client_side_request.h b/src/client_side_request.h
index 704802b..7ab3262 100644
--- a/src/client_side_request.h
+++ b/src/client_side_request.h
@@ -131,7 +131,7 @@ public:

     dlink_node active;
     dlink_list client_stream;
-    int mRangeCLen();
+    int64_t mRangeCLen() const;

     ClientRequestContext *calloutContext;
     void doCallouts();
@@ -148,6 +148,11 @@ public:
     /// neither the current request nor the parsed request URI are known
     void setErrorUri(const char *errorUri);

+    /// Prepares to satisfy a Range request with a generated HTTP 206 response.
+    /// Initializes range_iter state to allow raw range_iter access.
+    /// \returns Content-Length value for the future response; never negative
+    int64_t prepPartialResponseGeneration();
+
     /// Build an error reply. For use with the callouts.
     void calloutsError(const err_type error, const int errDetail);

diff --git a/src/http/Stream.cc b/src/http/Stream.cc
index 1370862..ff44496 100644
--- a/src/http/Stream.cc
+++ b/src/http/Stream.cc
@@ -444,59 +444,27 @@ Http::Stream::buildRangeHeader(HttpReply *rep)
     } else {
         /* XXX: TODO: Review, this unconditional set may be wrong. */
         rep->sline.set(rep->sline.version, Http::scPartialContent);
-        // web server responded with a valid, but unexpected range.
-        // will (try-to) forward as-is.
-        //TODO: we should cope with multirange request/responses
-        // TODO: review, since rep->content_range is always nil here.
-        bool replyMatchRequest = contentRange != nullptr ?
-                                 request->range->contains(contentRange->spec) :
-                                 true;
+
+        // before range_iter accesses
+        const auto actual_clen = http->prepPartialResponseGeneration();
+
         const int spec_count = http->request->range->specs.size();
-        int64_t actual_clen = -1;

         debugs(33, 3, "range spec count: " << spec_count <<
                " virgin clen: " << rep->content_length);
         assert(spec_count > 0);
         /* append appropriate header(s) */
         if (spec_count == 1) {
-            if (!replyMatchRequest) {
-                hdr->putContRange(contentRange);
-                actual_clen = rep->content_length;
-                //http->range_iter.pos = rep->content_range->spec.begin();
-                (*http->range_iter.pos)->offset = contentRange->spec.offset;
-                (*http->range_iter.pos)->length = contentRange->spec.length;
-
-            } else {
-                HttpHdrRange::iterator pos = http->request->range->begin();
-                assert(*pos);
-                /* append Content-Range */
-
-                if (!contentRange) {
-                    /* No content range, so this was a full object we are
-                     * sending parts of.
-                     */
-                    httpHeaderAddContRange(hdr, **pos, rep->content_length);
-                }
-
-                /* set new Content-Length to the actual number of bytes
-                 * transmitted in the message-body */
-                actual_clen = (*pos)->length;
-            }
+            const auto singleSpec = *http->request->range->begin();
+            assert(singleSpec);
+            httpHeaderAddContRange(hdr, *singleSpec, rep->content_length);
         } else {
             /* multipart! */
-            /* generate boundary string */
-            http->range_iter.boundary = http->rangeBoundaryStr();
             /* delete old Content-Type, add ours */
             hdr->delById(Http::HdrType::CONTENT_TYPE);
             httpHeaderPutStrf(hdr, Http::HdrType::CONTENT_TYPE,
                               "multipart/byteranges; boundary=\"" SQUIDSTRINGPH "\"",
                               SQUIDSTRINGPRINT(http->range_iter.boundary));
-            /* Content-Length is not required in multipart responses
-             * but it is always nice to have one */
-            actual_clen = http->mRangeCLen();
-
-            /* http->out needs to start where we want data at */
-            http->out.offset = http->range_iter.currentSpec()->offset;
         }

         /* replace Content-Length header */
@@ -504,9 +472,6 @@ Http::Stream::buildRangeHeader(HttpReply *rep)
         hdr->delById(Http::HdrType::CONTENT_LENGTH);
         hdr->putInt64(Http::HdrType::CONTENT_LENGTH, actual_clen);
         debugs(33, 3, "actual content length: " << actual_clen);
-
-        /* And start the range iter off */
-        http->range_iter.updateSpec();
     }
 }

--
2.23.0