From: Adam Simpkins <simpkins@fb.com>
Date: Sun, 24 Nov 2013 23:07:33 +0000 (-0800)
Subject: fix bugs in the cursor StringOperations test
X-Git-Tag: v0.22.0~785
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=efc30713168057144e45a0bdea094d1025c6e9cc;p=folly.git

fix bugs in the cursor StringOperations test

Summary:
The IOBuf.StringOperations test contained two buggy tests.  According to
the comments, they were intended to test a string spanning multiple
buffers.  They created a chain of two buffers, but used Appender to
write the string, which only writes in the final buffer in the chain.

Additionally, the test didn't request enough space in the final buffer
for the string data, and didn't allow Appender to grow the buffer.  (The
code also didn't check the return value from pushAtMost() to see if it
actually wrote all of the string data.)  The test only passed because
IOBuf would normally allocate more capacity than the caller requested.

I fixed the tests to request sufficient capacity for the string data,
and use push() rather than pushAtMost() to ensure that all data was
written correctly.  I also added two new tests which actually test with
strings spanning multiple buffers.

Test Plan:
Ran this test with some changes to IOBuf that cause it to only allocate
the requested capacity in some cases, and verified that the
StringOperations test pass now.

(The changes I was testing with use goodMallocSize() to determine the
allocation size even for small internal buffers.  When not using
jemalloc, goodMallocSize() returns the original size, so it only
allocates the capacity requested by the caller.)

Reviewed By: pgriess@fb.com

FB internal diff: D1072283
---

diff --git a/folly/io/test/IOBufCursorTest.cpp b/folly/io/test/IOBufCursorTest.cpp
index 54536e67..d349241b 100644
--- a/folly/io/test/IOBufCursorTest.cpp
+++ b/folly/io/test/IOBufCursorTest.cpp
@@ -415,7 +415,7 @@ TEST(IOBuf, StringOperations) {
   {
     std::unique_ptr<IOBuf> chain(IOBuf::create(16));
     Appender app(chain.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello\0world\0\x01"), 13);
+    app.push(reinterpret_cast<const uint8_t*>("hello\0world\0\x01"), 13);
 
     Cursor curs(chain.get());
     EXPECT_STREQ("hello", curs.readTerminatedString().c_str());
@@ -423,12 +423,26 @@ TEST(IOBuf, StringOperations) {
     EXPECT_EQ(1, curs.read<uint8_t>());
   }
 
+  // Test multiple buffers where the first is empty and the string starts in
+  // the second buffer.
+  {
+    std::unique_ptr<IOBuf> chain(IOBuf::create(8));
+    chain->prependChain(IOBuf::create(12));
+    Appender app(chain.get(), 0);
+    app.push(reinterpret_cast<const uint8_t*>("hello world\0"), 12);
+
+    Cursor curs(chain.get());
+    EXPECT_STREQ("hello world", curs.readTerminatedString().c_str());
+  }
+
   // Test multiple buffers with a single null-terminated string spanning them
   {
     std::unique_ptr<IOBuf> chain(IOBuf::create(8));
     chain->prependChain(IOBuf::create(8));
-    Appender app(chain.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello world\0"), 12);
+    chain->append(8);
+    chain->next()->append(4);
+    RWPrivateCursor rwc(chain.get());
+    rwc.push(reinterpret_cast<const uint8_t*>("hello world\0"), 12);
 
     Cursor curs(chain.get());
     EXPECT_STREQ("hello world", curs.readTerminatedString().c_str());
@@ -439,18 +453,18 @@ TEST(IOBuf, StringOperations) {
   {
     std::unique_ptr<IOBuf> chain(IOBuf::create(16));
     Appender app(chain.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello world\0"), 12);
+    app.push(reinterpret_cast<const uint8_t*>("hello world\0"), 12);
 
     Cursor curs(chain.get());
     EXPECT_THROW(curs.readTerminatedString('\0', 5), std::length_error);
   }
 
-  // Test reading a null-termianted string from a chain with an empty buffer at
+  // Test reading a null-terminated string from a chain with an empty buffer at
   // the front
   {
     std::unique_ptr<IOBuf> buf(IOBuf::create(8));
     Appender app(buf.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello\0"), 6);
+    app.push(reinterpret_cast<const uint8_t*>("hello\0"), 6);
     std::unique_ptr<IOBuf> chain(IOBuf::create(8));
     chain->prependChain(std::move(buf));
 
@@ -463,7 +477,7 @@ TEST(IOBuf, StringOperations) {
   {
     std::unique_ptr<IOBuf> chain(IOBuf::create(16));
     Appender app(chain.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("helloworld\x01"), 11);
+    app.push(reinterpret_cast<const uint8_t*>("helloworld\x01"), 11);
 
     Cursor curs(chain.get());
     EXPECT_STREQ("hello", curs.readFixedString(5).c_str());
@@ -471,12 +485,26 @@ TEST(IOBuf, StringOperations) {
     EXPECT_EQ(1, curs.read<uint8_t>());
   }
 
+  // Test multiple buffers where the first is empty and a fixed-length string
+  // starts in the second buffer.
+  {
+    std::unique_ptr<IOBuf> chain(IOBuf::create(8));
+    chain->prependChain(IOBuf::create(16));
+    Appender app(chain.get(), 0);
+    app.push(reinterpret_cast<const uint8_t*>("hello world"), 11);
+
+    Cursor curs(chain.get());
+    EXPECT_STREQ("hello world", curs.readFixedString(11).c_str());
+  }
+
   // Test multiple buffers with a single fixed-length string spanning them
   {
     std::unique_ptr<IOBuf> chain(IOBuf::create(8));
     chain->prependChain(IOBuf::create(8));
-    Appender app(chain.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello world"), 11);
+    chain->append(7);
+    chain->next()->append(4);
+    RWPrivateCursor rwc(chain.get());
+    rwc.push(reinterpret_cast<const uint8_t*>("hello world"), 11);
 
     Cursor curs(chain.get());
     EXPECT_STREQ("hello world", curs.readFixedString(11).c_str());
@@ -487,7 +515,7 @@ TEST(IOBuf, StringOperations) {
   {
     std::unique_ptr<IOBuf> buf(IOBuf::create(8));
     Appender app(buf.get(), 0);
-    app.pushAtMost(reinterpret_cast<const uint8_t*>("hello"), 5);
+    app.push(reinterpret_cast<const uint8_t*>("hello"), 5);
     std::unique_ptr<IOBuf> chain(IOBuf::create(8));
     chain->prependChain(std::move(buf));