From 0e99323ac281e8e13c3c0e90e62d7f5360779e02 Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Thu, 7 Sep 2017 14:11:15 -0700 Subject: [PATCH] Fix CLANG Analyze Summary: clang analyze shows warnings after we upgrade the CLANG version. Fix them. Closes https://github.com/facebook/rocksdb/pull/2839 Differential Revision: D5769060 Pulled By: siying fbshipit-source-id: 3f8e4df715590d8984f6564b608fa08cfdfa5f14 --- db/compaction_job.cc | 21 +++++++++++++++------ db/db_impl.cc | 4 +--- db/db_impl.h | 5 ++--- db/db_impl_files.cc | 10 +++++----- memtable/inlineskiplist.h | 5 ++++- memtable/skiplist.h | 2 ++ utilities/backupable/backupable_db_test.cc | 2 +- 7 files changed, 30 insertions(+), 19 deletions(-) diff --git a/db/compaction_job.cc b/db/compaction_job.cc index 1d023ca45..13a43a00a 100644 --- a/db/compaction_job.cc +++ b/db/compaction_job.cc @@ -1067,16 +1067,18 @@ Status CompactionJob::FinishCompactionOutputFile( range_del_agg->AddToBuilder(sub_compact->builder.get(), lower_bound, upper_bound, meta, range_del_out_stats, bottommost_level_); + meta->marked_for_compaction = sub_compact->builder->NeedCompact(); } const uint64_t current_entries = sub_compact->builder->NumEntries(); - meta->marked_for_compaction = sub_compact->builder->NeedCompact(); if (s.ok()) { s = sub_compact->builder->Finish(); } else { sub_compact->builder->Abandon(); } const uint64_t current_bytes = sub_compact->builder->FileSize(); - meta->fd.file_size = current_bytes; + if (s.ok()) { + meta->fd.file_size = current_bytes; + } sub_compact->current_output()->finished = true; sub_compact->total_bytes += current_bytes; @@ -1144,17 +1146,24 @@ Status CompactionJob::FinishCompactionOutputFile( meta->marked_for_compaction ? " (need compaction)" : ""); } } - std::string fname = TableFileName(db_options_.db_paths, meta->fd.GetNumber(), - meta->fd.GetPathId()); + std::string fname; + FileDescriptor output_fd; + if (meta != nullptr) { + fname = TableFileName(db_options_.db_paths, meta->fd.GetNumber(), + meta->fd.GetPathId()); + output_fd = meta->fd; + } else { + fname = "(nil)"; + } EventHelpers::LogAndNotifyTableFileCreationFinished( event_logger_, cfd->ioptions()->listeners, dbname_, cfd->GetName(), fname, - job_id_, meta->fd, tp, TableFileCreationReason::kCompaction, s); + job_id_, output_fd, tp, TableFileCreationReason::kCompaction, s); #ifndef ROCKSDB_LITE // Report new file to SstFileManagerImpl auto sfm = static_cast(db_options_.sst_file_manager.get()); - if (sfm && meta->fd.GetPathId() == 0) { + if (sfm && meta != nullptr && meta->fd.GetPathId() == 0) { auto fn = TableFileName(cfd->ioptions()->db_paths, meta->fd.GetNumber(), meta->fd.GetPathId()); sfm->OnAddFile(fn); diff --git a/db/db_impl.cc b/db/db_impl.cc index 0652ce0c0..0e1a821b0 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -777,9 +777,7 @@ void DBImpl::BackgroundCallPurge() { purge_queue_.pop_front(); mutex_.Unlock(); - Status file_deletion_status; - DeleteObsoleteFileImpl(file_deletion_status, job_id, fname, type, number, - path_id); + DeleteObsoleteFileImpl(job_id, fname, type, number, path_id); mutex_.Lock(); } else { assert(!logs_to_free_queue_.empty()); diff --git a/db/db_impl.h b/db/db_impl.h index e44b81260..5115ac6f1 100644 --- a/db/db_impl.h +++ b/db/db_impl.h @@ -685,9 +685,8 @@ class DBImpl : public DB { // Delete any unneeded files and stale in-memory entries. void DeleteObsoleteFiles(); // Delete obsolete files and log status and information of file deletion - void DeleteObsoleteFileImpl(Status file_deletion_status, int job_id, - const std::string& fname, FileType type, - uint64_t number, uint32_t path_id); + void DeleteObsoleteFileImpl(int job_id, const std::string& fname, + FileType type, uint64_t number, uint32_t path_id); // Background process needs to call // auto x = CaptureCurrentFileNumberInPendingOutputs() diff --git a/db/db_impl_files.cc b/db/db_impl_files.cc index e44e42318..4add98a81 100644 --- a/db/db_impl_files.cc +++ b/db/db_impl_files.cc @@ -300,9 +300,10 @@ bool CompareCandidateFile(const JobContext::CandidateFileInfo& first, }; // namespace // Delete obsolete files and log status and information of file deletion -void DBImpl::DeleteObsoleteFileImpl(Status file_deletion_status, int job_id, - const std::string& fname, FileType type, - uint64_t number, uint32_t path_id) { +void DBImpl::DeleteObsoleteFileImpl(int job_id, const std::string& fname, + FileType type, uint64_t number, + uint32_t path_id) { + Status file_deletion_status; if (type == kTableFile) { file_deletion_status = DeleteSSTFile(&immutable_db_options_, fname, path_id); @@ -488,8 +489,7 @@ void DBImpl::PurgeObsoleteFiles(const JobContext& state, bool schedule_only) { InstrumentedMutexLock guard_lock(&mutex_); SchedulePendingPurge(fname, type, number, path_id, state.job_id); } else { - DeleteObsoleteFileImpl(file_deletion_status, state.job_id, fname, type, - number, path_id); + DeleteObsoleteFileImpl(state.job_id, fname, type, number, path_id); } } diff --git a/memtable/inlineskiplist.h b/memtable/inlineskiplist.h index 5cf6c57d5..505b73d28 100644 --- a/memtable/inlineskiplist.h +++ b/memtable/inlineskiplist.h @@ -483,11 +483,13 @@ InlineSkipList::FindLessThan(const char* key, Node** prev, // KeyIsAfter(key, last_not_after) is definitely false Node* last_not_after = nullptr; while (true) { + assert(x != nullptr); Node* next = x->Next(level); assert(x == head_ || next == nullptr || KeyIsAfterNode(next->Key(), x)); assert(x == head_ || KeyIsAfterNode(key, x)); if (next != last_not_after && KeyIsAfterNode(key, next)) { // Keep searching in this list + assert(next != nullptr); x = next; } else { if (prev != nullptr) { @@ -863,6 +865,7 @@ void InlineSkipList::TEST_Validate() const { // levels. Node* nodes[kMaxPossibleHeight]; int max_height = GetMaxHeight(); + assert(max_height > 0); for (int i = 0; i < max_height; i++) { nodes[i] = head_; } @@ -892,7 +895,7 @@ void InlineSkipList::TEST_Validate() const { } } for (int i = 1; i < max_height; i++) { - assert(nodes[i]->Next(i) == nullptr); + assert(nodes[i] != nullptr && nodes[i]->Next(i) == nullptr); } } diff --git a/memtable/skiplist.h b/memtable/skiplist.h index 0162dccb7..58996be3e 100644 --- a/memtable/skiplist.h +++ b/memtable/skiplist.h @@ -310,6 +310,7 @@ typename SkipList::Node* SkipList:: int level = GetMaxHeight() - 1; Node* last_bigger = nullptr; while (true) { + assert(x != nullptr); Node* next = x->Next(level); // Make sure the lists are sorted assert(x == head_ || next == nullptr || KeyIsAfterNode(next->key, x)); @@ -338,6 +339,7 @@ SkipList::FindLessThan(const Key& key, Node** prev) const { // KeyIsAfter(key, last_not_after) is definitely false Node* last_not_after = nullptr; while (true) { + assert(x != nullptr); Node* next = x->Next(level); assert(x == head_ || next == nullptr || KeyIsAfterNode(next->key, x)); assert(x == head_ || KeyIsAfterNode(key, x)); diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index bd1e9fd95..01d7667a3 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -843,7 +843,7 @@ TEST_F(BackupableDBTest, NoDoubleCopy) { ASSERT_OK(test_backup_env_->FileExists(backupdir_ + "/shared/00015.sst")); // MANIFEST file size should be only 100 - uint64_t size; + uint64_t size = 0; test_backup_env_->GetFileSize(backupdir_ + "/private/2/MANIFEST-01", &size); ASSERT_EQ(100UL, size); test_backup_env_->GetFileSize(backupdir_ + "/shared/00015.sst", &size); -- GitLab