From a0ba0aa877f0c15d08d7d4470d0d95c2019b5952 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 19 Oct 2016 10:59:46 -0700 Subject: [PATCH] Fix uninitialized variable gcc error for MyRocks Summary: make sure seq_ is properly initialized even if ParseInternalKey() fails. Test Plan: run myrocks release tests Reviewers: lightmark, mung, sdong Reviewed By: sdong Subscribers: andrewkr, dhruba, leveldb Differential Revision: https://reviews.facebook.net/D65199 --- db/compaction_iterator_test.cc | 2 +- db/dbformat.h | 6 +----- db/range_del_aggregator.cc | 17 +++++++++++------ db/range_del_aggregator.h | 6 +++--- db/table_cache.cc | 5 ++++- table/table_test.cc | 4 +++- 6 files changed, 23 insertions(+), 17 deletions(-) diff --git a/db/compaction_iterator_test.cc b/db/compaction_iterator_test.cc index 61b8ba970..12df6ea83 100644 --- a/db/compaction_iterator_test.cc +++ b/db/compaction_iterator_test.cc @@ -26,7 +26,7 @@ class CompactionIteratorTest : public testing::Test { std::unique_ptr range_del_iter( new test::VectorIterator(range_del_ks, range_del_vs)); range_del_agg_.reset(new RangeDelAggregator(icmp_, snapshots_)); - range_del_agg_->AddTombstones(std::move(range_del_iter)); + ASSERT_OK(range_del_agg_->AddTombstones(std::move(range_del_iter))); merge_helper_.reset(new MergeHelper(Env::Default(), cmp_, nullptr, nullptr, nullptr, 0U, false, 0)); diff --git a/db/dbformat.h b/db/dbformat.h index 7d1066a31..6aaff1390 100644 --- a/db/dbformat.h +++ b/db/dbformat.h @@ -516,11 +516,7 @@ struct RangeTombstone { explicit RangeTombstone(Slice sk, Slice ek, SequenceNumber sn) : start_key_(sk), end_key_(ek), seq_(sn) {} - explicit RangeTombstone(Slice internal_key, Slice value) { - ParsedInternalKey parsed_key; - if (!ParseInternalKey(internal_key, &parsed_key)) { - assert(false); - } + explicit RangeTombstone(ParsedInternalKey parsed_key, Slice value) { start_key_ = parsed_key.user_key; seq_ = parsed_key.sequence; end_key_ = value; diff --git a/db/range_del_aggregator.cc b/db/range_del_aggregator.cc index 1515a63f6..f79ce48b2 100644 --- a/db/range_del_aggregator.cc +++ b/db/range_del_aggregator.cc @@ -73,25 +73,30 @@ bool RangeDelAggregator::ShouldAddTombstones( return false; } -void RangeDelAggregator::AddTombstones(ScopedArenaIterator input) { - AddTombstones(input.release(), true /* arena */); +Status RangeDelAggregator::AddTombstones(ScopedArenaIterator input) { + return AddTombstones(input.release(), true /* arena */); } -void RangeDelAggregator::AddTombstones( +Status RangeDelAggregator::AddTombstones( std::unique_ptr input) { - AddTombstones(input.release(), false /* arena */); + return AddTombstones(input.release(), false /* arena */); } -void RangeDelAggregator::AddTombstones(InternalIterator* input, bool arena) { +Status RangeDelAggregator::AddTombstones(InternalIterator* input, bool arena) { pinned_iters_mgr_.PinIterator(input, arena); input->SeekToFirst(); while (input->Valid()) { - RangeTombstone tombstone(input->key(), input->value()); + ParsedInternalKey parsed_key; + if (!ParseInternalKey(input->key(), &parsed_key)) { + return Status::Corruption("Unable to parse range tombstone InternalKey"); + } + RangeTombstone tombstone(parsed_key, input->value()); auto& tombstone_map = GetStripeMapIter(tombstone.seq_)->second; tombstone_map.emplace(tombstone.start_key_.ToString(), std::move(tombstone)); input->Next(); } + return Status::OK(); } RangeDelAggregator::StripeMap::iterator RangeDelAggregator::GetStripeMapIter( diff --git a/db/range_del_aggregator.h b/db/range_del_aggregator.h index 97a76e484..92cb4cae4 100644 --- a/db/range_del_aggregator.h +++ b/db/range_del_aggregator.h @@ -27,8 +27,8 @@ class RangeDelAggregator { bool ShouldDelete(const Slice& internal_key, bool for_compaction = false); bool ShouldAddTombstones(bool bottommost_level = false); - void AddTombstones(ScopedArenaIterator input); - void AddTombstones(std::unique_ptr input); + Status AddTombstones(ScopedArenaIterator input); + Status AddTombstones(std::unique_ptr input); // write tombstones covering a range to a table builder // usually don't add to a max-level table builder void AddToBuilder(TableBuilder* builder, bool extend_before_min_key, @@ -43,7 +43,7 @@ class RangeDelAggregator { // their seqnums are greater than the next smaller snapshot's seqnum. typedef std::map StripeMap; - void AddTombstones(InternalIterator* input, bool arena); + Status AddTombstones(InternalIterator* input, bool arena); StripeMap::iterator GetStripeMapIter(SequenceNumber seq); PinnedIteratorsManager pinned_iters_mgr_; diff --git a/db/table_cache.cc b/db/table_cache.cc index 1ae95d925..01606271a 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -226,7 +226,10 @@ InternalIterator* TableCache::NewIterator( if (range_del_agg != nullptr) { std::unique_ptr iter( table_reader->NewRangeTombstoneIterator(options)); - range_del_agg->AddTombstones(std::move(iter)); + Status s = range_del_agg->AddTombstones(std::move(iter)); + if (!s.ok()) { + return NewErrorInternalIterator(s, arena); + } } InternalIterator* result = nullptr; diff --git a/table/table_test.cc b/table/table_test.cc index 5a1906f42..f6dc75f81 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -1184,7 +1184,9 @@ TEST_F(BlockBasedTableTest, RangeDelBlock) { ASSERT_EQ(true, iter->Valid()); for (int i = 0; i < 2; i++) { ASSERT_TRUE(iter->Valid()); - RangeTombstone t(iter->key(), iter->value()); + ParsedInternalKey parsed_key; + ASSERT_TRUE(ParseInternalKey(iter->key(), &parsed_key)); + RangeTombstone t(parsed_key, iter->value()); ASSERT_EQ(t.start_key_, keys[i]); ASSERT_EQ(t.end_key_, vals[i]); ASSERT_EQ(t.seq_, i);