common: drop backward iteration from bufferlist.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index c0c23f4bedc03d1ed4bc2c7afe8e2eace889be67..79dcffd0ad5d245c6e8e932f9dbc6e80bc844771 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -848,43 +848,41 @@ using namespace ceph;
   buffer::list::iterator_impl<is_const>::iterator_impl(const buffer::list::iterator& i)
     : iterator_impl<is_const>(i.bl, i.off, i.p, i.p_off) {}
 
+#ifdef BL_BACKWARD_COMPAT
+  /// The backward iteration over is bufferlist is DEPRECATED!
+  /// The old int-taking variant of advance() is solely for compatibility.
   template<bool is_const>
   void buffer::list::iterator_impl<is_const>::advance(int o)
   {
-    //cout << this << " advance " << o << " from " << off << " (p_off " << p_off << " in " << p->length() << ")" << std::endl;
-    if (o > 0) {
-      p_off += o;
-      while (p_off > 0) {
-       if (p == ls->end())
-         throw end_of_buffer();
-       if (p_off >= p->length()) {
-         // skip this buffer
-         p_off -= p->length();
-         p++;
-       } else {
-         // somewhere in this buffer!
-         break;
-       }
-      }
-      off += o;
+    seek(off + o);
+  }
+#endif // BL_BACKWARD_COMPAT
+
+  template<bool is_const>
+  void buffer::list::iterator_impl<is_const>::advance(unsigned o)
+  {
+    //cout << this << " advance " << o << " from " << off
+    //     << " (p_off " << p_off << " in " << p->length() << ")"
+    //     << std::endl;
+
+    p_off += o;
+
+    if (!o) {
       return;
     }
-    while (o < 0) {
-      if (p_off) {
-       unsigned d = -o;
-       if (d > p_off)
-         d = p_off;
-       p_off -= d;
-       off -= d;
-       o += d;
-      } else if (off > 0) {
-       ceph_assert(p != ls->begin());
-       p--;
-       p_off = p->length();
+    while (p_off > 0) {
+      if (p == ls->end())
+        throw end_of_buffer();
+      if (p_off >= p->length()) {
+        // skip this buffer
+        p_off -= p->length();
+        p++;
       } else {
-       throw end_of_buffer();
+        // somewhere in this buffer!
+        break;
       }
     }
+    off += o;
   }
 
   template<bool is_const>
@@ -909,7 +907,7 @@ using namespace ceph;
   {
     if (p == ls->end())
       throw end_of_buffer();
-    advance(1);
+    advance(1u);
     return *this;
   }
 
@@ -1086,7 +1084,16 @@ using namespace ceph;
     : iterator_impl(l, o, ip, po)
   {}
 
+#ifdef BL_BACKWARD_COMPAT
+  /// The backward iteration over is bufferlist is DEPRECATED!
+  /// The old int-taking variant of advance() is solely for compatibility.
   void buffer::list::iterator::advance(int o)
+  {
+    seek(off + o);
+  }
+#endif // BL_BACKWARD_COMPAT
+
+  void buffer::list::iterator::advance(unsigned o)
   {
     buffer::list::iterator_impl<false>::advance(o);
   }

diff --git a/src/include/buffer.h b/src/include/buffer.h
index 1c941db15ce0c807d4f2fa9bbe025cf8ac1fa322..ffbd2f6a8329ccbc39fc83ecf56cd4566e729047 100644 (file)
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -438,7 +438,14 @@ namespace buffer CEPH_BUFFER_API {
        //return off == bl->length();
       }
 
+#ifdef BL_BACKWARD_COMPAT
       void advance(int o);
+#else
+      void advance(int o) = delete;
+#endif // BL_BACKWARD_COMPAT
+
+      void advance(unsigned o);
+      void advance(size_t o) { advance(static_cast<unsigned>(o)); }
       void seek(unsigned o);
       char operator*() const;
       iterator_impl& operator++();
@@ -484,7 +491,15 @@ namespace buffer CEPH_BUFFER_API {
       iterator(bl_t *l, unsigned o=0);
       iterator(bl_t *l, unsigned o, list_iter_t ip, unsigned po);
 
+#ifdef BL_BACKWARD_COMPAT
       void advance(int o);
+#else
+      void advance(int o) = delete;
+#endif // BL_BACKWARD_COMPAT
+
+      void advance(unsigned o);
+      void advance(size_t o) { advance(static_cast<unsigned>(o)); }
+
       void seek(unsigned o);
       using iterator_impl<false>::operator*;
       char operator*();

diff --git a/src/include/denc.h b/src/include/denc.h
index c8d79f8886893bcfc593096983d28e0a25eacdc2..7f2d5d9aeec3d79f1e45e572544fae7199c4b510 100644 (file)
--- a/src/include/denc.h
+++ b/src/include/denc.h
@@ -1519,7 +1519,7 @@ inline std::enable_if_t<traits::supported && !traits::need_contiguous> decode(
     t.copy_shallow(remaining, tmp);
     auto cp = std::cbegin(tmp);
     traits::decode(o, cp);
-    p.advance((ssize_t)cp.get_offset());
+    p.advance(cp.get_offset());
   }
 }
 
@@ -1540,7 +1540,7 @@ inline std::enable_if_t<traits::supported && traits::need_contiguous> decode(
   t.copy_shallow(p.get_bl().length() - p.get_off(), tmp);
   auto cp = std::cbegin(tmp);
   traits::decode(o, cp);
-  p.advance((ssize_t)cp.get_offset());
+  p.advance(cp.get_offset());
 }
 
 // nohead variants
@@ -1571,7 +1571,7 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
   t.copy_shallow(p.get_bl().length() - p.get_off(), tmp);
   auto cp = std::cbegin(tmp);
   traits::decode_nohead(num, o, cp);
-  p.advance((ssize_t)cp.get_offset());
+  p.advance(cp.get_offset());
 }
 }
 

diff --git a/src/include/encoding.h b/src/include/encoding.h
index 3b21569c46dd68436260197f873996a449d3d575..65f8222f98f8c2ce7111195d2957cc657f5ba2fb 100644 (file)
--- a/src/include/encoding.h
+++ b/src/include/encoding.h
@@ -1203,19 +1203,31 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
  * @param v current (code) version of the encoding
  * @param compat oldest code version that can decode it
  * @param bl bufferlist to encode to
+ *
+ * NOTE: to not advance() backward, we snapshot the end of bufferlist
+ * in a given moment, and then rely on copy_in()'s ability to rewind:
+ *
+ * void buffer::list::iterator::copy_in(...)
+ * {
+ *   // copy
+ *   if (p == ls->end())
+ *     seek(off);
+ *
+ *   // ...
+ * }
+ *
+ * This can be quite costly, so next commit is expected to remedy.
  */
 #define ENCODE_START(v, compat, bl)                         \
   using ::ceph::encode;                                             \
   __u8 struct_v = v, struct_compat = compat;                \
   encode(struct_v, (bl));                                   \
-  encode(struct_compat, (bl));                      \
   ::ceph::buffer::list::iterator struct_compat_it = (bl).end();        \
-  struct_compat_it.advance(-1);                                     \
+  encode(struct_compat, (bl));                      \
   ceph_le32 struct_len;                                             \
   struct_len = 0;                                            \
-  encode(struct_len, (bl));                                 \
   ::ceph::buffer::list::iterator struct_len_it = (bl).end(); \
-  struct_len_it.advance(-4);                                \
+  encode(struct_len, (bl));                                 \
   do {
 
 /**
@@ -1282,7 +1294,7 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
     if (v < struct_compat)                                             \
       throw buffer::malformed_input(DECODE_ERR_OLDVERSION(__PRETTY_FUNCTION__, v, struct_compat)); \
   } else if (skip_v) {                                                 \
-    if ((int)bl.get_remaining() < skip_v)                              \
+    if (bl.get_remaining() < skip_v)                                   \
       throw buffer::malformed_input(DECODE_ERR_PAST(__PRETTY_FUNCTION__)); \
     bl.advance(skip_v);                                                        \
   }                                                                    \
@@ -1311,7 +1323,7 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
  * @param bl bufferlist::iterator containing the encoded data
  */
 #define DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, bl)           \
-  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 0, bl)
+  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 0u, bl)
 
 /**
  * start a decoding block with legacy support for older encoding schemes
@@ -1331,10 +1343,10 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
  * @param bl bufferlist::iterator containing the encoded data
  */
 #define DECODE_START_LEGACY_COMPAT_LEN_32(v, compatv, lenv, bl)                \
