From 33042573db3a225369e333881a33ea4ebe090979 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 5 Jul 2017 12:02:00 -0700 Subject: [PATCH] Fix GetCurrentTime() initialization for valgrind Summary: Valgrind had false positive complaints about the initialization pattern for `GetCurrentTime()`'s argument in #2480. We can instead have the client initialize the time variable before calling `GetCurrentTime()`, and have `GetCurrentTime()` promise to only overwrite it in success case. Closes https://github.com/facebook/rocksdb/pull/2526 Differential Revision: D5358689 Pulled By: ajkr fbshipit-source-id: 857b189f24c19196f6bb299216f3e23e7bc4be42 --- db/compaction_job.cc | 7 ++----- db/db_impl_open.cc | 7 ++----- db/flush_job.cc | 7 ++----- db/repair.cc | 7 ++----- env/mock_env.cc | 6 ++++-- include/rocksdb/env.h | 1 + tools/dump/db_dump_tool.cc | 2 +- utilities/date_tiered/date_tiered_test.cc | 2 +- utilities/ttl/ttl_test.cc | 2 +- 9 files changed, 16 insertions(+), 25 deletions(-) diff --git a/db/compaction_job.cc b/db/compaction_job.cc index 4dc9574ea..407671e85 100644 --- a/db/compaction_job.cc +++ b/db/compaction_job.cc @@ -1283,11 +1283,8 @@ Status CompactionJob::OpenCompactionOutputFile( uint64_t output_file_creation_time = sub_compact->compaction->MaxInputFileCreationTime(); if (output_file_creation_time == 0) { - int64_t _current_time; - auto status = db_options_.env->GetCurrentTime(&_current_time); - if (!status.ok()) { - _current_time = 0; - } + int64_t _current_time = 0; + db_options_.env->GetCurrentTime(&_current_time); // ignore error output_file_creation_time = static_cast(_current_time); } diff --git a/db/db_impl_open.cc b/db/db_impl_open.cc index 754fc2ae6..e8d8c9106 100644 --- a/db/db_impl_open.cc +++ b/db/db_impl_open.cc @@ -854,11 +854,8 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd, bool paranoid_file_checks = cfd->GetLatestMutableCFOptions()->paranoid_file_checks; - int64_t _current_time; - s = env_->GetCurrentTime(&_current_time); - if (!s.ok()) { - _current_time = 0; - } + int64_t _current_time = 0; + env_->GetCurrentTime(&_current_time); // ignore error const uint64_t current_time = static_cast(_current_time); { diff --git a/db/flush_job.cc b/db/flush_job.cc index d93e2c64e..57a6ca731 100644 --- a/db/flush_job.cc +++ b/db/flush_job.cc @@ -299,11 +299,8 @@ Status FlushJob::WriteLevel0Table() { EnvOptions optimized_env_options = db_options_.env->OptimizeForCompactionTableWrite(env_options_, db_options_); - int64_t _current_time; - auto status = db_options_.env->GetCurrentTime(&_current_time); - if (!status.ok()) { - _current_time = 0; - } + int64_t _current_time = 0; + db_options_.env->GetCurrentTime(&_current_time); // ignore error const uint64_t current_time = static_cast(_current_time); s = BuildTable( diff --git a/db/repair.cc b/db/repair.cc index 943c4fc0e..58668bf41 100644 --- a/db/repair.cc +++ b/db/repair.cc @@ -383,11 +383,8 @@ class Repairer { EnvOptions optimized_env_options = env_->OptimizeForCompactionTableWrite(env_options_, immutable_db_options_); - int64_t _current_time; - status = env_->GetCurrentTime(&_current_time); - if (!status.ok()) { - _current_time = 0; - } + int64_t _current_time = 0; + status = env_->GetCurrentTime(&_current_time); // ignore error const uint64_t current_time = static_cast(_current_time); status = BuildTable( diff --git a/env/mock_env.cc b/env/mock_env.cc index c195e6c23..16cd0a66c 100644 --- a/env/mock_env.cc +++ b/env/mock_env.cc @@ -148,7 +148,7 @@ class MemFile { private: uint64_t Now() { - int64_t unix_time; + int64_t unix_time = 0; auto s = env_->GetCurrentTime(&unix_time); assert(s.ok()); return static_cast(unix_time); @@ -734,7 +734,9 @@ Status MockEnv::GetTestDirectory(std::string* path) { Status MockEnv::GetCurrentTime(int64_t* unix_time) { auto s = EnvWrapper::GetCurrentTime(unix_time); - *unix_time += fake_sleep_micros_.load() / (1000 * 1000); + if (s.ok()) { + *unix_time += fake_sleep_micros_.load() / (1000 * 1000); + } return s; } diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index faa1b0d6e..682fc65c5 100644 --- a/include/rocksdb/env.h +++ b/include/rocksdb/env.h @@ -353,6 +353,7 @@ class Env { virtual Status GetHostName(char* name, uint64_t len) = 0; // Get the number of seconds since the Epoch, 1970-01-01 00:00:00 (UTC). + // Only overwrites *unix_time on success. virtual Status GetCurrentTime(int64_t* unix_time) = 0; // Get full directory name for this db. diff --git a/tools/dump/db_dump_tool.cc b/tools/dump/db_dump_tool.cc index 18ab60de4..ea332e19e 100644 --- a/tools/dump/db_dump_tool.cc +++ b/tools/dump/db_dump_tool.cc @@ -27,7 +27,7 @@ bool DbDumpTool::Run(const DumpOptions& dump_options, rocksdb::Status status; std::unique_ptr dumpfile; char hostname[1024]; - int64_t timesec; + int64_t timesec = 0; std::string abspath; char json[4096]; diff --git a/utilities/date_tiered/date_tiered_test.cc b/utilities/date_tiered/date_tiered_test.cc index 55b72517f..55e3f622d 100644 --- a/utilities/date_tiered/date_tiered_test.cc +++ b/utilities/date_tiered/date_tiered_test.cc @@ -37,7 +37,7 @@ class SpecialTimeEnv : public EnvWrapper { } private: - int64_t current_time_; + int64_t current_time_ = 0; }; class DateTieredTest : public testing::Test { diff --git a/utilities/ttl/ttl_test.cc b/utilities/ttl/ttl_test.cc index 233946060..203008688 100644 --- a/utilities/ttl/ttl_test.cc +++ b/utilities/ttl/ttl_test.cc @@ -36,7 +36,7 @@ class SpecialTimeEnv : public EnvWrapper { } private: - int64_t current_time_; + int64_t current_time_ = 0; }; class TtlTest : public testing::Test {