librbd: fix valgrind memcheck errors

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc
index a6f8b08cf36cbbf2237f9bfa4e0e4704aa7f3973..8c421215281cb2c8e2f612292c747ab327170d9e 100644 (file)
--- a/src/librbd/ExclusiveLock.cc
+++ b/src/librbd/ExclusiveLock.cc
@@ -113,7 +113,7 @@ void ExclusiveLock<I>::handle_peer_notification(int r) {
 
 template <typename I>
 void ExclusiveLock<I>::handle_init_complete(uint64_t features) {
-  ldout(m_image_ctx.cct, 10) << "features=" << features << dendl;
+  ldout(m_image_ctx.cct, 10) << ": features=" << features << dendl;
 
   if ((features & RBD_FEATURE_JOURNALING) != 0) {
     m_image_ctx.io_work_queue->set_require_lock_on_read();

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 74366f5c633c6f25e9246c6b3be010e5745645f8..fff3e40c9883d70177e232b6cf582a98f90ecc4b 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -629,18 +629,23 @@ struct C_InvalidateCache : public Context {
     return -ENOENT;
   }
 
-  bool ImageCtx::test_flags(uint64_t flags) const
+  int ImageCtx::test_flags(uint64_t flags, bool *flags_set) const
   {
     RWLock::RLocker l(snap_lock);
-    return test_flags(flags, snap_lock);
+    return test_flags(flags, snap_lock, flags_set);
   }
 
-  bool ImageCtx::test_flags(uint64_t flags, const RWLock &in_snap_lock) const
+  int ImageCtx::test_flags(uint64_t flags, const RWLock &in_snap_lock,
+                           bool *flags_set) const
   {
     assert(snap_lock.is_locked());
     uint64_t snap_flags;
-    get_flags(snap_id, &snap_flags);
-    return ((snap_flags & flags) == flags);
+    int r = get_flags(snap_id, &snap_flags);
+    if (r < 0) {
+      return r;
+    }
+    *flags_set = ((snap_flags & flags) == flags);
+    return 0;
   }
 
   int ImageCtx::update_flags(snap_t in_snap_id, uint64_t flag, bool enabled)

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 499f1e2781fae013af6e31085aa51fe4ef1248a9..542680ab97d2037b463a41a329e615ff263220f0 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -269,8 +269,9 @@ namespace librbd {
     bool test_features(uint64_t test_features,
                        const RWLock &in_snap_lock) const;
     int get_flags(librados::snap_t in_snap_id, uint64_t *flags) const;
-    bool test_flags(uint64_t test_flags) const;
-    bool test_flags(uint64_t test_flags, const RWLock &in_snap_lock) const;
+    int test_flags(uint64_t test_flags, bool *flags_set) const;
+    int test_flags(uint64_t test_flags, const RWLock &in_snap_lock,
+                   bool *flags_set) const;
     int update_flags(librados::snap_t in_snap_id, uint64_t flag, bool enabled);
 
     const ParentInfo* get_parent_info(librados::snap_t in_snap_id) const;

diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc
index 9a0683fc68dc40115843acb03a5eb3c9c95143fc..0930e6a56a4bf1c9b0baef6b242c0771f0ab0a59 100644 (file)
--- a/src/librbd/ObjectMap.cc
+++ b/src/librbd/ObjectMap.cc
@@ -91,9 +91,14 @@ bool ObjectMap<I>::object_may_exist(uint64_t object_no) const
 
   // Fall back to default logic if object map is disabled or invalid
   if (!m_image_ctx.test_features(RBD_FEATURE_OBJECT_MAP,
-                                 m_image_ctx.snap_lock) ||
-      m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
-                             m_image_ctx.snap_lock)) {
+                                 m_image_ctx.snap_lock)) {
+    return true;
+  }
+
+  bool flags_set;
+  int r = m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+                                 m_image_ctx.snap_lock, &flags_set);
+  if (r < 0 || flags_set) {
     return true;
   }
 

diff --git a/src/librbd/image/RemoveRequest.cc b/src/librbd/image/RemoveRequest.cc
index 629fc7c799344abb3a86bbfd8a026677b4601a48..cba69c51903da29379fa8e5593c61ed51745ca35 100644 (file)
--- a/src/librbd/image/RemoveRequest.cc
+++ b/src/librbd/image/RemoveRequest.cc
@@ -107,7 +107,8 @@ void RemoveRequest<I>::acquire_exclusive_lock() {
   if (m_force) {
     Context *ctx = create_context_callback<
       klass, &klass::handle_exclusive_lock_force>(this);
-    m_image_ctx->exclusive_lock->shut_down(ctx);
+    m_exclusive_lock = m_image_ctx->exclusive_lock;
+    m_exclusive_lock->shut_down(ctx);
   } else {
     Context *ctx = create_context_callback<
       klass, &klass::handle_exclusive_lock>(this);
@@ -120,6 +121,9 @@ template<typename I>
 Context *RemoveRequest<I>::handle_exclusive_lock_force(int *result) {
   ldout(m_cct, 20) << ": r=" << *result << dendl;
 
+  delete m_exclusive_lock;
+  m_exclusive_lock = nullptr;
+
   if (*result < 0) {
     lderr(m_cct) << "error shutting down exclusive lock: "
                  << cpp_strerror(*result) << dendl;

diff --git a/src/librbd/image/RemoveRequest.h b/src/librbd/image/RemoveRequest.h
index 939abd49dd028ca72c858d8068b356fddf4cf6a1..0853c6b2b87951bbb2e3fe4f000c5b633584dc0a 100644 (file)
--- a/src/librbd/image/RemoveRequest.h
+++ b/src/librbd/image/RemoveRequest.h
@@ -118,6 +118,8 @@ private:
   bool m_unknown_format = true;
   ImageCtxT *m_image_ctx;
 
+  decltype(m_image_ctx->exclusive_lock) m_exclusive_lock = nullptr;
+
   int m_ret_val = 0;
   bufferlist m_out_bl;
   std::list<obj_watch_t> m_watchers;

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index 00606fcd2e6d81c5b12bc3a021629e81e0586fdc..13b8f635238506897f38d87af764750bf6cafd54 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1398,12 +1398,14 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
                        ictx->exclusive_lock->is_lock_owner();
       if (is_locked) {
         C_SaferCond ctx;
-        ictx->exclusive_lock->shut_down(&ctx);
+        auto exclusive_lock = ictx->exclusive_lock;
+        exclusive_lock->shut_down(&ctx);
         ictx->owner_lock.put_read();
         int r = ctx.wait();
         if (r < 0) {
           lderr(cct) << "error shutting down exclusive lock" << dendl;
         }
+        delete exclusive_lock;
       } else {
         ictx->owner_lock.put_read();
       }

diff --git a/src/librbd/object_map/Request.cc b/src/librbd/object_map/Request.cc
index e54aed0b21e0acc18e1f2c4ddb99290a20409c0c..82aaa7d199d644b6873732de870fbb65df80ffa4 100644 (file)
--- a/src/librbd/object_map/Request.cc
+++ b/src/librbd/object_map/Request.cc
@@ -48,7 +48,9 @@ bool Request::should_complete(int r) {
 }
 
 bool Request::invalidate() {
-  if (m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID)) {
+  bool flags_set;
+  int r = m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set);
+  if (r == 0 && flags_set) {
     return true;
   }
 

diff --git a/src/pybind/rbd/rbd.pyx b/src/pybind/rbd/rbd.pyx
index cb03878141d8748000297d77d16cc4bc6616bd2d..40c8843261dfa6b09f11c1cd3faee02c9e4102e7 100644 (file)
--- a/src/pybind/rbd/rbd.pyx
+++ b/src/pybind/rbd/rbd.pyx
@@ -2798,6 +2798,10 @@ cdef class MetadataIterator(object):
                 break
             self.get_next_chunk()
 
+    def __dealloc__(self):
+        if self.last_read:
+            free(self.last_read)
+
     def get_next_chunk(self):
         cdef:
             char *c_keys = NULL

diff --git a/src/test/librbd/object_map/test_mock_InvalidateRequest.cc b/src/test/librbd/object_map/test_mock_InvalidateRequest.cc
index 275d0de805396e49f0927106d667e549bd30990e..e43129f192509a2577817b8da444415687853ad4 100644 (file)
--- a/src/test/librbd/object_map/test_mock_InvalidateRequest.cc
+++ b/src/test/librbd/object_map/test_mock_InvalidateRequest.cc
@@ -26,7 +26,9 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesInMemoryFlag) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 
   C_SaferCond cond_ctx;
   AsyncRequest<> *request = new InvalidateRequest<>(*ictx, CEPH_NOSNAP, false, &cond_ctx);
@@ -42,7 +44,8 @@ TEST_F(TestMockObjectMapInvalidateRequest, UpdatesInMemoryFlag) {
   }
   ASSERT_EQ(0, cond_ctx.wait());
 
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 }
 
 TEST_F(TestMockObjectMapInvalidateRequest, UpdatesHeadOnDiskFlag) {

diff --git a/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc b/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc
index dc0fffee24d47094ea6a96ebd46925d28b83c913..e7fff9482a36ccfed5b1b46f5b27955d632ebb02 100644 (file)
--- a/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc
+++ b/src/test/librbd/object_map/test_mock_SnapshotRollbackRequest.cc
@@ -104,7 +104,9 @@ TEST_F(TestMockObjectMapSnapshotRollbackRequest, ReadMapError) {
     ASSERT_EQ(0, ictx->get_flags(snap_id, &flags));
     ASSERT_NE(0U, flags & RBD_FLAG_OBJECT_MAP_INVALID);
   }
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
   expect_unlock_exclusive_lock(*ictx);
 }
 
@@ -133,7 +135,9 @@ TEST_F(TestMockObjectMapSnapshotRollbackRequest, WriteMapError) {
     ASSERT_EQ(0, ictx->get_flags(snap_id, &flags));
     ASSERT_EQ(0U, flags & RBD_FLAG_OBJECT_MAP_INVALID);
   }
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
   expect_unlock_exclusive_lock(*ictx);
 }
 

diff --git a/src/test/librbd/test_ObjectMap.cc b/src/test/librbd/test_ObjectMap.cc
index ac5732a6f24ee6e44f5a14843298cc81294faa76..464c233faad0fad0c6eebe69cbd33e28b1d5a775 100644 (file)
--- a/src/test/librbd/test_ObjectMap.cc
+++ b/src/test/librbd/test_ObjectMap.cc
@@ -29,7 +29,9 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenCorrupt) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
   {
@@ -44,7 +46,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenCorrupt) {
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, bl));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 }
 
 TEST_F(TestObjectMap, RefreshInvalidatesWhenTooSmall) {
@@ -52,7 +55,9 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenTooSmall) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
   {
@@ -68,7 +73,8 @@ TEST_F(TestObjectMap, RefreshInvalidatesWhenTooSmall) {
   ASSERT_EQ(0, ictx->md_ctx.operate(oid, &op));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 }
 
 TEST_F(TestObjectMap, InvalidateFlagOnDisk) {
@@ -76,7 +82,9 @@ TEST_F(TestObjectMap, InvalidateFlagOnDisk) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 
   C_SaferCond lock_ctx;
   {
@@ -91,10 +99,12 @@ TEST_F(TestObjectMap, InvalidateFlagOnDisk) {
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, bl));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 }
 
 TEST_F(TestObjectMap, InvalidateFlagInMemoryOnly) {
@@ -102,7 +112,9 @@ TEST_F(TestObjectMap, InvalidateFlagInMemoryOnly) {
 
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  bool flags_set;
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 
   std::string oid = librbd::ObjectMap<>::object_map_name(ictx->id, CEPH_NOSNAP);
   bufferlist valid_bl;
@@ -113,10 +125,12 @@ TEST_F(TestObjectMap, InvalidateFlagInMemoryOnly) {
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, corrupt_bl));
 
   ASSERT_EQ(0, when_open_object_map(ictx));
-  ASSERT_TRUE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_TRUE(flags_set);
 
   ASSERT_EQ(0, ictx->md_ctx.write_full(oid, valid_bl));
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
-  ASSERT_FALSE(ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID));
+  ASSERT_EQ(0, ictx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID, &flags_set));
+  ASSERT_FALSE(flags_set);
 }
 

diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc
index 0fe8cc0281bf855e61f923a6c33d98ae92ef0d95..04137d95161f77d90ab8308f96cd74ed81ce39d6 100644 (file)
--- a/src/test/librbd/test_librbd.cc
+++ b/src/test/librbd/test_librbd.cc
@@ -1038,26 +1038,28 @@ TEST_F(TestLibRBD, TestGetSnapShotTimeStamp)
   rados_ioctx_create(_cluster, m_pool_name.c_str(), &ioctx);
 
   rbd_image_t image;
-  int order = 0; 
+  int order = 0;
   std::string name = get_temp_image_name();
   uint64_t size = 2 << 20;
   int num_snaps, max_size = 10;
   rbd_snap_info_t snaps[max_size];
-  
+
   ASSERT_EQ(0, create_image(ioctx, name.c_str(), size, &order));
   ASSERT_EQ(0, rbd_open(ioctx, name.c_str(), &image, NULL));
 
   ASSERT_EQ(0, rbd_snap_create(image, "snap1"));
   num_snaps = rbd_snap_list(image, snaps, &max_size);
-  ASSERT_EQ(1, num_snaps); 
+  ASSERT_EQ(1, num_snaps);
   ASSERT_EQ(0, test_get_snapshot_timestamp(image, snaps[0].id));
