Mojo: DataPipe: Finish docs/comments in core.h and fix some bugs.

Specifically, document the two-phase APIs and make sure that they
correctly check that the sizes are multiples of the element size.

R=darin@chromium.org

Review URL: https://codereview.chromium.org/136793010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244904 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 271 insertions, 27 deletions

diff --git a/mojo/public/system/core.h b/mojo/public/system/core.h
index 4908d45..923cd59 100644
--- a/mojo/public/system/core.h
+++ b/mojo/public/system/core.h
@@ -436,7 +436,6 @@ MOJO_SYSTEM_EXPORT MojoResult MojoReadMessage(MojoHandle message_pipe_handle,
                                               MojoReadMessageFlags flags);
 
 // Data pipe:
-// TODO(vtl): Moar docs.
 
 // Creates a data pipe, which is a unidirectional communication channel for
 // unframed data, with the given options. Data is unframed, but must come as
@@ -506,19 +505,74 @@ MOJO_SYSTEM_EXPORT MojoResult MojoWriteData(
     uint32_t* num_bytes,
     MojoWriteDataFlags flags);
 
-// TODO(vtl): Document me.
-// TODO(vtl): Note to self: |buffer_num_bytes| is an "in-out" parameter: on the
-// "in" side, |*buffer_num_bytes| is the number requested; on success, on the
-// "out" side, it's the number available (which may be GREATER or LESS than the
-// number requested; if the "all-or-nothing" flag is set, it's AT LEAST the
-// number requested).
+// Begins a two-phase write to the data pipe producer given by
+// |data_pipe_producer_handle|. On success, |*buffer| will be a pointer to which
+// the caller can write |*buffer_num_bytes| bytes of data. If flags has
+// |MOJO_WRITE_DATA_FLAG_ALL_OR_NONE| set, then the output value
+// |*buffer_num_bytes| will be at least as large as its input value, which must
+// also be a multiple of the element size (if |MOJO_WRITE_DATA_FLAG_ALL_OR_NONE|
+// is not set, the input value of |*buffer_num_bytes| is ignored).
+//
+// During a two-phase write, |data_pipe_producer_handle| is *not* writable.
+// E.g., if another thread tries to write to it, it will get |MOJO_RESULT_BUSY|;
+// that thread can then wait for |data_pipe_producer_handle| to become writable
+// again.
+//
+// Once the caller has finished writing data to |*buffer|, it should call
+// |MojoEndWriteData()| to specify the amount written and to complete the
+// two-phase write.
+//
+// Note: If the data pipe has the "may discard" option flag (specified on
+// creation) and |flags| has |MOJO_WRITE_DATA_FLAG_ALL_OR_NONE| set, this may
+// discard some data.
+//
+// Returns:
+//   |MOJO_RESULT_OK| on success.
+//   |MOJO_RESULT_INVALID_ARGUMENT| if some argument was invalid (e.g., if
+//       |data_pipe_producer_handle| is not a handle to a data pipe producer,
+//       |buffer| or |buffer_num_bytes| does not look like a valid pointer, or
+//       flags has |MOJO_WRITE_DATA_FLAG_ALL_OR_NONE| set and
+//       |*buffer_num_bytes| is not a multiple of the element size).
+//   |MOJO_RESULT_FAILED_PRECONDITION| if the data pipe consumer handle has been
+//       closed.
+//   |MOJO_RESULT_OUT_OF_RANGE| if |flags| has
+//       |MOJO_WRITE_DATA_FLAG_ALL_OR_NONE| set and the required amount of data
+//       (specified by |*buffer_num_bytes|) cannot be written contiguously at
+//       this time. (Note that there may be space available for the required
+//       amount of data, but the "next" write position may not be large enough.)
+//   |MOJO_RESULT_BUSY| if there is already a two-phase write ongoing with
+//       |data_pipe_producer_handle| (i.e., |MojoBeginWriteData()| has been
+//       called, but not yet the matching |MojoEndWriteData()|).
+//   |MOJO_RESULT_SHOULD_WAIT| if no data can currently be written (and the
+//       consumer is still open).
 MOJO_SYSTEM_EXPORT MojoResult MojoBeginWriteData(
     MojoHandle data_pipe_producer_handle,
     void** buffer,
     uint32_t* buffer_num_bytes,
     MojoWriteDataFlags flags);
 
