Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
669153006:
Add HTTPBodyStream interface, three concrete implementations, and their tests. (Closed)
|
Created:
6 years, 2 months ago by Robert Sesek Modified:
6 years, 2 months ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Project:
crashpad Visibility:
Public. |
DescriptionAdd HTTPBodyStream interface, three concrete implementations, and their tests.
BUG=415544
R=mark@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/977a7a805210856303bfd32de7f7d52b0edcfdce
Patch Set 1 #
Total comments: 69
Patch Set 2 : Address comments. #Patch Set 3 : Close and read error logging #
Total comments: 13
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode21 util/net/http_body.cc:21: #include <vector> You don’t need this, it’s already been #included by http_body.h. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode31 util/net/http_body.cc:31: : string_(string), bytes_read_() { I’ve been explicitly initializing the base classes. Helps remind you what’s going on. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode43 util/net/http_body.cc:43: size_t num_bytes_returned = std::min(num_bytes_remaining, max_len); #include <algorithm> https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode43 util/net/http_body.cc:43: size_t num_bytes_returned = std::min(num_bytes_remaining, max_len); 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()); https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode58 util/net/http_body.cc:58: if (fd_ > 0) { 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(). https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode64 util/net/http_body.cc:64: if (fd_ == 0) { 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. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode65 util/net/http_body.cc:65: fd_ = open(path_.value().c_str(), O_RDONLY); open() needs to be wrapped in HANDLE_EINTR() (base/posix/eintr_wrapper.h). https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode79 util/net/http_body.cc:79: } You haven’t handled the ReadFD() error case. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode96 util/net/http_body.cc:96: ssize_t CompositeHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, 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. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h File util/net/http_body.h (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode35 util/net/http_body.h:35: //! \param[in] buffer A user supplied buffer into which this method will copy user-supplied (hyphen). https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode35 util/net/http_body.h:35: //! \param[in] buffer A user supplied buffer into which this method will copy \param[out] https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode37 util/net/http_body.h:37: //! \param[in] max_len The maximum length of \a buffer. At most this many It’s the length (or size) of buffer, or the maximum length to copy. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode41 util/net/http_body.h:41: //! actually copied to \a buffer. On failure a negative number. When failure, (comma). https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode41 util/net/http_body.h:41: //! actually copied to \a buffer. On failure a negative number. When Should the caller stop calling if this returns a failure code? https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode42 util/net/http_body.h:42: //! the stream has no more data, returns 0. I’ve been using `0` for things that are literal like this, same with `true` on line 45. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode43 util/net/http_body.h:43: virtual ssize_t GetBytesBuffer(uint8_t* buffer, size_t max_len) = 0; ssize_t is not guaranteed in <stdint.h>. #include <sys/types.h> too. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode47 util/net/http_body.h:47: virtual bool HasBytesRemaining() = 0; Is this even necessary? It seems like all the caller needs is GetBytesBuffer(), and once that returns 0, the data’s been exhausted. More comments on this are in the test (TwoEmptyStrings), where the existence of this call actually started to seem hazardous. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode61 util/net/http_body.h:61: ~StringHTTPBodyStream() override; Blank line before this so it doesn’t look like the documentation applies to it. Same for the other concrete classes. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode80 util/net/http_body.h:80: //! \param[in] path The file from this HTTPBodyStream will read. Missing word (“which”?) https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode91 util/net/http_body.h:91: // If |fd_| is zero, then the file has not yet been opened (with |at_eof_| This is dangerous, because 0 is a valid file descriptor value. Can we share -1 between “not yet open” and “error?” Or can we use -1 for “not yet open”, -2 for “error?” https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:23: std::string ReadStreamToString(HTTPBodyStream* stream) { I would move this down to above ReadASCIIFile, since it’s not used until that test, and the proximity kind of helps when you read the whole file, in the same way as “don’t declare variables until you use them” helps. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:42: void ExpectStringBuffer(const std::string& expected, Ditto, above SmallString. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:45: for (size_t i = 0; i < actual_len; ++i) { You could just do: std::string actual_string(reinterpret_cast<const char*>(actual), actual_len); EXPECT_EQ(expected, actual); https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:51: TEST(StringHTTPBodyStreamTest, EmptyString) { Except for death tests, the test class name never ends in Test in crashpad, because it’s just redundant boilerplate noise. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:53: memset(buf, '!', sizeof(buf)); #include <string.h> https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:59: EXPECT_EQ('!', buf[0]); Check the whole (remainder) of the buffer. Maybe you could have a helper do this. Same for SmallString and the other cases where you validate the unused portion of the buffer was actually untouched. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:124: base::FilePath path = base::FilePath("util/net/testdata/ascii_http_body.txt"); This is fine for now, but I filed bug 4 to do something better. You should link to that bug here and in the ReadBinaryFile test. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:136: uint8_t buf[4]; Comment that the buffer size was intentionally chosen so that it would take multiple reads. (For the string implementation test, no comment was necessary, because the test name was MultipleReads). https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:160: for (size_t i = 0; i < sizeof(buf); ++i) If you had a !-untouched verifier helper, you could use it here. And even on line 154. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:165: base::FilePath path = base::FilePath("/tmp/crashpad/util/net/http_body/null"); /var/empty, not /tmp https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:184: EXPECT_FALSE(stream.HasBytesRemaining()); All of your tests call HasBytesRemaining() before trying to get bytes, which might mask bugs where HasBytesRemaining() alters the state of the object (by making it usable). I don’t think that you actually need to expose (or perhaps even implement) HasBytesRemaining() at all. Merely having the documented behavior be “keep calling GetBytesBuffer() until it returns 0” should be sufficient. I can’t think of a reason for a real consumer to ever need to call HasBytesRemaining(). https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:236: std::string actual_string = ReadStreamToString(&stream); ReadStreamToString will get the whole thing in three reads, one for each of the three composite parts. Nothing actually tests that the composite part isn’t advancing to its next subpart until it’s exhausted the subpart’s data. For all this test knows, the composite part could just be advancing to the next subpart on each read. This is a very important attribute of the composite part implementation to test. https://codereview.chromium.org/669153006/diff/1/util/net/testdata/binary_htt... File util/net/testdata/binary_http_body.dat (right): https://codereview.chromium.org/669153006/diff/1/util/net/testdata/binary_htt... util/net/testdata/binary_http_body.dat:1: þíúΡ Allays!
https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode21 util/net/http_body.cc:21: #include <vector> On 2014/10/23 19:31:03, Mark Mentovai wrote: > You don’t need this, it’s already been #included by http_body.h. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode31 util/net/http_body.cc:31: : string_(string), bytes_read_() { On 2014/10/23 19:31:03, Mark Mentovai wrote: > I’ve been explicitly initializing the base classes. Helps remind you what’s > going on. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode43 util/net/http_body.cc:43: size_t num_bytes_returned = std::min(num_bytes_remaining, max_len); On 2014/10/23 19:31:03, Mark Mentovai wrote: > #include <algorithm> Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode43 util/net/http_body.cc:43: size_t num_bytes_returned = std::min(num_bytes_remaining, max_len); On 2014/10/23 19:31:03, Mark Mentovai wrote: > 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()); Done. I tried to write a test for this, but you can't allocate an array that big. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode58 util/net/http_body.cc:58: if (fd_ > 0) { On 2014/10/23 19:31:03, Mark Mentovai wrote: > 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(). ScopedFD does not check if fd<0 before trying to close. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode64 util/net/http_body.cc:64: if (fd_ == 0) { On 2014/10/23 19:31:02, Mark Mentovai wrote: > 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. Done. Added a test. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode65 util/net/http_body.cc:65: fd_ = open(path_.value().c_str(), O_RDONLY); On 2014/10/23 19:31:03, Mark Mentovai wrote: > open() needs to be wrapped in HANDLE_EINTR() (base/posix/eintr_wrapper.h). Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode79 util/net/http_body.cc:79: } On 2014/10/23 19:31:03, Mark Mentovai wrote: > You haven’t handled the ReadFD() error case. The semantics are the same as GetBytesBuffer (% errno), so I don't think it's necessary. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode96 util/net/http_body.cc:96: ssize_t CompositeHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, On 2014/10/23 19:31:03, Mark Mentovai wrote: > 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. Right, this is something I considered, and I decided that simpler here is better. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h File util/net/http_body.h (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode35 util/net/http_body.h:35: //! \param[in] buffer A user supplied buffer into which this method will copy On 2014/10/23 19:31:03, Mark Mentovai wrote: > \param[out] Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode35 util/net/http_body.h:35: //! \param[in] buffer A user supplied buffer into which this method will copy On 2014/10/23 19:31:03, Mark Mentovai wrote: > user-supplied (hyphen). Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode37 util/net/http_body.h:37: //! \param[in] max_len The maximum length of \a buffer. At most this many On 2014/10/23 19:31:04, Mark Mentovai wrote: > It’s the length (or size) of buffer, or the maximum length to copy. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode41 util/net/http_body.h:41: //! actually copied to \a buffer. On failure a negative number. When On 2014/10/23 19:31:03, Mark Mentovai wrote: > failure, (comma). Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode41 util/net/http_body.h:41: //! actually copied to \a buffer. On failure a negative number. When On 2014/10/23 19:31:03, Mark Mentovai wrote: > failure, (comma). Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode42 util/net/http_body.h:42: //! the stream has no more data, returns 0. On 2014/10/23 19:31:03, Mark Mentovai wrote: > I’ve been using `0` for things that are literal like this, same with `true` on > line 45. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode43 util/net/http_body.h:43: virtual ssize_t GetBytesBuffer(uint8_t* buffer, size_t max_len) = 0; On 2014/10/23 19:31:03, Mark Mentovai wrote: > ssize_t is not guaranteed in <stdint.h>. #include <sys/types.h> too. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode47 util/net/http_body.h:47: virtual bool HasBytesRemaining() = 0; On 2014/10/23 19:31:03, Mark Mentovai wrote: > Is this even necessary? It seems like all the caller needs is GetBytesBuffer(), > and once that returns 0, the data’s been exhausted. > > More comments on this are in the test (TwoEmptyStrings), where the existence of > this call actually started to seem hazardous. No, it's not. I didn't read NSInputStream carefully enough. Removed. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode61 util/net/http_body.h:61: ~StringHTTPBodyStream() override; On 2014/10/23 19:31:03, Mark Mentovai wrote: > Blank line before this so it doesn’t look like the documentation applies to it. > Same for the other concrete classes. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode80 util/net/http_body.h:80: //! \param[in] path The file from this HTTPBodyStream will read. On 2014/10/23 19:31:03, Mark Mentovai wrote: > Missing word (“which”?) Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.h#newcode91 util/net/http_body.h:91: // If |fd_| is zero, then the file has not yet been opened (with |at_eof_| On 2014/10/23 19:31:03, Mark Mentovai wrote: > This is dangerous, because 0 is a valid file descriptor value. Can we share -1 > between “not yet open” and “error?” Or can we use -1 for “not yet open”, -2 for > “error?” Good point. Done, latter suggestion. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:23: std::string ReadStreamToString(HTTPBodyStream* stream) { On 2014/10/23 19:31:04, Mark Mentovai wrote: > I would move this down to above ReadASCIIFile, since it’s not used until that > test, and the proximity kind of helps when you read the whole file, in the same > way as “don’t declare variables until you use them” helps. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:42: void ExpectStringBuffer(const std::string& expected, On 2014/10/23 19:31:04, Mark Mentovai wrote: > Ditto, above SmallString. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:45: for (size_t i = 0; i < actual_len; ++i) { On 2014/10/23 19:31:04, Mark Mentovai wrote: > You could just do: > > std::string actual_string(reinterpret_cast<const char*>(actual), actual_len); > EXPECT_EQ(expected, actual); Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:51: TEST(StringHTTPBodyStreamTest, EmptyString) { On 2014/10/23 19:31:04, Mark Mentovai wrote: > Except for death tests, the test class name never ends in Test in crashpad, > because it’s just redundant boilerplate noise. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:53: memset(buf, '!', sizeof(buf)); On 2014/10/23 19:31:04, Mark Mentovai wrote: > #include <string.h> Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:59: EXPECT_EQ('!', buf[0]); On 2014/10/23 19:31:04, Mark Mentovai wrote: > Check the whole (remainder) of the buffer. Maybe you could have a helper do > this. Same for SmallString and the other cases where you validate the unused > portion of the buffer was actually untouched. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:124: base::FilePath path = base::FilePath("util/net/testdata/ascii_http_body.txt"); On 2014/10/23 19:31:04, Mark Mentovai wrote: > This is fine for now, but I filed bug 4 to do something better. You should link > to that bug here and in the ReadBinaryFile test. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:136: uint8_t buf[4]; On 2014/10/23 19:31:04, Mark Mentovai wrote: > Comment that the buffer size was intentionally chosen so that it would take > multiple reads. (For the string implementation test, no comment was necessary, > because the test name was MultipleReads). Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:160: for (size_t i = 0; i < sizeof(buf); ++i) On 2014/10/23 19:31:04, Mark Mentovai wrote: > If you had a !-untouched verifier helper, you could use it here. And even on > line 154. Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:165: base::FilePath path = base::FilePath("/tmp/crashpad/util/net/http_body/null"); On 2014/10/23 19:31:04, Mark Mentovai wrote: > /var/empty, not /tmp Done. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:184: EXPECT_FALSE(stream.HasBytesRemaining()); On 2014/10/23 19:31:04, Mark Mentovai wrote: > All of your tests call HasBytesRemaining() before trying to get bytes, which > might mask bugs where HasBytesRemaining() alters the state of the object (by > making it usable). > > I don’t think that you actually need to expose (or perhaps even implement) > HasBytesRemaining() at all. Merely having the documented behavior be “keep > calling GetBytesBuffer() until it returns 0” should be sufficient. I can’t think > of a reason for a real consumer to ever need to call HasBytesRemaining(). HasBytesRemaining() is now gone. https://codereview.chromium.org/669153006/diff/1/util/net/http_body_test.cc#n... util/net/http_body_test.cc:236: std::string actual_string = ReadStreamToString(&stream); On 2014/10/23 19:31:04, Mark Mentovai wrote: > ReadStreamToString will get the whole thing in three reads, one for each of the > three composite parts. Nothing actually tests that the composite part isn’t > advancing to its next subpart until it’s exhausted the subpart’s data. For all > this test knows, the composite part could just be advancing to the next subpart > on each read. > > This is a very important attribute of the composite part implementation to test. Done. Now have a parameterized test for various buffer sizes.
https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode58 util/net/http_body.cc:58: if (fd_ > 0) { Robert Sesek wrote: > ScopedFD does not check if fd<0 before trying to close. 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(). https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode79 util/net/http_body.cc:79: } Robert Sesek wrote: > On 2014/10/23 19:31:03, Mark Mentovai wrote: > > You haven’t handled the ReadFD() error case. > > The semantics are the same as GetBytesBuffer (% errno), so I don't think it's > necessary. I think that at least a PLOG is warranted.
https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode58 util/net/http_body.cc:58: if (fd_ > 0) { On 2014/10/24 16:59:43, Mark Mentovai wrote: > Robert Sesek wrote: > > ScopedFD does not check if fd<0 before trying to close. > > 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(). Per offline discussion, fd_ now stores multiple negative values to indicate state. Added CloseFD() to fd_io.h to handle the plog. https://codereview.chromium.org/669153006/diff/1/util/net/http_body.cc#newcode79 util/net/http_body.cc:79: } On 2014/10/24 16:59:43, Mark Mentovai wrote: > Robert Sesek wrote: > > On 2014/10/23 19:31:03, Mark Mentovai wrote: > > > You haven’t handled the ReadFD() error case. > > > > The semantics are the same as GetBytesBuffer (% errno), so I don't think it's > > necessary. > > I think that at least a PLOG is warranted. Done.
LGTM https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h File util/file/fd_io.h (right): https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h#newcode84 util/file/fd_io.h:84: //! \return On success, `0` is returned. On failure, an error is logged and Map this to a bool? https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h#newcode86 util/file/fd_io.h:86: int CloseFD(int fd); I’m a little troubled by the fact that you’d conceptually think that ReadFD and WriteFD go with CloseFD, but CloseFD is the only one of the three that logs on error. CheckedCloseFD would obviously PCHECK and that goes with CheckedRead/WriteFD, but if you don’t want a CHECKing variant, maybe the best way to resolve this is to give it a different name. LoggingCloseFD? https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc#ne... util/net/http_body.cc:57: if (fd_ > kUnopenedFile) { This is sensitive to the ordering of the enum. I’d either make this |>= 0| (which adheres to the documentation you’ve written for the fd_ member) or add an enum member like kMinimumValidFD to be used here. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc#ne... util/net/http_body.cc:63: if (fd_ == kUnopenedFile) { switch? https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.... util/net/http_body_test.cc:99: while (true) { Optional, but this could have been ssize_t bytes_read; while ((bytes_read = stream->GetBytesBuffer(buf.get(), buffer_size) != 0) { Then you can get rid of the break. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.... util/net/http_body_test.cc:126: EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf))); Do this twice to make sure that it actually returns 0 the second time, instead of reopening the file and returning more data. Do this for the ReadBinaryFile test too. And actually, you might want to augment ReadStreamToString() to do this as well. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.... util/net/http_body_test.cc:231: testing::Values(1, 2, 9, 16, 31, 128)); Awesome.
https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h File util/file/fd_io.h (right): https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h#newcode84 util/file/fd_io.h:84: //! \return On success, `0` is returned. On failure, an error is logged and On 2014/10/24 17:46:58, Mark Mentovai wrote: > Map this to a bool? Done. https://codereview.chromium.org/669153006/diff/40001/util/file/fd_io.h#newcode86 util/file/fd_io.h:86: int CloseFD(int fd); On 2014/10/24 17:46:58, Mark Mentovai wrote: > I’m a little troubled by the fact that you’d conceptually think that ReadFD and > WriteFD go with CloseFD, but CloseFD is the only one of the three that logs on > error. > > CheckedCloseFD would obviously PCHECK and that goes with CheckedRead/WriteFD, > but if you don’t want a CHECKing variant, maybe the best way to resolve this is > to give it a different name. LoggingCloseFD? Yea, that bothered me too. Renamed to LoggingCloseFD(). https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc#ne... util/net/http_body.cc:57: if (fd_ > kUnopenedFile) { On 2014/10/24 17:46:58, Mark Mentovai wrote: > This is sensitive to the ordering of the enum. I’d either make this |>= 0| > (which adheres to the documentation you’ve written for the fd_ member) or add an > enum member like kMinimumValidFD to be used here. Done. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body.cc#ne... util/net/http_body.cc:63: if (fd_ == kUnopenedFile) { On 2014/10/24 17:46:58, Mark Mentovai wrote: > switch? Done. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.... util/net/http_body_test.cc:99: while (true) { On 2014/10/24 17:46:58, Mark Mentovai wrote: > Optional, but this could have been > > ssize_t bytes_read; > while ((bytes_read = stream->GetBytesBuffer(buf.get(), buffer_size) != 0) { > > Then you can get rid of the break. Done. https://codereview.chromium.org/669153006/diff/40001/util/net/http_body_test.... util/net/http_body_test.cc:126: EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf))); On 2014/10/24 17:46:58, Mark Mentovai wrote: > Do this twice to make sure that it actually returns 0 the second time, instead > of reopening the file and returning more data. Do this for the ReadBinaryFile > test too. And actually, you might want to augment ReadStreamToString() to do > this as well. Done.
LGTM https://codereview.chromium.org/669153006/diff/60001/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/60001/util/net/http_body.cc#ne... util/net/http_body.cc:76: } default: break; It’s required by the style guide. https://codereview.chromium.org/669153006/diff/60001/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/60001/util/net/http_body_test.... util/net/http_body_test.cc:101: if (bytes_read == 0) { Get rid of this now.
https://codereview.chromium.org/669153006/diff/60001/util/net/http_body.cc File util/net/http_body.cc (right): https://codereview.chromium.org/669153006/diff/60001/util/net/http_body.cc#ne... util/net/http_body.cc:76: } On 2014/10/24 18:58:21, Mark Mentovai wrote: > default: > break; > > It’s required by the style guide. Done. https://codereview.chromium.org/669153006/diff/60001/util/net/http_body_test.cc File util/net/http_body_test.cc (right): https://codereview.chromium.org/669153006/diff/60001/util/net/http_body_test.... util/net/http_body_test.cc:101: if (bytes_read == 0) { On 2014/10/24 18:58:21, Mark Mentovai wrote: > Get rid of this now. I did, but I forgot to save before uploading. Done.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 977a7a805210856303bfd32de7f7d52b0edcfdce (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
