Add HTTPBodyStream interface, three concrete implementations, and their tests.

util/net/http_body.cc

Index: util/net/http_body.cc
diff --git a/util/net/http_body.cc b/util/net/http_body.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6e0a7ed097fd21dbc37f005ab058cffc27f866a4
--- /dev/null
+++ b/util/net/http_body.cc
@@ -0,0 +1,124 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/net/http_body.h"
+
+#include <fcntl.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <vector>

[Mark Mentovai, 2014/10/23 19:31:03]

You don’t need this, it’s already been #included by http_body.h.

[Robert Sesek, 2014/10/24 16:53:32]

Done.

+
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
+#include "util/file/fd_io.h"
+
+namespace crashpad {
+
+StringHTTPBodyStream::StringHTTPBodyStream(const std::string& string)
+    : string_(string), bytes_read_() {

[Mark Mentovai, 2014/10/23 19:31:03]

I’ve been explicitly initializing the base classes. Helps remind you what’s
going on.

[Robert Sesek, 2014/10/24 16:53:32]

Done.

+}
+
+StringHTTPBodyStream::~StringHTTPBodyStream() {
+}
+
+ssize_t StringHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, size_t max_len) {
+  size_t num_bytes_remaining = string_.length() - bytes_read_;
+  if (num_bytes_remaining == 0) {
+    return num_bytes_remaining;
+  }
+
+  size_t num_bytes_returned = std::min(num_bytes_remaining, max_len);

[Mark Mentovai, 2014/10/23 19:31:03]

#include <algorithm>

[Mark Mentovai, 2014/10/23 19:31:03]

You should also make sure that this doesn’t overflow the ssize_t return value.

#include <limits>
  size_t num_bytes_returned = std::min(std::min(num_bytes_remaining, max_len),
std::numeric_limits<ssize_t>::max());

[Robert Sesek, 2014/10/24 16:53:32]

Done.

[Robert Sesek, 2014/10/24 16:53:32]

Done. I tried to write a test for this, but you can't allocate an array that
big.

+  memcpy(buffer, &string_[bytes_read_], num_bytes_returned);
+  bytes_read_ += num_bytes_returned;
+  return num_bytes_returned;
+}
+
+bool StringHTTPBodyStream::HasBytesRemaining() {
+  return bytes_read_ < string_.length();
+}
+
+FileHTTPBodyStream::FileHTTPBodyStream(const base::FilePath& path)
+    : path_(path), fd_(0), at_eof_(false) {
+}
+
+FileHTTPBodyStream::~FileHTTPBodyStream() {
+  if (fd_ > 0) {

[Mark Mentovai, 2014/10/23 19:31:03]

You could have used a ScopedFD from base/files/scoped_file.h. That also gets you
a nice handy CHECK that the close worked, here and in the other location where
you close().

[Robert Sesek, 2014/10/24 16:53:32]

ScopedFD does not check if fd<0 before trying to close.

[Mark Mentovai, 2014/10/24 16:59:43]

Robert Sesek wrote:
Sure it does.

Well, it checks that it’s not -1, but that’s the only value that’s important.

ScopedFDCloseTraits::InvalidValue() returns -1. ScopedGeneric::FreeIfNecessary()
only calls Free() (which does the close()) if the fd is not InvalidValue(), and
once it’s done, it sets the fd to InvalidValue().

[Robert Sesek, 2014/10/24 17:20:37]

Per offline discussion, fd_ now stores multiple negative values to indicate
state. Added CloseFD() to fd_io.h to handle the plog.

+    close(fd_);
+  }
+}
+
+ssize_t FileHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, size_t max_len) {
+  if (fd_ == 0) {

[Mark Mentovai, 2014/10/23 19:31:02]

If someone calls this again after GetBytesBuffer() returns 0, it will reopen the
file and start providing data again. That shouldn’t happen. It should keep
returning 0 without reopening the file.

[Robert Sesek, 2014/10/24 16:53:32]

Done. Added a test.

+    fd_ = open(path_.value().c_str(), O_RDONLY);

[Mark Mentovai, 2014/10/23 19:31:03]

open() needs to be wrapped in HANDLE_EINTR() (base/posix/eintr_wrapper.h).

[Robert Sesek, 2014/10/24 16:53:32]

Done.

+    if (fd_ < 0) {
+      PLOG(ERROR) << "Cannot open " << path_.value();
+      return -1;
+    }
+  } else if (fd_ < 0) {
+    return -2;
+  }
+
+  ssize_t rv = ReadFD(fd_, buffer, max_len);
+  if (rv == 0) {
+    close(fd_);
+    fd_ = 0;
+    at_eof_ = true;
+  }

[Mark Mentovai, 2014/10/23 19:31:03]

You haven’t handled the ReadFD() error case.

[Robert Sesek, 2014/10/24 16:53:32]

The semantics are the same as GetBytesBuffer (% errno), so I don't think it's
necessary.

[Mark Mentovai, 2014/10/24 16:59:43]

Robert Sesek wrote:
I think that at least a PLOG is warranted.

[Robert Sesek, 2014/10/24 17:20:37]

Done.

+  return rv;
+}
+
+bool FileHTTPBodyStream::HasBytesRemaining() {
+  return fd_ >= 0 && !at_eof_;
+}
+
+CompositeHTTPBodyStream::CompositeHTTPBodyStream(
+    const CompositeHTTPBodyStream::PartsList& parts)
+    : parts_(parts), current_part_(parts_.begin()) {
+}
+
+CompositeHTTPBodyStream::~CompositeHTTPBodyStream() {
+  STLDeleteContainerPointers(parts_.begin(), parts_.end());
+}
+
+ssize_t CompositeHTTPBodyStream::GetBytesBuffer(uint8_t* buffer,

[Mark Mentovai, 2014/10/23 19:31:03]

This is good. The only thing I could think of that might improve it (this is
optional) would be to try to fill as much of buffer as possible, up to max_len,
from multiple parts. Depending on the transport library this gets hooked up to,
that might result in filling packets more fully, resulting in fewer, larger
packets.

[Robert Sesek, 2014/10/24 16:53:32]

Right, this is something I considered, and I decided that simpler here is
better.

+                                                size_t max_len) {
+  // The call to HasBytesRemaining() will advance to the next part if the
+  // current part no longer has bytes remaining.
+  if (!HasBytesRemaining())
+    return 0;
+
+  ssize_t rv = (*current_part_)->GetBytesBuffer(buffer, max_len);
+
+  if (rv == 0) {
+    // If the current part has returned 0 indicating EOF, call recursively to
+    // advance to the next part and try that.
+    return GetBytesBuffer(buffer, max_len);
+  }
+
+  return rv;
+}
+
+bool CompositeHTTPBodyStream::HasBytesRemaining() {
+  while (current_part_ != parts_.end()) {
+    if ((*current_part_)->HasBytesRemaining())
+      return true;
+    ++current_part_;
+  }
+
+  return false;
+}
+
+}  // namespace crashpad