-// TODO(vtl): Document me.
+// Ends a two-phase write to the data pipe producer given by
+// |data_pipe_producer_handle| that was begun by a call to
+// |MojoBeginWriteData()| on the same handle. |num_bytes_written| should
+// indicate the amount of data actually written; it must be less than or equal
+// to the value of |*buffer_num_bytes| output by |MojoBeginWriteData()| and must
+// be a multiple of the element size. The buffer given by |*buffer| from
+// |MojoBeginWriteData()| must have been filled with exactly |num_bytes_written|
+// bytes of data.
+//
+// On failure, the two-phase write (if any) is ended (so the handle may become
+// writable again, if there's space available) but no data written to |*buffer|
+// is "put into" the data pipe.
+//
+// Returns:
+//   |MOJO_RESULT_OK| on success.
+//   |MOJO_RESULT_INVALID_ARGUMENT| if |data_pipe_producer_handle| is not a
+//       handle to a data pipe producer or |num_bytes_written| is invalid
+//       (greater than the maximum value provided by |MojoBeginWriteData()| or
+//       not a multiple of the element size).
+//   |MOJO_RESULT_FAILED_PRECONDITION| if the data pipe producer is not in a
+//       two-phase write (e.g., |MojoBeginWriteData()| was not called or
+//       |MojoEndWriteData()| has already been called).
 MOJO_SYSTEM_EXPORT MojoResult MojoEndWriteData(
     MojoHandle data_pipe_producer_handle,
     uint32_t num_bytes_written);
