From 15e7b16ff24684e2678fe1b4964125b42a4c4018 Mon Sep 17 00:00:00 2001 From: "keybuk@chromium.org" Date: Sat, 10 Mar 2012 01:12:52 +0000 Subject: [PATCH] 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 --- .../chromeos/dbus/cros_dbus_service.cc | 12 ++++- .../dbus/cros_dbus_service_unittest.cc | 3 +- ...xy_resolution_service_provider_unittest.cc | 1 - dbus/bus.cc | 45 ++++++++++++++++--- dbus/bus.h | 39 ++++++++++++---- dbus/bus_unittest.cc | 7 +-- dbus/exported_object.cc | 4 -- dbus/exported_object.h | 5 +-- dbus/mock_bus.h | 8 ++-- dbus/mock_exported_object.cc | 3 +- dbus/mock_exported_object.h | 1 - dbus/test_service.cc | 17 +++++-- dbus/test_service.h | 4 ++ 13 files changed, 108 insertions(+), 41 deletions(-) diff --git a/chrome/browser/chromeos/dbus/cros_dbus_service.cc b/chrome/browser/chromeos/dbus/cros_dbus_service.cc index 5f912ed2ff28..8552aca9b081 100644 --- a/chrome/browser/chromeos/dbus/cros_dbus_service.cc +++ b/chrome/browser/chromeos/dbus/cros_dbus_service.cc @@ -4,6 +4,7 @@ #include "chrome/browser/chromeos/dbus/cros_dbus_service.h" +#include "base/bind.h" #include "base/stl_util.h" #include "base/threading/platform_thread.h" #include "chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h" @@ -39,8 +40,11 @@ class CrosDBusServiceImpl : public CrosDBusService { if (service_started_) return; + bus_->RequestOwnership(kLibCrosServiceName, + base::Bind(&CrosDBusServiceImpl::OnOwnership, + base::Unretained(this))); + exported_object_ = bus_->GetExportedObject( - kLibCrosServiceName, dbus::ObjectPath(kLibCrosServicePath)); for (size_t i = 0; i < service_providers_.size(); ++i) @@ -63,6 +67,12 @@ class CrosDBusServiceImpl : public CrosDBusService { return base::PlatformThread::CurrentId() == origin_thread_id_; } + // Called when an ownership request is completed. + void OnOwnership(const std::string& service_name, + bool success) { + LOG_IF(ERROR, !success) << "Failed to own: " << service_name; + } + bool service_started_; base::PlatformThreadId origin_thread_id_; dbus::Bus* bus_; diff --git a/chrome/browser/chromeos/dbus/cros_dbus_service_unittest.cc b/chrome/browser/chromeos/dbus/cros_dbus_service_unittest.cc index 3dc63590c21a..cb62417d0d6a 100644 --- a/chrome/browser/chromeos/dbus/cros_dbus_service_unittest.cc +++ b/chrome/browser/chromeos/dbus/cros_dbus_service_unittest.cc @@ -48,13 +48,12 @@ class CrosDBusServiceTest : public testing::Test { // org.chromium.CrosDBusService. mock_exported_object_ = new dbus::MockExportedObject(mock_bus_.get(), - kLibCrosServiceName, dbus::ObjectPath(kLibCrosServicePath)); // |mock_bus_|'s GetExportedObject() will return mock_exported_object_| // for the given service name and the object path. EXPECT_CALL(*mock_bus_, GetExportedObject( - kLibCrosServiceName, dbus::ObjectPath(kLibCrosServicePath))) + dbus::ObjectPath(kLibCrosServicePath))) .WillOnce(Return(mock_exported_object_.get())); // Create a mock proxy resolution service. diff --git a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc index e7f3f9fb0567..02f162a66ea5 100644 --- a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc +++ b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc @@ -80,7 +80,6 @@ class ProxyResolutionServiceProviderTest : public testing::Test { // org.chromium.CrosDBusService. mock_exported_object_ = new dbus::MockExportedObject(mock_bus_.get(), - kLibCrosServiceName, dbus::ObjectPath(kLibCrosServicePath)); // |mock_exported_object_|'s ExportMethod() will use diff --git a/dbus/bus.cc b/dbus/bus.cc index 3b2f059c5975..e391bc578b97 100644 --- a/dbus/bus.cc +++ b/dbus/bus.cc @@ -233,20 +233,18 @@ ObjectProxy* Bus::GetObjectProxyWithOptions(const std::string& service_name, 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 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 @@ void Bus::ShutdownOnDBusThreadAndBlock() { 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 e045386cfce1..0cafd9ea8dbc 100644 --- a/dbus/bus.h +++ b/dbus/bus.h @@ -176,6 +176,12 @@ class Bus : public base::RefCountedThreadSafe { // 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 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 @@ class Bus : public base::RefCountedThreadSafe { 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 @@ class Bus : public base::RefCountedThreadSafe { // 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 @@ class Bus : public base::RefCountedThreadSafe { // 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 @@ class Bus : public base::RefCountedThreadSafe { // 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 @@ class Bus : public base::RefCountedThreadSafe { // 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 > ExportedObjectTable; ExportedObjectTable exported_object_table_; diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc index c66a05b4bc5d..4e7e8fd73714 100644 --- a/dbus/bus_unittest.cc +++ b/dbus/bus_unittest.cc @@ -89,21 +89,18 @@ TEST(BusTest, GetExportedObject) { scoped_refptr 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 730c98b2ee76..7c4bada3bbcf 100644 --- a/dbus/exported_object.cc +++ b/dbus/exported_object.cc @@ -35,10 +35,8 @@ std::string GetAbsoluteMethodName( } // 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 @@ bool ExportedObject::ExportMethodAndBlock( 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 24db66e8e52d..315ad986194e 100644 --- a/dbus/exported_object.h +++ b/dbus/exported_object.h @@ -35,9 +35,7 @@ class ExportedObject : public base::RefCountedThreadSafe { 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 @@ class ExportedObject : public base::RefCountedThreadSafe { void* user_data); scoped_refptr 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 463790a2bf30..88fc0844bcf2 100644 --- a/dbus/mock_bus.h +++ b/dbus/mock_bus.h @@ -26,13 +26,15 @@ class MockBus : public Bus { 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 f49cd3d733bc..ff507ddf582d 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 7deb1118d00c..07b2f00e0481 100644 --- a/dbus/mock_exported_object.h +++ b/dbus/mock_exported_object.h @@ -18,7 +18,6 @@ namespace dbus { class MockExportedObject : public ExportedObject { public: MockExportedObject(Bus* bus, - const std::string& service_name, const ObjectPath& object_path); virtual ~MockExportedObject(); diff --git a/dbus/test_service.cc b/dbus/test_service.cc index 23c4bc556ef5..7532d5bc5f17 100644 --- a/dbus/test_service.cc +++ b/dbus/test_service.cc @@ -95,13 +95,21 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) { dbus::MessageWriter writer(&signal); writer.AppendString(message); + bus_->RequestOwnership("org.chromium.TestService", + base::Bind(&TestService::OnOwnership, + base::Unretained(this))); + // Use "/" just like dbus-send does. ExportedObject* root_object = - bus_->GetExportedObject("org.chromium.TestService", - dbus::ObjectPath("/")); + bus_->GetExportedObject(dbus::ObjectPath("/")); root_object->SendSignal(&signal); } +void TestService::OnOwnership(const std::string& service_name, + bool success) { + LOG_IF(ERROR, !success) << "Failed to own: " << service_name; +} + void TestService::OnExported(const std::string& interface_name, const std::string& method_name, bool success) { @@ -125,8 +133,11 @@ void TestService::Run(MessageLoop* message_loop) { bus_options.dbus_thread_message_loop_proxy = dbus_thread_message_loop_proxy_; bus_ = new Bus(bus_options); + bus_->RequestOwnership("org.chromium.TestService", + base::Bind(&TestService::OnOwnership, + base::Unretained(this))); + exported_object_ = bus_->GetExportedObject( - "org.chromium.TestService", dbus::ObjectPath("/org/chromium/TestObject")); int num_methods = 0; diff --git a/dbus/test_service.h b/dbus/test_service.h index 2b49229dfc1a..b5a90c73fce3 100644 --- a/dbus/test_service.h +++ b/dbus/test_service.h @@ -76,6 +76,10 @@ class TestService : public base::Thread { // Helper function for ShutdownAndBlock(). void ShutdownAndBlockInternal(); + // Called when an ownership request is completed. + void OnOwnership(const std::string& service_name, + bool success); + // Called when a method is exported. void OnExported(const std::string& interface_name, const std::string& method_name,