- 
+  free((void *)snaps[0].name);
 
   ASSERT_EQ(0, rbd_snap_create(image, "snap2"));
   num_snaps = rbd_snap_list(image, snaps, &max_size);
   ASSERT_EQ(2, num_snaps);
-  ASSERT_EQ(0, test_get_snapshot_timestamp(image, snaps[0].id)); 
+  ASSERT_EQ(0, test_get_snapshot_timestamp(image, snaps[0].id));
   ASSERT_EQ(0, test_get_snapshot_timestamp(image, snaps[1].id));
+  free((void *)snaps[0].name);
+  free((void *)snaps[1].name);
 
   ASSERT_EQ(0, rbd_close(image));
 
@@ -4438,6 +4440,7 @@ TEST_F(TestLibRBD, Metadata)
   ASSERT_EQ(0, rbd_close(image));
   ASSERT_EQ(0, rbd_close(image1));
   ASSERT_EQ(0, rbd_close(image2));
+  rados_ioctx_destroy(ioctx);
 }
 
 TEST_F(TestLibRBD, MetadataPP)

diff --git a/src/test/rbd_mirror/test_ImageSync.cc b/src/test/rbd_mirror/test_ImageSync.cc
index cd50af8ded533fbd1c0da1650bf8c19acd71345f..3b9f02f89ba4a99e723f47341312c5b6b97aeab0 100644 (file)
--- a/src/test/rbd_mirror/test_ImageSync.cc
+++ b/src/test/rbd_mirror/test_ImageSync.cc
@@ -285,8 +285,11 @@ TEST_F(TestImageSync, SnapshotStress) {
       RWLock::RLocker snap_locker(m_local_image_ctx->snap_lock);
       local_size = m_local_image_ctx->get_image_size(
         m_local_image_ctx->snap_id);
-      ASSERT_FALSE(m_local_image_ctx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
-                                                 m_local_image_ctx->snap_lock));
+      bool flags_set;
+      ASSERT_EQ(0, m_local_image_ctx->test_flags(RBD_FLAG_OBJECT_MAP_INVALID,
+                                                 m_local_image_ctx->snap_lock,
+                                                 &flags_set));
+      ASSERT_FALSE(flags_set);
     }
 
     ASSERT_EQ(remote_size, local_size);