From 5b278b53ae89ce6506487b79cb07060e770da0b1 Mon Sep 17 00:00:00 2001 From: Mayank Agarwal Date: Fri, 8 Mar 2013 12:29:19 -0800 Subject: [PATCH] Fix valgrind errors in rocksdb tests: auto_roll_logger_test, reduce_levels_test Summary: Fix for memory leaks in rocksdb tests. Also modified the variable NUM_FAILED_TESTS to print the actual number of failed tests. Test Plan: make ; valgrind --leak-check=full ./ Reviewers: sheki, dhruba Reviewed By: sheki CC: leveldb Differential Revision: https://reviews.facebook.net/D9333 --- db/version_set_reduce_num_levels.cc | 3 ++- util/auto_roll_logger_test.cc | 20 +++++++------------- util/ldb_cmd.cc | 29 ++++++++++++----------------- valgrind_test.sh | 2 +- 4 files changed, 22 insertions(+), 32 deletions(-) diff --git a/db/version_set_reduce_num_levels.cc b/db/version_set_reduce_num_levels.cc index b110ef6f2..574a45c44 100644 --- a/db/version_set_reduce_num_levels.cc +++ b/db/version_set_reduce_num_levels.cc @@ -67,7 +67,8 @@ Status VersionSet::ReduceNumberOfLevels(int new_levels, port::Mutex* mu) { num_levels_ = new_levels; compact_pointer_ = new std::string[new_levels]; Init(new_levels); - st = LogAndApply(new VersionEdit(new_levels), mu, true); + VersionEdit ve(new_levels); + st = LogAndApply(&ve , mu, true); return st; } diff --git a/util/auto_roll_logger_test.cc b/util/auto_roll_logger_test.cc index 4b12fb857..63764d485 100755 --- a/util/auto_roll_logger_test.cc +++ b/util/auto_roll_logger_test.cc @@ -122,13 +122,11 @@ TEST(AutoRollLoggerTest, RollLogFileBySize) { InitTestDb(); size_t log_max_size = 1024 * 5; - AutoRollLogger* logger = new AutoRollLogger( - Env::Default(), kTestDir, "", log_max_size, 0); + AutoRollLogger logger(Env::Default(), kTestDir, "", log_max_size, 0); - RollLogFileBySizeTest(logger, log_max_size, + RollLogFileBySizeTest(&logger, log_max_size, kSampleMessage + ":RollLogFileBySize"); - delete logger; } TEST(AutoRollLoggerTest, RollLogFileByTime) { @@ -138,13 +136,10 @@ TEST(AutoRollLoggerTest, RollLogFileByTime) { InitTestDb(); // -- Test the existence of file during the server restart. ASSERT_TRUE(!env->FileExists(kLogFile)); - AutoRollLogger* logger = new AutoRollLogger( - Env::Default(), kTestDir, "", log_size, 1); + AutoRollLogger logger(Env::Default(), kTestDir, "", log_size, 1); ASSERT_TRUE(env->FileExists(kLogFile)); - RollLogFileByTimeTest(logger, time, kSampleMessage + ":RollLogFileByTime"); - - delete logger; + RollLogFileByTimeTest(&logger, time, kSampleMessage + ":RollLogFileByTime"); } TEST(AutoRollLoggerTest, @@ -178,16 +173,15 @@ TEST(AutoRollLoggerTest, CompositeRollByTimeAndSizeLogger) { InitTestDb(); - AutoRollLogger* logger = new AutoRollLogger( - Env::Default(), kTestDir, "", log_max_size, time); + AutoRollLogger logger(Env::Default(), kTestDir, "", log_max_size, time); // Test the ability to roll by size RollLogFileBySizeTest( - logger, log_max_size, + &logger, log_max_size, kSampleMessage + ":CompositeRollByTimeAndSizeLogger"); // Test the ability to roll by Time - RollLogFileByTimeTest( logger, time, + RollLogFileByTimeTest( &logger, time, kSampleMessage + ":CompositeRollByTimeAndSizeLogger"); } diff --git a/util/ldb_cmd.cc b/util/ldb_cmd.cc index 3553527cd..0c15cf64b 100644 --- a/util/ldb_cmd.cc +++ b/util/ldb_cmd.cc @@ -392,7 +392,7 @@ void DBLoaderCommand::DoCommand() { std::cout << "Warning: " << bad_lines << " bad lines ignored." << std::endl; } if (compact_) { - db_->CompactRange(NULL, NULL); + db_->CompactRange(nullptr, nullptr); } } @@ -560,27 +560,24 @@ leveldb::Options ReduceDBLevelsCommand::PrepareOptionsForOpenDB() { Status ReduceDBLevelsCommand::GetOldNumOfLevels(leveldb::Options& opt, int* levels) { - TableCache* tc = new TableCache(db_path_, &opt, 10); - const InternalKeyComparator* cmp = new InternalKeyComparator( - opt.comparator); - VersionSet* versions = new VersionSet(db_path_, &opt, - tc, cmp); + TableCache tc(db_path_, &opt, 10); + const InternalKeyComparator cmp(opt.comparator); + VersionSet versions(db_path_, &opt, &tc, &cmp); // We rely the VersionSet::Recover to tell us the internal data structures // in the db. And the Recover() should never do any change // (like LogAndApply) to the manifest file. - Status st = versions->Recover(); + Status st = versions.Recover(); if (!st.ok()) { return st; } int max = -1; - for (int i = 0; i < versions->NumberLevels(); i++) { - if (versions->NumLevelFiles(i)) { + for (int i = 0; i < versions.NumberLevels(); i++) { + if (versions.NumLevelFiles(i)) { max = i; } } *levels = max + 1; - delete versions; return st; } @@ -619,15 +616,13 @@ void ReduceDBLevelsCommand::DoCommand() { db_->CompactRange(nullptr, nullptr); CloseDB(); - TableCache* tc = new TableCache(db_path_, &opt, 10); - const InternalKeyComparator* cmp = new InternalKeyComparator( - opt.comparator); - VersionSet* versions = new VersionSet(db_path_, &opt, - tc, cmp); + TableCache tc(db_path_, &opt, 10); + const InternalKeyComparator cmp(opt.comparator); + VersionSet versions(db_path_, &opt, &tc, &cmp); // We rely the VersionSet::Recover to tell us the internal data structures // in the db. And the Recover() should never do any change (like LogAndApply) // to the manifest file. - st = versions->Recover(); + st = versions.Recover(); if (!st.ok()) { exec_state_ = LDBCommandExecuteResult::FAILED(st.ToString()); return; @@ -635,7 +630,7 @@ void ReduceDBLevelsCommand::DoCommand() { port::Mutex mu; mu.Lock(); - st = versions->ReduceNumberOfLevels(new_levels_, &mu); + st = versions.ReduceNumberOfLevels(new_levels_, &mu); mu.Unlock(); if (!st.ok()) { diff --git a/valgrind_test.sh b/valgrind_test.sh index 95c3af114..88692fc6b 100755 --- a/valgrind_test.sh +++ b/valgrind_test.sh @@ -4,7 +4,7 @@ VALGRIND_DIR=VALGRIND_LOGS make -j$(nproc) valgrind_check -NUM_FAILED_TESTS=`wc -l $VALGRIND_DIR/valgrind_failed_tests | awk '{print $1}'` +NUM_FAILED_TESTS=$((`wc -l $VALGRIND_DIR/valgrind_failed_tests | awk '{print $1}'` - 1)) if [ $NUM_FAILED_TESTS -le 1 ]; then echo No tests have valgrind errors exit 0