From 965ee270baf1f81d47618a12e469ed8940072184 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 31 May 2017 20:46:45 -0400 Subject: [PATCH] librbd: fix valgrind memcheck errors Signed-off-by: Jason Dillaman --- src/librbd/ExclusiveLock.cc | 2 +- src/librbd/ImageCtx.cc | 15 +++++--- src/librbd/ImageCtx.h | 5 +-- src/librbd/ObjectMap.cc | 11 ++++-- src/librbd/image/RemoveRequest.cc | 6 +++- src/librbd/image/RemoveRequest.h | 2 ++ src/librbd/internal.cc | 4 ++- src/librbd/object_map/Request.cc | 4 ++- src/pybind/rbd/rbd.pyx | 4 +++ .../object_map/test_mock_InvalidateRequest.cc | 7 ++-- .../test_mock_SnapshotRollbackRequest.cc | 8 +++-- src/test/librbd/test_ObjectMap.cc | 34 +++++++++++++------ src/test/librbd/test_librbd.cc | 13 ++++--- src/test/rbd_mirror/test_ImageSync.cc | 7 ++-- 14 files changed, 87 insertions(+), 35 deletions(-) diff --git a/src/librbd/ExclusiveLock.cc b/src/librbd/ExclusiveLock.cc index a6f8b08cf36cb..8c421215281cb 100644 --- a/src/librbd/ExclusiveLock.cc +++ b/src/librbd/ExclusiveLock.cc @@ -113,7 +113,7 @@ void ExclusiveLock::handle_peer_notification(int r) { template void ExclusiveLock::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 74366f5c633c6..fff3e40c9883d 100644 --- 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 499f1e2781fae..542680ab97d20 100644 --- 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 9a0683fc68dc4..0930e6a56a4bf 100644 --- a/src/librbd/ObjectMap.cc +++ b/src/librbd/ObjectMap.cc @@ -91,9 +91,14 @@ bool ObjectMap::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 629fc7c799344..cba69c51903da 100644 --- a/src/librbd/image/RemoveRequest.cc +++ b/src/librbd/image/RemoveRequest.cc @@ -107,7 +107,8 @@ void RemoveRequest::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 Context *RemoveRequest::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 939abd49dd028..0853c6b2b8795 100644 --- 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 m_watchers; diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 00606fcd2e6d8..13b8f63523850 100644 --- 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 e54aed0b21e0a..82aaa7d199d64 100644 --- 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 cb03878141d87..40c8843261dfa 100644 --- 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 275d0de805396..e43129f192509 100644 --- 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 dc0fffee24d47..e7fff9482a36c 100644 --- 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 ac5732a6f24ee..464c233faad0f 100644 --- 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 0fe8cc0281bf8..04137d95161f7 100644 --- 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 cd50af8ded533..3b9f02f89ba4a 100644 --- 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); -- 2.39.5