libbinder: attachObject APIs work with threads
These APIs now return values so that they can be used in a
multi-threaded environment without needing additional locking.
Bug: 192023359
Test: binderLibTest
Change-Id: Idc9054bde869a57c2cb5142963aae362674ce0c0
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 02321cd..4965386 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -311,15 +311,13 @@
return NO_ERROR;
}
-void BBinder::attachObject(
- const void* objectID, void* object, void* cleanupCookie,
- object_cleanup_func func)
-{
+void* BBinder::attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) {
Extras* e = getOrCreateExtras();
- if (!e) return; // out of memory
+ if (!e) return nullptr; // out of memory
AutoMutex _l(e->mLock);
- e->mObjects.attach(objectID, object, cleanupCookie, func);
+ return e->mObjects.attach(objectID, object, cleanupCookie, func);
}
void* BBinder::findObject(const void* objectID) const
@@ -331,13 +329,12 @@
return e->mObjects.find(objectID);
}
-void BBinder::detachObject(const void* objectID)
-{
+void* BBinder::detachObject(const void* objectID) {
Extras* e = mExtras.load(std::memory_order_acquire);
- if (!e) return;
+ if (!e) return nullptr;
AutoMutex _l(e->mLock);
- e->mObjects.detach(objectID);
+ return e->mObjects.detach(objectID);
}
BBinder* BBinder::localBinder()
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 5e44a0f..1f67825 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -61,22 +61,22 @@
kill();
}
-void BpBinder::ObjectManager::attach(
- const void* objectID, void* object, void* cleanupCookie,
- IBinder::object_cleanup_func func)
-{
+void* BpBinder::ObjectManager::attach(const void* objectID, void* object, void* cleanupCookie,
+ IBinder::object_cleanup_func func) {
entry_t e;
e.object = object;
e.cleanupCookie = cleanupCookie;
e.func = func;
- if (mObjects.indexOfKey(objectID) >= 0) {
- ALOGE("Trying to attach object ID %p to binder ObjectManager %p with object %p, but object ID already in use",
- objectID, this, object);
- return;
+ if (ssize_t idx = mObjects.indexOfKey(objectID); idx >= 0) {
+ ALOGI("Trying to attach object ID %p to binder ObjectManager %p with object %p, but object "
+ "ID already in use",
+ objectID, this, object);
+ return mObjects[idx].object;
}
mObjects.add(objectID, e);
+ return nullptr;
}
void* BpBinder::ObjectManager::find(const void* objectID) const
@@ -86,9 +86,12 @@
return mObjects.valueAt(i).object;
}
-void BpBinder::ObjectManager::detach(const void* objectID)
-{
- mObjects.removeItem(objectID);
+void* BpBinder::ObjectManager::detach(const void* objectID) {
+ ssize_t idx = mObjects.indexOfKey(objectID);
+ if (idx < 0) return nullptr;
+ void* value = mObjects[idx].object;
+ mObjects.removeItemsAt(idx, 1);
+ return value;
}
void BpBinder::ObjectManager::kill()
@@ -406,14 +409,11 @@
recipient->binderDied(wp<BpBinder>::fromExisting(this));
}
-
-void BpBinder::attachObject(
- const void* objectID, void* object, void* cleanupCookie,
- object_cleanup_func func)
-{
+void* BpBinder::attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) {
AutoMutex _l(mLock);
ALOGV("Attaching object %p to binder %p (manager=%p)", object, this, &mObjects);
- mObjects.attach(objectID, object, cleanupCookie, func);
+ return mObjects.attach(objectID, object, cleanupCookie, func);
}
void* BpBinder::findObject(const void* objectID) const
@@ -422,10 +422,9 @@
return mObjects.find(objectID);
}
-void BpBinder::detachObject(const void* objectID)
-{
+void* BpBinder::detachObject(const void* objectID) {
AutoMutex _l(mLock);
- mObjects.detach(objectID);
+ return mObjects.detach(objectID);
}
BpBinder* BpBinder::remoteBinder()
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index 472e546..b2fa250 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -54,12 +54,10 @@
uint32_t flags = 0,
wp<DeathRecipient>* outRecipient = nullptr);
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) final;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) final;
virtual void* findObject(const void* objectID) const final;
- virtual void detachObject(const void* objectID) final;
+ virtual void* detachObject(const void* objectID) final;
virtual BBinder* localBinder();
diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h
index 61bf018..7c89759 100644
--- a/libs/binder/include/binder/BpBinder.h
+++ b/libs/binder/include/binder/BpBinder.h
@@ -72,12 +72,10 @@
uint32_t flags = 0,
wp<DeathRecipient>* outRecipient = nullptr);
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) final;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) final;
virtual void* findObject(const void* objectID) const final;
- virtual void detachObject(const void* objectID) final;
+ virtual void* detachObject(const void* objectID) final;
virtual BpBinder* remoteBinder();
@@ -91,27 +89,23 @@
static void setLimitCallback(binder_proxy_limit_callback cb);
static void setBinderProxyCountWatermarks(int high, int low);
- class ObjectManager
- {
+ class ObjectManager {
public:
- ObjectManager();
- ~ObjectManager();
+ ObjectManager();
+ ~ObjectManager();
- void attach( const void* objectID,
- void* object,
- void* cleanupCookie,
- IBinder::object_cleanup_func func);
- void* find(const void* objectID) const;
- void detach(const void* objectID);
+ void* attach(const void* objectID, void* object, void* cleanupCookie,
+ IBinder::object_cleanup_func func);
+ void* find(const void* objectID) const;
+ void* detach(const void* objectID);
- void kill();
+ void kill();
private:
- ObjectManager(const ObjectManager&);
+ ObjectManager(const ObjectManager&);
ObjectManager& operator=(const ObjectManager&);
- struct entry_t
- {
+ struct entry_t {
void* object;
void* cleanupCookie;
IBinder::object_cleanup_func func;
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index f9cdac7..4e0952f 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -255,26 +255,23 @@
* objects are invoked with their respective objectID, object, and
* cleanupCookie. Access to these APIs can be made from multiple threads,
* but calls from different threads are allowed to be interleaved.
+ *
+ * This returns the object which is already attached. If this returns a
+ * non-null value, it means that attachObject failed. TODO(b/192023359):
+ * remove logs and add [[nodiscard]]
*/
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) = 0;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) = 0;
/**
* Returns object attached with attachObject.
*/
virtual void* findObject(const void* objectID) const = 0;
/**
- * WARNING: this API does not call the cleanup function for legacy reasons.
- * It also does not return void* for legacy reasons. If you need to detach
- * an object and destroy it, there are two options:
- * - if you can, don't call detachObject and instead wait for the destructor
- * to clean it up.
- * - manually retrieve and destruct the object (if multiple of your threads
- * are accessing these APIs, you must guarantee that attachObject isn't
- * called after findObject and before detachObject is called).
+ * Returns object attached with attachObject, and detaches it. This does not
+ * delete the object. This is equivalent to using attachObject to attach a null
+ * object.
*/
- virtual void detachObject(const void* objectID) = 0;
+ virtual void* detachObject(const void* objectID) = 0;
virtual BBinder* localBinder();
virtual BpBinder* remoteBinder();
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index 8faa3b8..565f88e 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -84,7 +84,7 @@
enabled: false,
},
},
- srcs: ["binderParcelUnitTest.cpp"],
+ srcs: ["binderParcelUnitTest.cpp", "binderBinderUnitTest.cpp"],
shared_libs: [
"libbinder",
"libutils",
diff --git a/libs/binder/tests/binderBinderUnitTest.cpp b/libs/binder/tests/binderBinderUnitTest.cpp
new file mode 100644
index 0000000..1be0c59
--- /dev/null
+++ b/libs/binder/tests/binderBinderUnitTest.cpp
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <binder/Binder.h>
+#include <binder/IBinder.h>
+#include <gtest/gtest.h>
+
+using android::BBinder;
+using android::OK;
+using android::sp;
+
+const void* kObjectId1 = reinterpret_cast<const void*>(1);
+const void* kObjectId2 = reinterpret_cast<const void*>(2);
+void* kObject1 = reinterpret_cast<void*>(101);
+void* kObject2 = reinterpret_cast<void*>(102);
+void* kObject3 = reinterpret_cast<void*>(103);
+
+TEST(Binder, AttachObject) {
+ auto binder = sp<BBinder>::make();
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject1, nullptr, nullptr));
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId2, kObject2, nullptr, nullptr));
+ EXPECT_EQ(kObject1, binder->attachObject(kObjectId1, kObject3, nullptr, nullptr));
+}
+
+TEST(Binder, DetachObject) {
+ auto binder = sp<BBinder>::make();
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject1, nullptr, nullptr));
+ EXPECT_EQ(kObject1, binder->detachObject(kObjectId1));
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject2, nullptr, nullptr));
+}