From 91c7ecb490606d51d89792cdf534fa7ce032b949 Mon Sep 17 00:00:00 2001
From: Kefu Chai <kchai@redhat.com>
Date: Sat, 7 Sep 2019 01:29:49 +0800
Subject: [PATCH] librados: release pimpl pointer in destructor

before this change, the librados applications are responsible to call
`AioCompletion::release()` explicitly to release its internal pimpl
pointer. this is error prone and not intuitive.

after this change, the destructor of `AioCompletion` and
`PoolAsyncCompletion` will do this automatically. while
`AioCompletion::release()` and `PoolAsyncCompletion::release()` still
delete the instance as they did before. so this change is backward
compatible, as existing librados clients can still use `ptr->release()`
to free the completion instance, while new clients can just `delete
ptr`.

librados_test_stub is updated accordingly to match the new model

Signed-off-by: Kefu Chai <kchai@redhat.com>
---
 src/include/rados/librados.hpp                  |  2 ++
 src/librados/librados_c.cc                      |  1 +
 src/librados/librados_cxx.cc                    | 16 ++++++++++++----
 src/test/librados_test_stub/LibradosTestStub.cc |  8 ++++++--
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/include/rados/librados.hpp b/src/include/rados/librados.hpp
index 69e9c249c67..2cda4d84756 100644
--- a/src/include/rados/librados.hpp
+++ b/src/include/rados/librados.hpp
@@ -191,6 +191,7 @@ inline namespace v14_2_0 {
 
   struct CEPH_RADOS_API AioCompletion {
     AioCompletion(AioCompletionImpl *pc_) : pc(pc_) {}
+    ~AioCompletion();
     int set_complete_callback(void *cb_arg, callback_t cb);
     int set_safe_callback(void *cb_arg, callback_t cb)
       __attribute__ ((deprecated));
@@ -211,6 +212,7 @@ inline namespace v14_2_0 {
 
   struct CEPH_RADOS_API PoolAsyncCompletion {
     PoolAsyncCompletion(PoolAsyncCompletionImpl *pc_) : pc(pc_) {}
+    ~PoolAsyncCompletion();
     int set_callback(void *cb_arg, callback_t cb);
     int wait();
     bool is_complete();
diff --git a/src/librados/librados_c.cc b/src/librados/librados_c.cc
index bbdd9120955..f950b8f2476 100644
--- a/src/librados/librados_c.cc
+++ b/src/librados/librados_c.cc
@@ -3024,6 +3024,7 @@ extern "C" int _rados_aio_unlock(rados_ioctx_t io, const char *o, const char *na
   librados::IoCtx ctx;
   librados::IoCtx::from_rados_ioctx_t(io, ctx);
   librados::AioCompletionImpl *comp = (librados::AioCompletionImpl*)completion;
+  comp->get();
   librados::AioCompletion c(comp);
   int retval = ctx.aio_unlock(o, name, cookie, &c);
   tracepoint(librados, rados_aio_unlock_exit, retval);
diff --git a/src/librados/librados_cxx.cc b/src/librados/librados_cxx.cc
index 4e240fe0c02..0c780a2bd71 100644
--- a/src/librados/librados_cxx.cc
+++ b/src/librados/librados_cxx.cc
@@ -962,6 +962,12 @@ uint32_t librados::NObjectIterator::get_pg_hash_position() const
 const librados::NObjectIterator librados::NObjectIterator::__EndObjectIterator(NULL);
 
 ///////////////////////////// PoolAsyncCompletion //////////////////////////////
+librados::PoolAsyncCompletion::PoolAsyncCompletion::~PoolAsyncCompletion()
+{
+  auto c = reinterpret_cast<PoolAsyncCompletionImpl *>(pc);
+  c->release();
+}
+
 int librados::PoolAsyncCompletion::PoolAsyncCompletion::set_callback(void *cb_arg,
 								     rados_callback_t cb)
 {
@@ -989,12 +995,16 @@ int librados::PoolAsyncCompletion::PoolAsyncCompletion::get_return_value()
 
 void librados::PoolAsyncCompletion::PoolAsyncCompletion::release()
 {
-  PoolAsyncCompletionImpl *c = (PoolAsyncCompletionImpl *)pc;
-  c->release();
   delete this;
 }
 
 ///////////////////////////// AioCompletion //////////////////////////////
+librados::AioCompletion::AioCompletion::~AioCompletion()
+{
+  auto c = reinterpret_cast<AioCompletionImpl *>(pc);
+  c->release();
+}
+
 int librados::AioCompletion::AioCompletion::set_complete_callback(void *cb_arg, rados_callback_t cb)
 {
   AioCompletionImpl *c = (AioCompletionImpl *)pc;
@@ -1075,8 +1085,6 @@ uint64_t librados::AioCompletion::AioCompletion::get_version64()
 
 void librados::AioCompletion::AioCompletion::release()
 {
-  AioCompletionImpl *c = (AioCompletionImpl *)pc;
-  c->release();
   delete this;
 }
 
diff --git a/src/test/librados_test_stub/LibradosTestStub.cc b/src/test/librados_test_stub/LibradosTestStub.cc
index be7af75e4a2..c80dd6fe52b 100644
--- a/src/test/librados_test_stub/LibradosTestStub.cc
+++ b/src/test/librados_test_stub/LibradosTestStub.cc
@@ -345,9 +345,13 @@ extern "C" int rados_wait_for_latest_osdmap(rados_t cluster) {
 
 namespace librados {
 
-void AioCompletion::release() {
-  AioCompletionImpl *c = reinterpret_cast<AioCompletionImpl *>(pc);
+AioCompletion::~AioCompletion()
+{
+  auto c = reinterpret_cast<AioCompletionImpl *>(pc);
   c->release();
+}
+
+void AioCompletion::release() {
   delete this;
 }
 
-- 
2.39.5