Merge pull request ceph#63844 from aainscow/internal_map_non_const_iters

interval_map: Add non-const iterators 

Reviewed-by: Radoslaw Zarzynski <[email protected]>

Author: aainscow

src/common/interval_map.h

@@ -30,7 +30,7 @@
  * commutativity, which doesn't work if we want more recent insertions
  * to overwrite previous ones.
  */
-template <typename K, typename V, typename S, template<typename, typename, typename ...> class C = std::map>
+template <typename K, typename V, typename S, template<typename, typename, typename ...> class C = std::map, bool nonconst_iterator = false>
 class interval_map {
   S s;
   using Map = C<K, std::pair<K, V> >;
@@ -215,6 +215,7 @@ class interval_map {
       set.insert(i.get_off(), i.get_len());
     }
   }
+
   class const_iterator {
     Cmapiter it;
     const_iterator(Cmapiter &&it) : it(std::move(it)) {}
@@ -269,6 +270,77 @@ class interval_map {
   const_iterator end() const {
     return const_iterator(m.end());
   }
+
+  const_iterator cbegin() const {
+    return const_iterator(m.begin());
+  }
+  const_iterator cend() const {
+    return const_iterator(m.end());
+  }
+
+  class iterator {
+    Mapiter it;
+    bool end;
+    iterator(Mapiter &&it, bool end) : it(std::move(it)), end(end) {}
+
+    friend class interval_map;
+  public:
+    iterator(const iterator &) = default;
+    iterator &operator=(const iterator &) = default;
+
+    iterator &operator++() {
+      /* While the buffer can be modified with a non-const iterator, it
+       * not change size. Allow changes in size would allow for the interval
+       * to be merged into the next one, which would allow for unexpected
+       * behaviour.
+       */
+      if (!end && get_val().length() != get_len()) {
+        throw std::out_of_range("buffer length has changed");
+      }
+      ++it;
+      return *this;
+    }
+    iterator operator++(int) {
+      return const_iterator(it++);
+    }
+    iterator &operator--() {
+      --it;
+      return *this;
+    }
+    iterator operator--(int) {
+      return const_iterator(it--);
+    }
+    bool operator==(const iterator &rhs) const {
+      return it == rhs.it;
+    }
+    bool operator!=(const iterator &rhs) const {
+      return it != rhs.it;
+    }
+    K get_off() const {
+      return it->first;
+    }
+    K get_len() const {
+      return it->second.first;
+    }
+    V &get_val() {
+      return it->second.second;
+    }
+    iterator &operator*() {
+      return *this;
+    }
+    constexpr bool contains(K _off, K _len) const {
+      K off = get_off();
+      K len = get_len();
+      return off <= _off && _off + _len <= off + len;
+    }
+  };
+  static constexpr bool nonconst_iterator_cond() { return nonconst_iterator; }
+  iterator begin() requires (nonconst_iterator) {
+    return iterator(m.begin(), false);
+  }
+  iterator end() requires (nonconst_iterator) {
+    return iterator(m.end(), true);
+  }
   std::pair<const_iterator, const_iterator> get_containing_range(
     K off,
     K len) const {

src/osd/ECUtil.h

@@ -58,7 +58,7 @@ struct bl_split_merge {
 
 using extent_set = interval_set<uint64_t, boost::container::flat_map, false>;
 using extent_map = interval_map<uint64_t, ceph::buffer::list, bl_split_merge,
-                                boost::container::flat_map>;
+                                boost::container::flat_map, true>;
 
 /* Slice iterator.  This looks for contiguous buffers which are common
  * across all shards in the out_set.
@@ -74,8 +74,8 @@ class slice_iterator {
   uint64_t length = std::numeric_limits<uint64_t>::max();
   uint64_t start = std::numeric_limits<uint64_t>::max();
   uint64_t end = std::numeric_limits<uint64_t>::max();
-  shard_id_map<std::pair<extent_map::const_iterator,
-                         bufferlist::const_iterator>> iters;
+  shard_id_map<std::pair<extent_map::iterator,
+                         bufferlist::iterator>> iters;
   shard_id_map<bufferptr> in;
   shard_id_map<bufferptr> out;
   const shard_id_set &out_set;
@@ -992,10 +992,10 @@ class shard_extent_map_t {
   size_t shard_count() { return extent_maps.size(); }
 
 
-  void assert_buffer_contents_equal(shard_extent_map_t other) const {
+  void assert_buffer_contents_equal(shard_extent_map_t other) {
     for (auto &&[shard, emap] : extent_maps) {
       for (auto &&i : emap) {
-        bufferlist bl = i.get_val();
+        bufferlist &bl = i.get_val();
         bufferlist otherbl;
         other.get_buffer(shard, i.get_off(), i.get_len(), otherbl);
         ceph_assert(bl.contents_equal(otherbl));

src/test/common/test_interval_map.cc

@@ -27,7 +27,7 @@ class IntervalMapTest : public ::testing::Test {
   using TestType = T;
 };
 
-template <typename _key, typename _can_merge, template<typename, typename, typename ...> class _map = std::map>
+template <typename _key, typename _can_merge, template<typename, typename, typename ...> class _map = std::map, bool _nonconst_iterator = false>
 struct bufferlist_test_type {
   using key = _key;
   using value = bufferlist;
@@ -62,7 +62,7 @@ struct bufferlist_test_type {
   };
 
   using splitter = boost::mpl::apply<make_splitter, _can_merge>;
-  using imap = interval_map<key, value, splitter, _map>;
+  using imap = interval_map<key, value, splitter, _map, _nonconst_iterator>;
 
   struct generate_random {
     bufferlist operator()(key len) {
@@ -81,7 +81,8 @@ using IntervalMapTypes = ::testing::Types<
   bufferlist_test_type<uint64_t, std::false_type>,
   bufferlist_test_type<uint64_t, std::true_type>,
   bufferlist_test_type<uint64_t, std::false_type, boost::container::flat_map>,
-  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map>
+  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map>,
+  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map, true>
 >;
 
 TYPED_TEST_SUITE(IntervalMapTest, IntervalMapTypes);
@@ -405,3 +406,27 @@ TYPED_TEST(IntervalMapTest, print) {
     EXPECT_EQ("{0~5(5),10~5(5)}", out.str() );
   }
 }
+
+/* This test does nothing unless nonconst_iterator is set on the interval map
+ * If it is set, then the simple fact that append_zero() compiles means that
+ * the non-const iterator has been enabled and used. The test then checks that
+ * changing the size of a buffer is illegal.
+ */
+TYPED_TEST(IntervalMapTest, append_buffer_in_loop) {
+  USING_WITH_MERGE;
+  imap m;
+  if constexpr (m.nonconst_iterator_cond()) {
+    m.insert(0, 5, gen(5));
+    try {
+      for (auto && i : m) {
+        i.get_val().append_zero(10);
+      }
+      FAIL(); // Should panic
+    } catch (const std::out_of_range& exception) {
+      ASSERT_EQ("buffer length has changed"sv, exception.what());
+    } catch (const std::exception&) {
+      // Wrong exception.
+      FAIL();
+    }
+  }
+}