@@ -565,23 +619,74 @@ MOJO_SYSTEM_EXPORT MojoResult MojoEndWriteData(
 //   |MOJO_RESULT_SHOULD_WAIT| if there is no data to be read or discarded (and
 //       the producer is still open) and |flags| does *not* have
 //       |MOJO_READ_DATA_FLAG_ALL_OR_NONE| set.
-//
-// TODO(vtl): Note to self: If |MOJO_READ_DATA_FLAG_QUERY| is set, then
-// |elements| must be null (and nothing will be read).
 MOJO_SYSTEM_EXPORT MojoResult MojoReadData(
     MojoHandle data_pipe_consumer_handle,
     void* elements,
     uint32_t* num_bytes,
     MojoReadDataFlags flags);
 
-// TODO(vtl): Document me.
+// Begins a two-phase read from the data pipe consumer given by
+// |data_pipe_consumer_handle|. On success, |*buffer| will be a pointer from
+// which the caller can read |*buffer_num_bytes| bytes of data. If flags has
+// |MOJO_READ_DATA_FLAG_ALL_OR_NONE| set, then the output value
+// |*buffer_num_bytes| will be at least as large as its input value, which must
+// also be a multiple of the element size (if |MOJO_READ_DATA_FLAG_ALL_OR_NONE|
+// is not set, the input value of |*buffer_num_bytes| is ignored). |flags| must
+// not have |MOJO_READ_DATA_FLAG_DISCARD| or |MOJO_READ_DATA_FLAG_QUERY| set.
+//
+// During a two-phase read, |data_pipe_consumer_handle| is *not* readable.
+// E.g., if another thread tries to read from it, it will get
+// |MOJO_RESULT_BUSY|; that thread can then wait for |data_pipe_consumer_handle|
+// to become readable again.
+//
+// Once the caller has finished reading data from |*buffer|, it should call
+// |MojoEndReadData()| to specify the amount read and to complete the two-phase
+// read.
+//
+// Returns:
+//   |MOJO_RESULT_OK| on success.
+//   |MOJO_RESULT_INVALID_ARGUMENT| if some argument was invalid (e.g., if
+//       |data_pipe_consumer_handle| is not a handle to a data pipe consumer,
+//       |buffer| or |buffer_num_bytes| does not look like a valid pointer,
+//       |flags| has |MOJO_READ_DATA_FLAG_ALL_OR_NONE| set and
+//       |*buffer_num_bytes| is not a multiple of the element size, or |flags|
+//       has invalid flags set).
+//   |MOJO_RESULT_FAILED_PRECONDITION| if the data pipe producer handle has been
+//       closed.
+//   |MOJO_RESULT_OUT_OF_RANGE| if |flags| has |MOJO_READ_DATA_FLAG_ALL_OR_NONE|
+//       set and the required amount of data (specified by |*buffer_num_bytes|)
+//       cannot be read from a contiguous buffer at this time. (Note that there
+//       may be the required amount of data, but it may not be contiguous.)
+//   |MOJO_RESULT_BUSY| if there is already a two-phase read ongoing with
+//       |data_pipe_consumer_handle| (i.e., |MojoBeginReadData()| has been
+//       called, but not yet the matching |MojoEndReadData()|).
+//   |MOJO_RESULT_SHOULD_WAIT| if no data can currently be read (and the
+//       producer is still open).
 MOJO_SYSTEM_EXPORT MojoResult MojoBeginReadData(
     MojoHandle data_pipe_consumer_handle,
     const void** buffer,
     uint32_t* buffer_num_bytes,
     MojoReadDataFlags flags);
 
-// TODO(vtl): Document me.
+// Ends a two-phase read from the data pipe consumer given by
+// |data_pipe_consumer_handle| that was begun by a call to |MojoBeginReadData()|
+// on the same handle. |num_bytes_read| should indicate the amount of data
+// actually read; it must be less than or equal to the value of
+// |*buffer_num_bytes| output by |MojoBeginReadData()| and must be a multiple of
+// the element size.
+//
+// On failure, the two-phase read (if any) is ended (so the handle may become
+// readable again) but no data is "removed" from the data pipe.
+//
+// Returns:
+//   |MOJO_RESULT_OK| on success.
+//   |MOJO_RESULT_INVALID_ARGUMENT| if |data_pipe_consumer_handle| is not a
+//       handle to a data pipe consumer or |num_bytes_written| is invalid
+//       (greater than the maximum value provided by |MojoBeginReadData()| or
+//       not a multiple of the element size).
+//   |MOJO_RESULT_FAILED_PRECONDITION| if the data pipe consumer is not in a
+//       two-phase read (e.g., |MojoBeginReadData()| was not called or
+//       |MojoEndReadData()| has already been called).
 MOJO_SYSTEM_EXPORT MojoResult MojoEndReadData(
     MojoHandle data_pipe_consumer_handle,
     uint32_t num_bytes_read);
diff --git a/mojo/system/data_pipe.cc b/mojo/system/data_pipe.cc
index 639bb5e..670d9fd 100644
--- a/mojo/system/data_pipe.cc
+++ b/mojo/system/data_pipe.cc
@@ -118,6 +118,9 @@ MojoResult DataPipe::ProducerBeginWriteData(void** buffer,
   if (!consumer_open_no_lock())
     return MOJO_RESULT_FAILED_PRECONDITION;
 
+  if (all_or_none && *buffer_num_bytes % element_num_bytes_ != 0)
+    return MOJO_RESULT_INVALID_ARGUMENT;
+
   MojoResult rv = ProducerBeginWriteDataImplNoLock(buffer, buffer_num_bytes,
                                                    all_or_none);
   if (rv != MOJO_RESULT_OK)
@@ -140,7 +143,14 @@ MojoResult DataPipe::ProducerEndWriteData(uint32_t num_bytes_written) {
   // consumer has been closed.
 
   MojoWaitFlags old_consumer_satisfied_flags = ConsumerSatisfiedFlagsNoLock();
-  MojoResult rv = ProducerEndWriteDataImplNoLock(num_bytes_written);
+  MojoResult rv;
+  if (num_bytes_written > producer_two_phase_max_num_bytes_written_ ||
+      num_bytes_written % element_num_bytes_ != 0) {
+    rv = MOJO_RESULT_INVALID_ARGUMENT;
+    producer_two_phase_max_num_bytes_written_ = 0;
+  } else {
+    rv = ProducerEndWriteDataImplNoLock(num_bytes_written);
+  }
   // Two-phase write ended even on failure.
   DCHECK(!producer_in_two_phase_write_no_lock());
   // If we're now writable, we *became* writable (since we weren't writable
@@ -256,6 +266,9 @@ MojoResult DataPipe::ConsumerBeginReadData(const void** buffer,
   if (consumer_in_two_phase_read_no_lock())
     return MOJO_RESULT_BUSY;
 
+  if (all_or_none && *buffer_num_bytes % element_num_bytes_ != 0)
+    return MOJO_RESULT_INVALID_ARGUMENT;
+
   MojoResult rv = ConsumerBeginReadDataImplNoLock(buffer, buffer_num_bytes,
                                                   all_or_none);
   if (rv != MOJO_RESULT_OK)
@@ -272,7 +285,14 @@ MojoResult DataPipe::ConsumerEndReadData(uint32_t num_bytes_read) {
     return MOJO_RESULT_FAILED_PRECONDITION;
 
   MojoWaitFlags old_producer_satisfied_flags = ProducerSatisfiedFlagsNoLock();
-  MojoResult rv = ConsumerEndReadDataImplNoLock(num_bytes_read);
+  MojoResult rv;
+  if (num_bytes_read > consumer_two_phase_max_num_bytes_read_ ||
+      num_bytes_read % element_num_bytes_ != 0) {
+    rv = MOJO_RESULT_INVALID_ARGUMENT;
+    consumer_two_phase_max_num_bytes_read_ = 0;
+  } else {
+    rv = ConsumerEndReadDataImplNoLock(num_bytes_read);
+  }
   // Two-phase read ended even on failure.
   DCHECK(!consumer_in_two_phase_read_no_lock());
   // If we're now readable, we *became* readable (since we weren't readable
diff --git a/mojo/system/local_data_pipe.cc b/mojo/system/local_data_pipe.cc
index 2a1549c..942e4be 100644
--- a/mojo/system/local_data_pipe.cc
+++ b/mojo/system/local_data_pipe.cc
@@ -143,12 +143,8 @@ MojoResult LocalDataPipe::ProducerBeginWriteDataImplNoLock(
 
 MojoResult LocalDataPipe::ProducerEndWriteDataImplNoLock(
     uint32_t num_bytes_written) {
-  if (num_bytes_written > producer_two_phase_max_num_bytes_written_no_lock()) {
-    // Note: The two-phase write ends here even on failure.
-    set_producer_two_phase_max_num_bytes_written_no_lock(0);
-    return MOJO_RESULT_INVALID_ARGUMENT;
-  }
-
+  DCHECK_LE(num_bytes_written,
+            producer_two_phase_max_num_bytes_written_no_lock());
   current_num_bytes_ += num_bytes_written;
   DCHECK_LE(current_num_bytes_, capacity_num_bytes());
   set_producer_two_phase_max_num_bytes_written_no_lock(0);
@@ -275,12 +271,7 @@ MojoResult LocalDataPipe::ConsumerBeginReadDataImplNoLock(
 
 MojoResult LocalDataPipe::ConsumerEndReadDataImplNoLock(
     uint32_t num_bytes_read) {
-  if (num_bytes_read > consumer_two_phase_max_num_bytes_read_no_lock()) {
-    // Note: The two-phase read ends here even on failure.
-    set_consumer_two_phase_max_num_bytes_read_no_lock(0);
-    return MOJO_RESULT_INVALID_ARGUMENT;
-  }
-
+  DCHECK_LE(num_bytes_read, consumer_two_phase_max_num_bytes_read_no_lock());
   DCHECK_LE(start_index_ + num_bytes_read, capacity_num_bytes());
   MarkDataAsConsumedNoLock(num_bytes_read);
   set_consumer_two_phase_max_num_bytes_read_no_lock(0);
diff --git a/mojo/system/local_data_pipe_unittest.cc b/mojo/system/local_data_pipe_unittest.cc
index 37babdf..5fd45a2 100644
--- a/mojo/system/local_data_pipe_unittest.cc
+++ b/mojo/system/local_data_pipe_unittest.cc
@@ -1090,6 +1090,14 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
   EXPECT_EQ(MOJO_RESULT_OUT_OF_RANGE,
             dp->ProducerBeginWriteData(&write_ptr, &num_bytes, true));
 
+  // Try writing an amount which isn't a multiple of the element size
+  // (two-phase).
+  COMPILE_ASSERT(sizeof(int32_t) > 1u, wow_int32_ts_have_size_1);
+  num_bytes = 1u;
+  write_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
+            dp->ProducerBeginWriteData(&write_ptr, &num_bytes, true));
+
   // Try reading way too much (two-phase).
   num_bytes = 20u * sizeof(int32_t);
   const void* read_ptr = NULL;
@@ -1107,6 +1115,13 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
   Seq(0, 5, static_cast<int32_t*>(write_ptr));
   EXPECT_EQ(MOJO_RESULT_OK, dp->ProducerEndWriteData(5u * sizeof(int32_t)));
 
+  // Try reading an amount which isn't a multiple of the element size
+  // (two-phase).
+  num_bytes = 1u;
+  read_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
+            dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
+
   // Read one (two-phase).
   num_bytes = 1u * sizeof(int32_t);
   read_ptr = NULL;
@@ -1431,6 +1446,119 @@ TEST(LocalDataPipeTest, CloseWriteRead) {
   }
 }
 
+TEST(LocalDataPipeTest, TwoPhaseMoreInvalidArguments) {
+  const MojoCreateDataPipeOptions options = {
+    kSizeOfOptions,  // |struct_size|.
+    MOJO_CREATE_DATA_PIPE_OPTIONS_FLAG_NONE,  // |flags|.
+    static_cast<uint32_t>(sizeof(int32_t)),  // |element_num_bytes|.
+    10 * sizeof(int32_t)  // |capacity_num_bytes|.
+  };
+  MojoCreateDataPipeOptions validated_options = { 0 };
+  EXPECT_EQ(MOJO_RESULT_OK,
+            DataPipe::ValidateOptions(&options, &validated_options));
+
+  scoped_refptr<LocalDataPipe> dp(new LocalDataPipe(validated_options));
+
+  // No data.
+  uint32_t num_bytes = 1000u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(0u, num_bytes);
+
+  // Try "ending" a two-phase write when one isn't active.
+  EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION,
+            dp->ProducerEndWriteData(1u * sizeof(int32_t)));
+
+  // Still no data.
+  num_bytes = 1000u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(0u, num_bytes);
+
+  // Try ending a two-phase write with an invalid amount (too much).
+  num_bytes = 0u;
+  void* write_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            dp->ProducerBeginWriteData(&write_ptr, &num_bytes, false));
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
+            dp->ProducerEndWriteData(
+                num_bytes + static_cast<uint32_t>(sizeof(int32_t))));
+
+  // But the two-phase write still ended.
+  EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION, dp->ProducerEndWriteData(0u));
+
+  // Still no data.
+  num_bytes = 1000u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(0u, num_bytes);
+
+  // Try ending a two-phase write with an invalid amount (not a multiple of the
+  // element size).
+  num_bytes = 0u;
+  write_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            dp->ProducerBeginWriteData(&write_ptr, &num_bytes, false));
+  EXPECT_GE(num_bytes, 1u);
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT, dp->ProducerEndWriteData(1u));
+
+  // But the two-phase write still ended.
+  EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION, dp->ProducerEndWriteData(0u));
+
+  // Still no data.
+  num_bytes = 1000u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(0u, num_bytes);
+
+  // Now write some data, so we'll be able to try reading.
+  int32_t element = 123;
+  num_bytes = 1u * sizeof(int32_t);
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ProducerWriteData(&element, &num_bytes, false));
+
+  // One element available.
+  num_bytes = 0u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
+
+  // Try "ending" a two-phase read when one isn't active.
+  EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION,
+            dp->ConsumerEndReadData(1u * sizeof(int32_t)));
+
+  // Still one element available.
+  num_bytes = 0u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
+
+  // Try ending a two-phase read with an invalid amount (too much).
+  num_bytes = 0u;
+  const void* read_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
+            dp->ConsumerEndReadData(
+                num_bytes + static_cast<uint32_t>(sizeof(int32_t))));
+
+  // Still one element available.
+  num_bytes = 0u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
+
+  // Try ending a two-phase read with an invalid amount (not a multiple of the
+  // element size).
+  num_bytes = 0u;
+  read_ptr = NULL;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
+  EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
+  EXPECT_EQ(123, static_cast<const int32_t*>(read_ptr)[0]);
+  EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT, dp->ConsumerEndReadData(1u));
+
+  // Still one element available.
+  num_bytes = 0u;
+  EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerQueryData(&num_bytes));
+  EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
+
+  dp->ProducerClose();
+  dp->ConsumerClose();
+}
+
 }  // namespace
 }  // namespace system
 }  // namespace mojo