From df49134e1a46971c60e36d8a8e41166d3255363b Mon Sep 17 00:00:00 2001 From: mwish Date: Thu, 19 Oct 2023 22:29:15 +0800 Subject: [PATCH 1/4] style enhancement for rdb --- src/common/rdb_stream.cc | 9 ++++----- src/common/rdb_stream.h | 6 +++--- src/storage/rdb.cc | 24 ++++++++++++++---------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/common/rdb_stream.cc b/src/common/rdb_stream.cc index 83855ea6d64..bfcaae2ee41 100644 --- a/src/common/rdb_stream.cc +++ b/src/common/rdb_stream.cc @@ -23,13 +23,13 @@ #include "fmt/format.h" #include "vendor/crc64.h" -StatusOr RdbStringStream::Read(char *buf, size_t n) { +Status RdbStringStream::Read(char *buf, size_t n) { if (pos_ + n > input_.size()) { return {Status::NotOK, "unexpected EOF"}; } memcpy(buf, input_.data() + pos_, n); pos_ += n; - return n; + return Status::OK(); } StatusOr RdbStringStream::GetCheckSum() const { @@ -59,11 +59,10 @@ StatusOr RdbFileStream::Read(char *buf, size_t len) { return Status(Status::NotOK, fmt::format("read failed: {}:", strerror(errno))); } check_sum_ = crc64(check_sum_, reinterpret_cast(buf), read_bytes); - buf = (char *)buf + read_bytes; + buf = buf + read_bytes; len -= read_bytes; total_read_bytes_ += read_bytes; n += read_bytes; } - - return n; + return Status::OK(); } diff --git a/src/common/rdb_stream.h b/src/common/rdb_stream.h index 842d24a7a6b..c67a1b468dd 100644 --- a/src/common/rdb_stream.h +++ b/src/common/rdb_stream.h @@ -32,7 +32,7 @@ class RdbStream { RdbStream() = default; virtual ~RdbStream() = default; - virtual StatusOr Read(char *buf, size_t len) = 0; + virtual Status Read(char *buf, size_t len) = 0; virtual StatusOr GetCheckSum() const = 0; StatusOr ReadByte() { uint8_t value = 0; @@ -51,7 +51,7 @@ class RdbStringStream : public RdbStream { RdbStringStream &operator=(const RdbStringStream &) = delete; ~RdbStringStream() override = default; - StatusOr Read(char *buf, size_t len) override; + Status Read(char *buf, size_t len) override; StatusOr GetCheckSum() const override; private: @@ -68,7 +68,7 @@ class RdbFileStream : public RdbStream { ~RdbFileStream() override = default; Status Open(); - StatusOr Read(char *buf, size_t len) override; + Status Read(char *buf, size_t len) override; StatusOr GetCheckSum() const override { uint64_t crc = check_sum_; memrev64ifbe(&crc); diff --git a/src/storage/rdb.cc b/src/storage/rdb.cc index c73c172ef45..bd433a4d860 100644 --- a/src/storage/rdb.cc +++ b/src/storage/rdb.cc @@ -173,9 +173,13 @@ StatusOr RDB::loadEncodedString() { } // Normal string - std::vector vec(len); - GET_OR_RET(stream_->Read(vec.data(), len)); - return std::string(vec.data(), len); + if (len == 0) { + return ""; + } + std::string read_string; + read_string.resize(len); + GET_OR_RET(stream_->Read(read_string.data(), len)); + return read_string; } StatusOr> RDB::LoadListWithQuickList(int type) { @@ -196,7 +200,7 @@ StatusOr> RDB::LoadListWithQuickList(int type) { if (container == QuickListNodeContainerPlain) { auto element = GET_OR_RET(loadEncodedString()); - list.push_back(element); + list.push_back(std::move(element)); continue; } @@ -204,11 +208,11 @@ StatusOr> RDB::LoadListWithQuickList(int type) { if (type == RDBTypeListQuickList2) { ListPack lp(encoded_string); auto elements = GET_OR_RET(lp.Entries()); - list.insert(list.end(), elements.begin(), elements.end()); + list.insert(list.end(), std::make_move_iterator(elements.begin()), std::make_move_iterator(elements.end())); } else { ZipList zip_list(encoded_string); auto elements = GET_OR_RET(zip_list.Entries()); - list.insert(list.end(), elements.begin(), elements.end()); + list.insert(list.end(), std::make_move_iterator(elements.begin()), std::make_move_iterator(elements.end())); } } return list; @@ -222,7 +226,7 @@ StatusOr> RDB::LoadListObject() { } for (size_t i = 0; i < len; i++) { auto element = GET_OR_RET(loadEncodedString()); - list.push_back(element); + list.push_back(std::move(element)); } return list; } @@ -241,7 +245,7 @@ StatusOr> RDB::LoadSetObject() { } for (size_t i = 0; i < len; i++) { auto element = GET_OR_RET(LoadStringObject()); - set.push_back(element); + set.push_back(std::move(element)); } return set; } @@ -268,7 +272,7 @@ StatusOr> RDB::LoadHashObject() { for (size_t i = 0; i < len; i++) { auto field = GET_OR_RET(LoadStringObject()); auto value = GET_OR_RET(LoadStringObject()); - hash[field] = value; + hash[field] = std::move(value); } return hash; } @@ -471,7 +475,7 @@ Status RDB::saveRdbObject(int type, const std::string &key, const RedisObjValue const auto &member_scores = std::get>(obj); redis::ZSet zset_db(storage_, ns_); uint64_t count = 0; - db_status = zset_db.Add(key, ZAddFlags(0), (redis::ZSet::MemberScores *)&member_scores, &count); + db_status = zset_db.Add(key, ZAddFlags(0), const_cast *>(&member_scores), &count); } else if (type == RDBTypeHash || type == RDBTypeHashListPack || type == RDBTypeHashZipList || type == RDBTypeHashZipMap) { const auto &entries = std::get>(obj); From e35a69541a89048dacdf878626617954324ee1ea Mon Sep 17 00:00:00 2001 From: mwish Date: Thu, 19 Oct 2023 22:51:53 +0800 Subject: [PATCH 2/4] fix lint --- src/common/rdb_stream.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/common/rdb_stream.cc b/src/common/rdb_stream.cc index bfcaae2ee41..0b4fc95170a 100644 --- a/src/common/rdb_stream.cc +++ b/src/common/rdb_stream.cc @@ -50,19 +50,18 @@ Status RdbFileStream::Open() { return Status::OK(); } -StatusOr RdbFileStream::Read(char *buf, size_t len) { - size_t n = 0; +Status RdbFileStream::Read(char *buf, size_t len) { while (len) { size_t read_bytes = std::min(max_read_chunk_size_, len); ifs_.read(buf, static_cast(read_bytes)); if (!ifs_.good()) { - return Status(Status::NotOK, fmt::format("read failed: {}:", strerror(errno))); + return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))}; } check_sum_ = crc64(check_sum_, reinterpret_cast(buf), read_bytes); buf = buf + read_bytes; + DCHECK(len >= read_bytes); len -= read_bytes; total_read_bytes_ += read_bytes; - n += read_bytes; } return Status::OK(); } From 9d0e0e145d41e7555f94f793b0b2bab34991af4d Mon Sep 17 00:00:00 2001 From: mwish Date: Thu, 19 Oct 2023 23:10:41 +0800 Subject: [PATCH 3/4] fix compile in tests --- tests/cppunit/rdb_stream_test.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/cppunit/rdb_stream_test.cc b/tests/cppunit/rdb_stream_test.cc index c82a04cfac1..267703b8bed 100644 --- a/tests/cppunit/rdb_stream_test.cc +++ b/tests/cppunit/rdb_stream_test.cc @@ -29,7 +29,6 @@ TEST(RdbFileStreamOpenTest, FileNotExist) { RdbFileStream reader("not_exist.rdb"); ASSERT_FALSE(reader.Open().IsOK()); - ; } TEST(RdbFileStreamOpenTest, FileExist) { @@ -51,18 +50,18 @@ TEST(RdbFileStreamReadTest, ReadRdb) { ASSERT_TRUE(reader.Open().IsOK()); char buf[16] = {0}; - ASSERT_EQ(reader.Read(buf, 5).GetValue(), 5); + ASSERT_TRUE(reader.Read(buf, 5).IsOK()); ASSERT_EQ(strncmp(buf, "REDIS", 5), 0); size -= 5; auto len = static_cast(sizeof(buf) / sizeof(buf[0])); while (size >= len) { - ASSERT_EQ(reader.Read(buf, len).GetValue(), len); + ASSERT_TRUE(reader.Read(buf, len).IsOK()); size -= len; } if (size > 0) { - ASSERT_EQ(reader.Read(buf, size).GetValue(), size); + ASSERT_TRUE(reader.Read(buf, size).IsOK()); } } @@ -80,11 +79,11 @@ TEST(RdbFileStreamReadTest, ReadRdbLittleChunk) { auto len = static_cast(sizeof(buf) / sizeof(buf[0])); while (size >= len) { - ASSERT_EQ(reader.Read(buf, len).GetValue(), len); + ASSERT_TRUE(reader.Read(buf, len).IsOK()); size -= len; } if (size > 0) { - ASSERT_EQ(reader.Read(buf, size).GetValue(), size); + ASSERT_TRUE(reader.Read(buf, size).IsOK()); } } From ec7575c28e46b12b752d3f8b60df7622ffd18977 Mon Sep 17 00:00:00 2001 From: mwish Date: Fri, 20 Oct 2023 00:13:12 +0800 Subject: [PATCH 4/4] Extra handling EOF in RdbFileStream --- src/common/rdb_stream.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/common/rdb_stream.cc b/src/common/rdb_stream.cc index 0b4fc95170a..15896eebd90 100644 --- a/src/common/rdb_stream.cc +++ b/src/common/rdb_stream.cc @@ -55,7 +55,13 @@ Status RdbFileStream::Read(char *buf, size_t len) { size_t read_bytes = std::min(max_read_chunk_size_, len); ifs_.read(buf, static_cast(read_bytes)); if (!ifs_.good()) { - return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))}; + if (!ifs_.eof()) { + return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))}; + } + auto eof_read_bytes = static_cast(ifs_.gcount()); + if (read_bytes != eof_read_bytes) { + return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))}; + } } check_sum_ = crc64(check_sum_, reinterpret_cast(buf), read_bytes); buf = buf + read_bytes;