-  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 3, bl)
+  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 3u, bl)
 
 #define DECODE_START_LEGACY_COMPAT_LEN_16(v, compatv, lenv, bl)                \
-  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 1, bl)
+  __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, 1u, bl)
 
 /**
  * finish decode block

diff --git a/src/mds/CInode.cc b/src/mds/CInode.cc
index ad7b3eb9cd9d2aa9b1689224e31d314ff7f05c69..ec17cd64a0891574582e860b7b743c23c9ac8a72 100644 (file)
--- a/src/mds/CInode.cc
+++ b/src/mds/CInode.cc
@@ -3588,11 +3588,9 @@ int CInode::encode_inodestat(bufferlist& bl, Session *session,
     using ceph::encode;
     if (xattr_version) {
       ceph_le32 xbl_len;
+      auto xbl_len_it = bl.end();
       xbl_len = sizeof(__u32);
       encode(xbl_len, bl);
-
-      auto xbl_len_it = bl.end();
-      xbl_len_it.advance(-4);
       if (pxattrs)
        encode(*pxattrs, bl);
       else

diff --git a/src/msg/simple/Pipe.cc b/src/msg/simple/Pipe.cc
index 434a8e67854c2ca4204b63da1fb34bc4a0540f73..697212f0c87581b0fd8cc0368067975fa0594000 100644 (file)
--- a/src/msg/simple/Pipe.cc
+++ b/src/msg/simple/Pipe.cc
@@ -2185,7 +2185,7 @@ int Pipe::read_message(Message **pm, AuthSessionHandler* auth_handler)
       if (got < 0)
        goto out_dethrottle;
       if (got > 0) {
-       blp.advance(got);
+       blp.advance(static_cast<size_t>(got));
        data.append(bp, 0, got);
        offset += got;
        left -= got;

diff --git a/src/os/bluestore/bluestore_types.cc b/src/os/bluestore/bluestore_types.cc
index 93dbe41de2948e48831519a2d60002e532190813..4d7e14afd863279e0d1e2fe0a344a8f0fea40818 100644 (file)
--- a/src/os/bluestore/bluestore_types.cc
+++ b/src/os/bluestore/bluestore_types.cc
@@ -36,7 +36,7 @@ void bluestore_bdev_label_t::encode(bufferlist& bl) const
 
 void bluestore_bdev_label_t::decode(bufferlist::const_iterator& p)
 {
-  p.advance(60); // see above
+  p.advance(60u); // see above
   DECODE_START(2, p);
   decode(osd_uuid, p);
   decode(size, p);

diff --git a/src/os/filestore/FileJournal.h b/src/os/filestore/FileJournal.h
index a9af1a64b41f2be94c5189cde3b5e6ddd3eea2dd..2093084b967c3b9a86f70f0a7a1d05fcf74a215f 100644 (file)
--- a/src/os/filestore/FileJournal.h
+++ b/src/os/filestore/FileJournal.h
@@ -182,7 +182,7 @@ public:
       decode(v, bl);
       if (v < 2) {  // normally 0, but conceivably 1
        // decode old header_t struct (pre v0.40).
-       bl.advance(4); // skip __u32 flags (it was unused by any old code)
+       bl.advance(4u); // skip __u32 flags (it was unused by any old code)
        flags = 0;
        uint64_t tfsid;
        decode(tfsid, bl);

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index f64638069573d6488194198ec6ce583e26c12f05..87b48255f0306c8cab86df2e000088314911cbae 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -618,9 +618,8 @@ void OSDMap::Incremental::encode(bufferlist& bl, uint64_t features) const
     ENCODE_FINISH(bl); // osd-only data
   }
 
-  encode((uint32_t)0, bl); // dummy inc_crc
   crc_it = bl.end();
-  crc_it.advance(-4);
+  encode((uint32_t)0, bl); // dummy inc_crc
   tail_offset = bl.length();
 
   encode(full_crc, bl);
@@ -756,8 +755,7 @@ void OSDMap::Incremental::decode(bufferlist::const_iterator& bl)
 
   DECODE_START_LEGACY_COMPAT_LEN(8, 7, 7, bl); // wrapper
   if (struct_v < 7) {
-    int struct_v_size = sizeof(struct_v);
-    bl.advance(-struct_v_size);
+    bl.seek(start_offset);
     decode_classic(bl);
     encode_features = 0;
     if (struct_v >= 6)
@@ -2805,9 +2803,8 @@ void OSDMap::encode(bufferlist& bl, uint64_t features) const
     ENCODE_FINISH(bl); // osd-only data
   }
 
-  encode((uint32_t)0, bl); // dummy crc
   crc_it = bl.end();
-  crc_it.advance(-4);
+  encode((uint32_t)0, bl); // dummy crc
   tail_offset = bl.length();
 
   ENCODE_FINISH(bl); // meta-encoding wrapper
@@ -2970,8 +2967,7 @@ void OSDMap::decode(bufferlist::const_iterator& bl)
 
   DECODE_START_LEGACY_COMPAT_LEN(8, 7, 7, bl); // wrapper
   if (struct_v < 7) {
-    int struct_v_size = sizeof(struct_v);
-    bl.advance(-struct_v_size);
+    bl.seek(start_offset);
     decode_classic(bl);
     return;
   }

diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc
index 3fdc2dc32214508d4e0ec906234382d29ef3f54e..a1d7f2bfcfd30582059bfaf0b59da8f36bd1c6b1 100644 (file)
--- a/src/osd/osd_types.cc
+++ b/src/osd/osd_types.cc
@@ -4867,7 +4867,7 @@ void SnapSet::decode(bufferlist::const_iterator& bl)
 {
   DECODE_START_LEGACY_COMPAT_LEN(3, 2, 2, bl);
   decode(seq, bl);
-  bl.advance(1);  // skip legacy head_exists (always true)
+  bl.advance(1u);  // skip legacy head_exists (always true)
   decode(snaps, bl);
   decode(clones, bl);
   decode(clone_overlap, bl);

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 1d9689fe966cfc5f524f1000076da9602cee6d62..cbab5919cdd8642a30e81a6b17fbc34d284529ef 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -810,8 +810,10 @@ TEST(BufferListIterator, constructors) {
     EXPECT_EQ('B', *i);
     EXPECT_EQ('C', *j);
     bl.c_str()[1] = 'X';
+#ifdef BL_BACKWARD_COMPAT
     j.advance(-1);
     EXPECT_EQ('X', *j);
+#endif // BL_BACKWARD_COMPAT
   }
 
   //
@@ -886,23 +888,27 @@ TEST(BufferListIterator, advance) {
 
   {
     bufferlist::iterator i(&bl);
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer);
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
   }
+#ifdef BL_BACKWARD_COMPAT
   {
     bufferlist::iterator i(&bl);
     EXPECT_THROW(i.advance(-1), buffer::end_of_buffer);
   }
+#endif // BL_BACKWARD_COMPAT
   {
     bufferlist::iterator i(&bl);
     EXPECT_EQ('A', *i);
-    i.advance(1);
+    i.advance(1u);
     EXPECT_EQ('B', *i);
-    i.advance(3);
+    i.advance(3u);
     EXPECT_EQ('E', *i);
+#ifdef BL_BACKWARD_COMPAT
     i.advance(-3);
     EXPECT_EQ('B', *i);
     i.advance(-1);
     EXPECT_EQ('A', *i);
+#endif // BL_BACKWARD_COMPAT
   }
 }
 
@@ -917,14 +923,14 @@ TEST(BufferListIterator, get_ptr_and_advance)
   bl.append(c);
   const char *ptr;
   bufferlist::iterator p = bl.begin();
-  ASSERT_EQ(3u, p.get_ptr_and_advance(11, &ptr));
+  ASSERT_EQ(3u, p.get_ptr_and_advance(11u, &ptr));
   ASSERT_EQ(bl.length() - 3u, p.get_remaining());
   ASSERT_EQ(0, memcmp(ptr, "one", 3));
-  ASSERT_EQ(2u, p.get_ptr_and_advance(2, &ptr));
+  ASSERT_EQ(2u, p.get_ptr_and_advance(2u, &ptr));
   ASSERT_EQ(0, memcmp(ptr, "tw", 2));
-  ASSERT_EQ(1u, p.get_ptr_and_advance(4, &ptr));
+  ASSERT_EQ(1u, p.get_ptr_and_advance(4u, &ptr));
   ASSERT_EQ(0, memcmp(ptr, "o", 1));
-  ASSERT_EQ(5u, p.get_ptr_and_advance(5, &ptr));
+  ASSERT_EQ(5u, p.get_ptr_and_advance(5u, &ptr));
   ASSERT_EQ(0, memcmp(ptr, "three", 5));
   ASSERT_EQ(0u, p.get_remaining());
 }
@@ -953,14 +959,14 @@ TEST(BufferListIterator, iterator_crc32c) {
 
   bl3.append(s.substr(98, 55));
   it = bl1.begin();
-  it.advance(98);
+  it.advance(98u);
   ASSERT_EQ(bl3.crc32c(0), it.crc32c(55, 0));
   ASSERT_EQ(4u, it.get_remaining());
 
   bl3.clear();
   bl3.append(s.substr(98 + 55));
   it = bl1.begin();
-  it.advance(98 + 55);
+  it.advance(98u + 55u);
   ASSERT_EQ(bl3.crc32c(0), it.crc32c(10, 0));
   ASSERT_EQ(0u, it.get_remaining());
 }
@@ -984,7 +990,7 @@ TEST(BufferListIterator, operator_star) {
   {
     bufferlist::iterator i(&bl);
     EXPECT_EQ('A', *i);
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer);
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     EXPECT_THROW(*i, buffer::end_of_buffer);
   }
 }
@@ -1076,7 +1082,7 @@ TEST(BufferListIterator, copy) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     i.copy(2, copy);
     EXPECT_EQ(0, ::memcmp(copy, expected, 2));
     EXPECT_EQ('X', copy[2]);
@@ -1116,7 +1122,7 @@ TEST(BufferListIterator, copy) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     i.copy(2, copy);
     EXPECT_EQ(0, ::memcmp(copy.c_str(), expected, 2));
     i.seek(0);
@@ -1137,7 +1143,7 @@ TEST(BufferListIterator, copy) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     i.copy_all(copy);
     EXPECT_EQ('A', copy[0]);
     EXPECT_EQ('B', copy[1]);
@@ -1153,7 +1159,7 @@ TEST(BufferListIterator, copy) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     i.copy(2, copy);
     EXPECT_EQ(0, ::memcmp(copy.c_str(), expected, 2));
     i.seek(0);
@@ -1179,7 +1185,7 @@ TEST(BufferListIterator, copy_in) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     const char *expected = "ABC";
     i.copy_in(3, expected);
     EXPECT_EQ(0, ::memcmp(bl.c_str(), expected, 3));
@@ -1196,7 +1202,7 @@ TEST(BufferListIterator, copy_in) {
     //
     // demonstrates that it seeks back to offset if p == ls->end()
     //
-    EXPECT_THROW(i.advance(200), buffer::end_of_buffer); 
+    EXPECT_THROW(i.advance(200u), buffer::end_of_buffer);
     bufferlist expected;
     expected.append("ABC", 3);
     i.copy_in(3, expected);
@@ -1902,10 +1908,10 @@ TEST(BufferList, begin) {
 
 TEST(BufferList, end) {
   bufferlist bl;
-  bl.append("ABC");
+  bl.append("AB");
   bufferlist::iterator i = bl.end();
-  i.advance(-1);
-  EXPECT_EQ('C', *i);
+  bl.append("C");
+  EXPECT_EQ('C', bl[i.get_off()]);
 }
 
 TEST(BufferList, copy) {