MB-20623: Warmup: Implement MutationLog::iterator copy assignment

MutationLog::iterator doesn't follow the Rule of Three - it doesn't
implement the copy-assigment operator. This means that it's not a
complete iterator implementation.

Fix this, and add a unit test for it.

Change-Id: I12d67bc072d72e481e6a195e2d45b16c0318fdc0
Reviewed-on: http://review.couchbase.org/68044
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>

diff --git a/src/mutation_log.cc b/src/mutation_log.cc
index 239117e..c0cfbd5 100644 (file)
--- a/src/mutation_log.cc
+++ b/src/mutation_log.cc
@@ -735,12 +735,12 @@ static const char* logType(uint8_t t) {
 // Mutation log iterator
 // ----------------------------------------------------------------------
 
-MutationLog::iterator::iterator(const MutationLog& l, bool e)
+MutationLog::iterator::iterator(const MutationLog* l, bool e)
   : log(l),
     entryBuf(),
     buf(),
     p(nullptr),
-    offset(l.header().blockSize() * l.header().blockCount()),
+    offset(l->header().blockSize() * l->header().blockCount()),
     items(0),
     isEnd(e)
 {
@@ -756,8 +756,8 @@ MutationLog::iterator::iterator(const MutationLog::iterator& mit)
     isEnd(mit.isEnd)
 {
     if (mit.buf != NULL) {
-        buf.reset(new uint8_t[log.header().blockSize()]());
-        memcpy(buf.get(), mit.buf.get(), log.header().blockSize());
+        buf.reset(new uint8_t[log->header().blockSize()]());
+        memcpy(buf.get(), mit.buf.get(), log->header().blockSize());
         p = buf.get() + (mit.p - mit.buf.get());
     }
 
@@ -767,6 +767,32 @@ MutationLog::iterator::iterator(const MutationLog::iterator& mit)
     }
 }
 
+MutationLog::iterator& MutationLog::iterator::operator=(const MutationLog::iterator& other)
+{
+    log = other.log;
+    if (other.buf != nullptr) {
+        buf.reset(new uint8_t[log->header().blockSize()]());
+        memcpy(buf.get(), other.buf.get(), log->header().blockSize());
+        p = buf.get() + (other.p - other.buf.get());
+    } else {
+        buf = nullptr;
+        p = nullptr;
+    }
+
+    if (other.entryBuf != nullptr) {
+        entryBuf.reset(new uint8_t[LOG_ENTRY_BUF_SIZE]());
+        memcpy(entryBuf.get(), other.entryBuf.get(), LOG_ENTRY_BUF_SIZE);
+    } else {
+        entryBuf = nullptr;
+    }
+
+    offset = other.offset;
+    items = other.items;
+    isEnd = other.isEnd;
+
+    return *this;
+}
+
 MutationLog::iterator::~iterator() {
 }
 
@@ -792,7 +818,7 @@ MutationLog::iterator& MutationLog::iterator::operator++() {
 }
 
 bool MutationLog::iterator::operator==(const MutationLog::iterator& rhs) const {
-    return log.fd() == rhs.log.fd()
+    return log->fd() == rhs.log->fd()
         && (
             (isEnd == rhs.isEnd)
             || (offset == rhs.offset
@@ -812,34 +838,34 @@ const MutationLogEntry* MutationLog::iterator::operator*() {
 }
 
 size_t MutationLog::iterator::bufferBytesRemaining() {
-    return log.header().blockSize() - (p - buf.get());
+    return log->header().blockSize() - (p - buf.get());
 }
 
 void MutationLog::iterator::nextBlock() {
-    if (log.isEnabled() && !log.isOpen()) {
+    if (log->isEnabled() && !log->isOpen()) {
         throw std::logic_error("MutationLog::iterator::nextBlock: "
                 "log is enabled and not open");
     }
 
     if (buf == NULL) {
-        buf.reset(new uint8_t[log.header().blockSize()]());
+        buf.reset(new uint8_t[log->header().blockSize()]());
     }
     p = buf.get();
 
-    ssize_t bytesread = pread(log.fd(), buf.get(), log.header().blockSize(),
+    ssize_t bytesread = pread(log->fd(), buf.get(), log->header().blockSize(),
                               offset);
     if (bytesread < 1) {
         isEnd = true;
         return;
     }
-    if (bytesread != (ssize_t)(log.header().blockSize())) {
+    if (bytesread != (ssize_t)(log->header().blockSize())) {
         LOG(EXTENSION_LOG_WARNING, "FATAL: too few bytes read in access log"
-                "'%s': %s", log.getLogFile().c_str(), strerror(errno));
+                "'%s': %s", log->getLogFile().c_str(), strerror(errno));
         throw ShortReadException();
     }
     offset += bytesread;
 
-    uint32_t crc32(crc32buf(buf.get() + 2, log.header().blockSize() - 2));
+    uint32_t crc32(crc32buf(buf.get() + 2, log->header().blockSize() - 2));
     uint16_t computed_crc16(crc32 & 0xffff);
     uint16_t retrieved_crc16;
     memcpy(&retrieved_crc16, buf.get(), sizeof(retrieved_crc16));

diff --git a/src/mutation_log.h b/src/mutation_log.h
index 11199e1..d3e2638 100644 (file)
--- a/src/mutation_log.h
+++ b/src/mutation_log.h
@@ -469,6 +469,8 @@ public:
 
         iterator(const iterator& mit);
 
+        iterator& operator=(const iterator& other);
+
         ~iterator();
 
         iterator& operator++();
@@ -483,13 +485,13 @@ public:
 
         friend class MutationLog;
 
-        iterator(const MutationLog& l, bool e=false);
+        iterator(const MutationLog* l, bool e=false);
 
         void nextBlock();
         size_t bufferBytesRemaining();
         void prepItem();
 
-        const MutationLog& log;
+        const MutationLog* log;
         std::unique_ptr<uint8_t[]> entryBuf;
         std::unique_ptr<uint8_t[]> buf;
         uint8_t           *p;
@@ -502,7 +504,7 @@ public:
      * An iterator pointing to the beginning of the log file.
      */
     iterator begin() {
-        iterator it(iterator(*this));
+        iterator it(iterator(this));
         it.nextBlock();
         return it;
     }
@@ -511,7 +513,7 @@ public:
      * An iterator pointing at the end of the log file.
      */
     iterator end() {
-        return iterator(*this, true);
+        return iterator(this, true);
     }
 
     //! Items logged by type.

diff --git a/tests/module_tests/mutation_log_test.cc b/tests/module_tests/mutation_log_test.cc
index 32aa60f..a82730c 100644 (file)
--- a/tests/module_tests/mutation_log_test.cc
+++ b/tests/module_tests/mutation_log_test.cc
@@ -421,6 +421,43 @@ TEST_F(MutationLogTest, YUNOOPEN) {
     set_file_perms(FilePerms::Read | FilePerms::Write);
 }
 
+// Test that the MutationLog::iterator class obeys expected iterator behaviour.
+TEST_F(MutationLogTest, Iterator) {
+    // Create a simple mutation log to work on.
+    {
+        MutationLog ml(tmp_log_filename.c_str());
+        ml.open();
+        ml.newItem(0, "key1", 0);
+        ml.newItem(0, "key2", 1);
+        ml.newItem(0, "key3", 2);
+        ml.commit1();
+        ml.commit2();
+
+        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
+        EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT1]);
+        EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT2]);
+    }
+
+    // Now check the iterators.
+    MutationLog ml(tmp_log_filename.c_str());
+    ml.open();
+
+    // Can copy-construct.
+    auto iter = ml.begin();
+    EXPECT_EQ(ml.begin(), iter);
+
+    // Can copy-assign.
+    iter = ml.end();
+    EXPECT_EQ(ml.end(), iter);
+
+    // Can advance the correct number.
+    size_t count = 0;
+    for (auto iter2 = ml.begin(); iter2 != ml.end(); ++iter2) {
+        count++;
+    }
+    EXPECT_EQ(5, count);
+}
+
 // @todo
 //   Test Read Only log
 //   Test close / open / close / open