Merge pull request #44 from dachary/wip-4070

buffer::ptr self assignment bug + patch

Reviewed-by: Sage Weil <sage@inktank.com>

2 files changed, 53 insertions, 5 deletions

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index b2d3ec6ed8c..58346df2383 100644
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -285,14 +285,14 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
   }
   buffer::ptr& buffer::ptr::operator= (const ptr& p)
   {
-    // be careful -- we need to properly handle self-assignment.
     if (p._raw) {
-      p._raw->nref.inc();                      // inc new
+      p._raw->nref.inc();
       bdout << "ptr " << this << " get " << _raw << bendl;
     }
-    release();                                 // dec (+ dealloc) old (if any)
-    if (p._raw) {
-      _raw = p._raw;
+    buffer::raw *raw = p._raw; 
+    release();
+    if (raw) {
+      _raw = raw;
       _off = p._off;
       _len = p._len;
     } else {
diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 4dafee8cd34..c7668a26541 100644
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -40,6 +40,54 @@ TEST(BufferList, TestPtrAppend) {
   ASSERT_EQ(memcmp(bl.c_str(), correct, curpos), 0);
 }
 
+TEST(BufferList, ptr_assignment) {
+  unsigned len = 17;
+  //
+  // override a bufferptr set with the same raw
+  //
+  {
+    bufferptr original(len);
+    bufferptr same_raw(original.get_raw());
+    unsigned offset = 5;
+    unsigned length = len - offset;
+    original.set_offset(offset);
+    original.set_length(length);
+    same_raw = original;
+    ASSERT_EQ(2, original.raw_nref());
+    ASSERT_EQ(same_raw.get_raw(), original.get_raw());
+    ASSERT_EQ(same_raw.offset(), original.offset());
+    ASSERT_EQ(same_raw.length(), original.length());
+  }
+
+  //
+  // self assignment is a noop
+  //
+  {
+    bufferptr original(len);
+    original = original;
+    ASSERT_EQ(1, original.raw_nref());
+    ASSERT_EQ((unsigned)0, original.offset());
+    ASSERT_EQ(len, original.length());
+  }
+  
+  //
+  // a copy points to the same raw
+  //
+  {
+    bufferptr original(len);
+    unsigned offset = 5;
+    unsigned length = len - offset;
+    original.set_offset(offset);
+    original.set_length(length);
+    bufferptr ptr;
+    ptr = original;
+    ASSERT_EQ(2, original.raw_nref());
+    ASSERT_EQ(ptr.get_raw(), original.get_raw());
+    ASSERT_EQ(original.offset(), ptr.offset());
+    ASSERT_EQ(original.length(), ptr.length());
+  }
+}
+
 TEST(BufferList, TestDirectAppend) {
   bufferlist bl;
   char correct[MAX_TEST];