libbinder: extension mechanism for all binders
See IBinder::getExtension in this CL for how to use this functionality,
this message only explains background/motivation.
This gives a generic way to extend (actually compose) binder interfaces
for downstream codebases. For instance, INetd.aidl has the API "IBinder
getOemNetd();" which allows someone to extend the INetd interface.
However, adding this manually to interfaces has two problems:
- it requires manaul effort by the maintainer
- we don't know who will extend which interfaces should be extended
(this is what justifies adding this extension mechanism in general)
* Fundamental problem these extensions solve:
In Android, there is a very common problem:
- year 1: AOSP maintainer adds interface
- year 1.5: downstream maintainer modifies interface to add
additional features
- year 2: AOSP maintainer modifies interface
PAIN - year 2.5: downstream maintainer must rebase PAIN
In order to avoid these rebases/merge conflicts in downstream trees,
this mechanism provides a generic way to extend the functionality of
an interface.
This same problem also occurs through chains of downstream
maintainers (perhaps the same interface will be changed many many
times in further downstream branches).
* Other attempts at addressing things (a.k.a. why composition):
HIDL solves this problem via inheritance, but we don't want to port
this to be in AIDL:
- single inheritance is not enough (would need multiple
inheritance).
For instance if we have downstream@1.0::IFoo extends
upstream@1.0::IFoo, if upstream adds upstream@1.1::IFoo (which
by convention extends upstream@1.0::IFoo), then downstream must
rebase their interface to have downstream@2.0::IFoo extends
upstream@1.1::IFoo.
- multiple inheritance is really messy
Bug: 136027762
Test: binderLibTest
Change-Id: Ifa6d228e47df3857421ac452c188be225bb16016
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 9e55c2c..221c002 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -81,6 +81,24 @@
return target->transact(SHELL_COMMAND_TRANSACTION, send, &reply);
}
+status_t IBinder::getExtension(sp<IBinder>* out) {
+ BBinder* local = this->localBinder();
+ if (local != nullptr) {
+ *out = local->getExtension();
+ return OK;
+ }
+
+ BpBinder* proxy = this->remoteBinder();
+ LOG_ALWAYS_FATAL_IF(proxy == nullptr);
+
+ Parcel data;
+ Parcel reply;
+ status_t status = transact(EXTENSION_TRANSACTION, data, &reply);
+ if (status != OK) return status;
+
+ return reply.readNullableStrongBinder(out);
+}
+
// ---------------------------------------------------------------------------
class BBinder::Extras
@@ -88,6 +106,7 @@
public:
// unlocked objects
bool mRequestingSid = false;
+ sp<IBinder> mExtension;
// for below objects
Mutex mLock;
@@ -130,6 +149,9 @@
case PING_TRANSACTION:
err = pingBinder();
break;
+ case EXTENSION_TRANSACTION:
+ err = reply->writeStrongBinder(getExtension());
+ break;
default:
err = onTransact(code, data, reply, flags);
break;
@@ -222,6 +244,17 @@
e->mRequestingSid = true;
}
+sp<IBinder> BBinder::getExtension() {
+ Extras* e = mExtras.load(std::memory_order_acquire);
+ if (e == nullptr) return nullptr;
+ return e->mExtension;
+}
+
+void BBinder::setExtension(const sp<IBinder>& extension) {
+ Extras* e = getOrCreateExtras();
+ e->mExtension = extension;
+}
+
BBinder::~BBinder()
{
Extras* e = mExtras.load(std::memory_order_relaxed);
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index dec75f5..1095c7f 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -64,6 +64,10 @@
// This must be called before the object is sent to another process. Not thread safe.
void setRequestingSid(bool requestSid);
+ sp<IBinder> getExtension();
+ // This must be called before the object is sent to another process. Not thread safe.
+ void setExtension(const sp<IBinder>& extension);
+
protected:
virtual ~BBinder();
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index aa44285..408037e 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -59,6 +59,7 @@
SHELL_COMMAND_TRANSACTION = B_PACK_CHARS('_','C','M','D'),
INTERFACE_TRANSACTION = B_PACK_CHARS('_', 'N', 'T', 'F'),
SYSPROPS_TRANSACTION = B_PACK_CHARS('_', 'S', 'P', 'R'),
+ EXTENSION_TRANSACTION = B_PACK_CHARS('_', 'E', 'X', 'T'),
// Corresponds to TF_ONE_WAY -- an asynchronous call.
FLAG_ONEWAY = 0x00000001
@@ -86,6 +87,49 @@
Vector<String16>& args, const sp<IShellCallback>& callback,
const sp<IResultReceiver>& resultReceiver);
+ /**
+ * This allows someone to add their own additions to an interface without
+ * having to modify the original interface.
+ *
+ * For instance, imagine if we have this interface:
+ * interface IFoo { void doFoo(); }
+ *
+ * If an unrelated owner (perhaps in a downstream codebase) wants to make a
+ * change to the interface, they have two options:
+ *
+ * A). Historical option that has proven to be BAD! Only the original
+ * author of an interface should change an interface. If someone
+ * downstream wants additional functionality, they should not ever
+ * change the interface or use this method.
+ *
+ * BAD TO DO: interface IFoo { BAD TO DO
+ * BAD TO DO: void doFoo(); BAD TO DO
+ * BAD TO DO: + void doBar(); // adding a method BAD TO DO
+ * BAD TO DO: } BAD TO DO
+ *
+ * B). Option that this method enables!
+ * Leave the original interface unchanged (do not change IFoo!).
+ * Instead, create a new interface in a downstream package:
+ *
+ * package com.<name>; // new functionality in a new package
+ * interface IBar { void doBar(); }
+ *
+ * When registering the interface, add:
+ * sp<MyFoo> foo = new MyFoo; // class in AOSP codebase
+ * sp<MyBar> bar = new MyBar; // custom extension class
+ * foo->setExtension(bar); // use method in BBinder
+ *
+ * Then, clients of IFoo can get this extension:
+ * sp<IBinder> binder = ...;
+ * sp<IFoo> foo = interface_cast<IFoo>(binder); // handle if null
+ * sp<IBinder> barBinder;
+ * ... handle error ... = binder->getExtension(&barBinder);
+ * sp<IBar> bar = interface_cast<IBar>(barBinder);
+ * // if bar is null, then there is no extension or a different
+ * // type of extension
+ */
+ status_t getExtension(sp<IBinder>* out);
+
// NOLINTNEXTLINE(google-default-arguments)
virtual status_t transact( uint32_t code,
const Parcel& data,
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index 4f0c969..5f8887b 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -766,6 +766,24 @@
EXPECT_TRUE(strong_from_weak == nullptr);
}
+TEST_F(BinderLibTest, LocalGetExtension) {
+ sp<BBinder> binder = new BBinder();
+ sp<IBinder> ext = new BBinder();
+ binder->setExtension(ext);
+ EXPECT_EQ(ext, binder->getExtension());
+}
+
+TEST_F(BinderLibTest, RemoteGetExtension) {
+ sp<IBinder> server = addServer();
+ ASSERT_TRUE(server != nullptr);
+
+ sp<IBinder> extension;
+ EXPECT_EQ(NO_ERROR, server->getExtension(&extension));
+ ASSERT_NE(nullptr, extension.get());
+
+ EXPECT_EQ(NO_ERROR, extension->pingBinder());
+}
+
TEST_F(BinderLibTest, CheckHandleZeroBinderHighBitsZeroCookie) {
status_t ret;
Parcel data, reply;
@@ -1170,6 +1188,13 @@
BinderLibTestService* testServicePtr;
{
sp<BinderLibTestService> testService = new BinderLibTestService(index);
+
+ /*
+ * Normally would also contain functionality as well, but we are only
+ * testing the extension mechanism.
+ */
+ testService->setExtension(new BBinder());
+
/*
* We need this below, but can't hold a sp<> because it prevents the
* node from being cleaned up automatically. It's safe in this case