dbus: remove service name from ExportedObject
Well-known names in D-Bus are merely aliases to unique connection ids
maintained by the bus, they have no purpose in qualifying object paths
or interfaces and it's perfectly legimiate for a client to make
requests to the unique connection id (e.g. in response to a signal,
which does not reference the well-known name of the origin connection).
Remove the service_name member from dbus::ExportedObject, from its
constructor and from dbus::Bus::GetExportedObject and require code to
call dbus::Bus::RequestOwnership if a well-known name is desired. This
requires making that function callable from the origin thread with
a callback for the return value.
BUG=chromium-os:27101
TEST=dbus_unittests
Change-Id: Ib91de8b68ad9c3b432e224a2c715f0c2ca1af463
Review URL: http://codereview.chromium.org/9668018
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125970 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 15e7b16ff24684e2678fe1b4964125b42a4c4018
diff --git a/dbus/bus.cc b/dbus/bus.cc
index 3b2f059..e391bc5 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -233,20 +233,18 @@
return object_proxy.get();
}
-ExportedObject* Bus::GetExportedObject(const std::string& service_name,
- const ObjectPath& object_path) {
+ExportedObject* Bus::GetExportedObject(const ObjectPath& object_path) {
AssertOnOriginThread();
// Check if we already have the requested exported object.
- const std::string key = service_name + object_path.value();
- ExportedObjectTable::iterator iter = exported_object_table_.find(key);
+ ExportedObjectTable::iterator iter = exported_object_table_.find(object_path);
if (iter != exported_object_table_.end()) {
return iter->second;
}
scoped_refptr<ExportedObject> exported_object =
- new ExportedObject(this, service_name, object_path);
- exported_object_table_[key] = exported_object;
+ new ExportedObject(this, object_path);
+ exported_object_table_[object_path] = exported_object;
return exported_object.get();
}
@@ -339,7 +337,40 @@
LOG_IF(ERROR, !signaled) << "Failed to shutdown the bus";
}
-bool Bus::RequestOwnership(const std::string& service_name) {
+void Bus::RequestOwnership(const std::string& service_name,
+ OnOwnershipCallback on_ownership_callback) {
+ AssertOnOriginThread();
+
+ PostTaskToDBusThread(FROM_HERE, base::Bind(
+ &Bus::RequestOwnershipInternal,
+ this, service_name, on_ownership_callback));
+}
+
+void Bus::RequestOwnershipInternal(const std::string& service_name,
+ OnOwnershipCallback on_ownership_callback) {
+ AssertOnDBusThread();
+
+ bool success = Connect();
+ if (success)
+ success = RequestOwnershipAndBlock(service_name);
+
+ PostTaskToOriginThread(FROM_HERE,
+ base::Bind(&Bus::OnOwnership,
+ this,
+ on_ownership_callback,
+ service_name,
+ success));
+}
+
+void Bus::OnOwnership(OnOwnershipCallback on_ownership_callback,
+ const std::string& service_name,
+ bool success) {
+ AssertOnOriginThread();
+
+ on_ownership_callback.Run(service_name, success);
+}
+
+bool Bus::RequestOwnershipAndBlock(const std::string& service_name) {
DCHECK(connection_);
// dbus_bus_request_name() is a blocking call.
AssertOnDBusThread();
diff --git a/dbus/bus.h b/dbus/bus.h
index e045386..0cafd9e 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -176,6 +176,12 @@
// Connect() is called.
explicit Bus(const Options& options);
+ // Called when an ownership request is complete.
+ // Parameters:
+ // - the requested service name.
+ // - whether ownership has been obtained or not.
+ typedef base::Callback<void (const std::string&, bool)> OnOwnershipCallback;
+
// Gets the object proxy for the given service name and the object path.
// The caller must not delete the returned object.
//
@@ -204,12 +210,11 @@
const ObjectPath& object_path,
int options);
- // Gets the exported object for the given service name and the object
- // path. The caller must not delete the returned object.
+ // Gets the exported object for the given object path.
+ // The caller must not delete the returned object.
//
// Returns an existing exported object if the bus object already owns
- // the exported object for the given service name and the object path.
- // Never returns NULL.
+ // the exported object for the given object path. Never returns NULL.
//
// The bus will own all exported objects created by the bus, to ensure
// that the exported objects are unregistered at the shutdown time of
@@ -219,8 +224,7 @@
// send signal from them.
//
// Must be called in the origin thread.
- virtual ExportedObject* GetExportedObject(const std::string& service_name,
- const ObjectPath& object_path);
+ virtual ExportedObject* GetExportedObject(const ObjectPath& object_path);
// Shuts down the bus and blocks until it's done. More specifically, this
// function does the following:
@@ -255,11 +259,21 @@
// BLOCKING CALL.
virtual bool Connect();
+ // Requests the ownership of the service name given by |service_name|.
+ // See also RequestOwnershipAndBlock().
+ //
+ // |on_ownership_callback| is called when the service name is obtained
+ // or failed to be obtained, in the origin thread.
+ //
+ // Must be called in the origin thread.
+ virtual void RequestOwnership(const std::string& service_name,
+ OnOwnershipCallback on_ownership_callback);
+
// Requests the ownership of the given service name.
// Returns true on success, or the the service name is already obtained.
//
// BLOCKING CALL.
- virtual bool RequestOwnership(const std::string& service_name);
+ virtual bool RequestOwnershipAndBlock(const std::string& service_name);
// Releases the ownership of the given service name.
// Returns true on success.
@@ -409,6 +423,15 @@
// Helper function used for ShutdownOnDBusThreadAndBlock().
void ShutdownOnDBusThreadAndBlockInternal();
+ // Helper function used for RequestOwnership().
+ void RequestOwnershipInternal(const std::string& service_name,
+ OnOwnershipCallback on_ownership_callback);
+
+ // Called when the ownership request is completed.
+ void OnOwnership(OnOwnershipCallback on_ownership_callback,
+ const std::string& service_name,
+ bool success);
+
// Processes the all incoming data to the connection, if any.
//
// BLOCKING CALL.
@@ -478,7 +501,7 @@
// ExportedObjectTable is used to hold the exported objects created by
// the bus object. Key is a concatenated string of service name +
// object path, like "org.chromium.TestService/org/chromium/TestObject".
- typedef std::map<std::string,
+ typedef std::map<const dbus::ObjectPath,
scoped_refptr<dbus::ExportedObject> > ExportedObjectTable;
ExportedObjectTable exported_object_table_;
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index c66a05b..4e7e8fd 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -89,21 +89,18 @@
scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
dbus::ExportedObject* object_proxy1 =
- bus->GetExportedObject("org.chromium.TestService",
- dbus::ObjectPath("/org/chromium/TestObject"));
+ bus->GetExportedObject(dbus::ObjectPath("/org/chromium/TestObject"));
ASSERT_TRUE(object_proxy1);
// This should return the same object.
dbus::ExportedObject* object_proxy2 =
- bus->GetExportedObject("org.chromium.TestService",
- dbus::ObjectPath("/org/chromium/TestObject"));
+ bus->GetExportedObject(dbus::ObjectPath("/org/chromium/TestObject"));
ASSERT_TRUE(object_proxy2);
EXPECT_EQ(object_proxy1, object_proxy2);
// This should not.
dbus::ExportedObject* object_proxy3 =
bus->GetExportedObject(
- "org.chromium.TestService",
dbus::ObjectPath("/org/chromium/DifferentTestObject"));
ASSERT_TRUE(object_proxy3);
EXPECT_NE(object_proxy1, object_proxy3);
diff --git a/dbus/exported_object.cc b/dbus/exported_object.cc
index 730c98b..7c4bada 100644
--- a/dbus/exported_object.cc
+++ b/dbus/exported_object.cc
@@ -35,10 +35,8 @@
} // namespace
ExportedObject::ExportedObject(Bus* bus,
- const std::string& service_name,
const ObjectPath& object_path)
: bus_(bus),
- service_name_(service_name),
object_path_(object_path),
object_is_registered_(false) {
}
@@ -65,8 +63,6 @@
return false;
if (!bus_->SetUpAsyncOperations())
return false;
- if (!bus_->RequestOwnership(service_name_))
- return false;
if (!Register())
return false;
diff --git a/dbus/exported_object.h b/dbus/exported_object.h
index 24db66e..315ad98 100644
--- a/dbus/exported_object.h
+++ b/dbus/exported_object.h
@@ -35,9 +35,7 @@
public:
// Client code should use Bus::GetExportedObject() instead of this
// constructor.
- ExportedObject(Bus* bus,
- const std::string& service_name,
- const ObjectPath& object_path);
+ ExportedObject(Bus* bus, const ObjectPath& object_path);
// Called to send a response from an exported method. Response* is the
// response message. Callers should pass a NULL Response* in the event
@@ -157,7 +155,6 @@
void* user_data);
scoped_refptr<Bus> bus_;
- std::string service_name_;
ObjectPath object_path_;
bool object_is_registered_;
diff --git a/dbus/mock_bus.h b/dbus/mock_bus.h
index 463790a..88fc084 100644
--- a/dbus/mock_bus.h
+++ b/dbus/mock_bus.h
@@ -26,13 +26,15 @@
ObjectProxy*(const std::string& service_name,
const ObjectPath& object_path,
int options));
- MOCK_METHOD2(GetExportedObject, ExportedObject*(
- const std::string& service_name,
+ MOCK_METHOD1(GetExportedObject, ExportedObject*(
const ObjectPath& object_path));
MOCK_METHOD0(ShutdownAndBlock, void());
MOCK_METHOD0(ShutdownOnDBusThreadAndBlock, void());
MOCK_METHOD0(Connect, bool());
- MOCK_METHOD1(RequestOwnership, bool(const std::string& service_name));
+ MOCK_METHOD2(RequestOwnership, void(
+ const std::string& service_name,
+ OnOwnershipCallback on_ownership_callback));
+ MOCK_METHOD1(RequestOwnershipAndBlock, bool(const std::string& service_name));
MOCK_METHOD1(ReleaseOwnership, bool(const std::string& service_name));
MOCK_METHOD0(SetUpAsyncOperations, bool());
MOCK_METHOD3(SendWithReplyAndBlock, DBusMessage*(DBusMessage* request,
diff --git a/dbus/mock_exported_object.cc b/dbus/mock_exported_object.cc
index f49cd3d..ff507dd 100644
--- a/dbus/mock_exported_object.cc
+++ b/dbus/mock_exported_object.cc
@@ -7,9 +7,8 @@
namespace dbus {
MockExportedObject::MockExportedObject(Bus* bus,
- const std::string& service_name,
const ObjectPath& object_path)
- : ExportedObject(bus, service_name, object_path) {
+ : ExportedObject(bus, object_path) {
}
MockExportedObject::~MockExportedObject() {
diff --git a/dbus/mock_exported_object.h b/dbus/mock_exported_object.h
index 7deb111..07b2f00 100644
--- a/dbus/mock_exported_object.h
+++ b/dbus/mock_exported_object.h
@@ -18,7 +18,6 @@
class MockExportedObject : public ExportedObject {
public:
MockExportedObject(Bus* bus,
- const std::string& service_name,
const ObjectPath& object_path);
virtual ~MockExportedObject();