diff options
Diffstat (limited to 'brillo/dbus')
-rw-r--r-- | brillo/dbus/data_serialization.cc | 9 | ||||
-rw-r--r-- | brillo/dbus/data_serialization.h | 20 | ||||
-rw-r--r-- | brillo/dbus/data_serialization_unittest.cc | 21 | ||||
-rw-r--r-- | brillo/dbus/dbus_method_invoker.h | 26 | ||||
-rw-r--r-- | brillo/dbus/dbus_method_invoker_unittest.cc | 146 | ||||
-rw-r--r-- | brillo/dbus/dbus_object.cc | 91 | ||||
-rw-r--r-- | brillo/dbus/dbus_object.h | 32 | ||||
-rw-r--r-- | brillo/dbus/dbus_object_unittest.cc | 35 | ||||
-rw-r--r-- | brillo/dbus/dbus_signal_handler.h | 10 | ||||
-rw-r--r-- | brillo/dbus/exported_object_manager.h | 7 | ||||
-rw-r--r-- | brillo/dbus/exported_property_set.cc | 15 | ||||
-rw-r--r-- | brillo/dbus/exported_property_set.h | 9 | ||||
-rw-r--r-- | brillo/dbus/exported_property_set_unittest.cc | 12 | ||||
-rw-r--r-- | brillo/dbus/file_descriptor.h | 57 |
14 files changed, 347 insertions, 143 deletions
diff --git a/brillo/dbus/data_serialization.cc b/brillo/dbus/data_serialization.cc index 746b241..4cae471 100644 --- a/brillo/dbus/data_serialization.cc +++ b/brillo/dbus/data_serialization.cc @@ -62,7 +62,7 @@ void AppendValueToWriter(dbus::MessageWriter* writer, } void AppendValueToWriter(dbus::MessageWriter* writer, - const base::ScopedFD& value) { + const FileDescriptor& value) { writer->AppendFileDescriptor(value.get()); } @@ -142,9 +142,8 @@ bool PopValueFromReader(dbus::MessageReader* reader, dbus::ObjectPath* value) { bool PopValueFromReader(dbus::MessageReader* reader, base::ScopedFD* value) { dbus::MessageReader variant_reader(nullptr); - bool ok = details::DescendIntoVariantIfPresent(&reader, &variant_reader) && - reader->PopFileDescriptor(value); - return ok; + return details::DescendIntoVariantIfPresent(&reader, &variant_reader) && + reader->PopFileDescriptor(value); } namespace { @@ -307,7 +306,7 @@ bool PopValueFromReader(dbus::MessageReader* reader, brillo::Any* value) { return false; case dbus::Message::UNIX_FD: CHECK(dbus::IsDBusTypeUnixFdSupported()) << "UNIX_FD data not supported"; - // base::ScopedFD is not a copyable type. Cannot be returned via + // File descriptors don't use copyable types. Cannot be returned via // brillo::Any. Fail here. LOG(ERROR) << "Cannot return FileDescriptor via Any"; return false; diff --git a/brillo/dbus/data_serialization.h b/brillo/dbus/data_serialization.h index 6919b19..1600919 100644 --- a/brillo/dbus/data_serialization.h +++ b/brillo/dbus/data_serialization.h @@ -36,7 +36,8 @@ // | (UVW...) | std::tuple<U,V,W,...> // DICT | a{KV} | std::map<K,V> // VARIANT | v | brillo::Any -// UNIX_FD | h | base::ScopedFD +// UNIX_FD | h | brillo::dbus_utils::FileDescriptor (write) +// | | base::ScopedFD (read) // SIGNATURE | g | (unsupported) // // Additional overloads/specialization can be provided for custom types. @@ -57,9 +58,11 @@ #include <utility> #include <vector> +#include <base/files/scoped_file.h> #include <base/logging.h> #include <base/files/scoped_file.h> #include <brillo/brillo_export.h> +#include <brillo/dbus/file_descriptor.h> #include <brillo/type_name_undecorate.h> #include <dbus/message.h> @@ -412,21 +415,28 @@ struct DBusType<dbus::ObjectPath> { } }; -// base::ScopedFD ------------------------------------------------------------- +// brillo::dbus_utils::FileDescriptor/base::ScopedFD -------------------------- BRILLO_EXPORT void AppendValueToWriter(dbus::MessageWriter* writer, - const base::ScopedFD& value); + const FileDescriptor& value); BRILLO_EXPORT bool PopValueFromReader(dbus::MessageReader* reader, base::ScopedFD* value); template<> -struct DBusType<base::ScopedFD> { +struct DBusType<FileDescriptor> { inline static std::string GetSignature() { return DBUS_TYPE_UNIX_FD_AS_STRING; } inline static void Write(dbus::MessageWriter* writer, - const base::ScopedFD& value) { + const FileDescriptor& value) { AppendValueToWriter(writer, value); } +}; + +template<> +struct DBusType<base::ScopedFD> { + inline static std::string GetSignature() { + return DBUS_TYPE_UNIX_FD_AS_STRING; + } inline static bool Read(dbus::MessageReader* reader, base::ScopedFD* value) { return PopValueFromReader(reader, value); diff --git a/brillo/dbus/data_serialization_unittest.cc b/brillo/dbus/data_serialization_unittest.cc index 8771fe9..c7d5e0f 100644 --- a/brillo/dbus/data_serialization_unittest.cc +++ b/brillo/dbus/data_serialization_unittest.cc @@ -12,7 +12,6 @@ #include "brillo/dbus/test.pb.h" -using base::ScopedFD; using dbus::Message; using dbus::MessageReader; using dbus::MessageWriter; @@ -34,7 +33,8 @@ TEST(DBusUtils, Supported_BasicTypes) { EXPECT_TRUE(IsTypeSupported<double>::value); EXPECT_TRUE(IsTypeSupported<std::string>::value); EXPECT_TRUE(IsTypeSupported<ObjectPath>::value); - EXPECT_TRUE(IsTypeSupported<ScopedFD>::value); + EXPECT_TRUE(IsTypeSupported<FileDescriptor>::value); + EXPECT_TRUE(IsTypeSupported<base::ScopedFD>::value); EXPECT_TRUE(IsTypeSupported<Any>::value); EXPECT_TRUE(IsTypeSupported<google::protobuf::MessageLite>::value); EXPECT_TRUE(IsTypeSupported<dbus_utils_test::TestMessage>::value); @@ -90,7 +90,8 @@ TEST(DBusUtils, Signatures_BasicTypes) { EXPECT_EQ("d", GetDBusSignature<double>()); EXPECT_EQ("s", GetDBusSignature<std::string>()); EXPECT_EQ("o", GetDBusSignature<ObjectPath>()); - EXPECT_EQ("h", GetDBusSignature<ScopedFD>()); + EXPECT_EQ("h", GetDBusSignature<FileDescriptor>()); + EXPECT_EQ("h", GetDBusSignature<base::ScopedFD>()); EXPECT_EQ("v", GetDBusSignature<Any>()); } @@ -106,7 +107,8 @@ TEST(DBusUtils, Signatures_Arrays) { EXPECT_EQ("ad", GetDBusSignature<std::vector<double>>()); EXPECT_EQ("as", GetDBusSignature<std::vector<std::string>>()); EXPECT_EQ("ao", GetDBusSignature<std::vector<ObjectPath>>()); - EXPECT_EQ("ah", GetDBusSignature<std::vector<ScopedFD>>()); + EXPECT_EQ("ah", GetDBusSignature<std::vector<FileDescriptor>>()); + EXPECT_EQ("ah", GetDBusSignature<std::vector<base::ScopedFD>>()); EXPECT_EQ("av", GetDBusSignature<std::vector<Any>>()); EXPECT_EQ("a(is)", (GetDBusSignature<std::vector<std::pair<int, std::string>>>())); @@ -239,24 +241,17 @@ TEST(DBusUtils, AppendAndPopFileDescriptor) { MessageWriter writer(message.get()); // Append stdout. - ScopedFD temp(1); - // Descriptor should not be valid until checked. - EXPECT_FALSE(temp.is_valid()); - // NB: thread IO requirements not relevant for unit tests. - temp.CheckValidity(); - EXPECT_TRUE(temp.is_valid()); + FileDescriptor temp = 1; AppendValueToWriter(&writer, temp); EXPECT_EQ("h", message->GetSignature()); - ScopedFD fd_value; + base::ScopedFD fd_value; MessageReader reader(message.get()); EXPECT_TRUE(reader.HasMoreData()); EXPECT_TRUE(PopValueFromReader(&reader, &fd_value)); EXPECT_FALSE(reader.HasMoreData()); - // Descriptor is automatically checked for validity as part of - // PopValueFromReader() call. EXPECT_TRUE(fd_value.is_valid()); } diff --git a/brillo/dbus/dbus_method_invoker.h b/brillo/dbus/dbus_method_invoker.h index 2a8708b..f8b6990 100644 --- a/brillo/dbus/dbus_method_invoker.h +++ b/brillo/dbus/dbus_method_invoker.h @@ -67,8 +67,10 @@ #include <tuple> #include <base/bind.h> +#include <base/files/scoped_file.h> #include <brillo/dbus/dbus_param_reader.h> #include <brillo/dbus/dbus_param_writer.h> +#include <brillo/dbus/file_descriptor.h> #include <brillo/dbus/utils.h> #include <brillo/errors/error.h> #include <brillo/errors/error_codes.h> @@ -140,6 +142,30 @@ inline std::unique_ptr<dbus::Response> CallMethodAndBlock( args...); } +namespace internal { +// In order to support non-copyable file descriptor types, we have this +// internal::HackMove() helper function that does really nothing for normal +// types but uses Pass() for file descriptors so we can move them out from +// the temporaries created inside DBusParamReader<...>::Invoke(). +// If only libchrome supported real rvalues so we can just do std::move() and +// be done with it. +template <typename T> +inline const T& HackMove(const T& val) { + return val; +} + +// Even though |val| here is passed as const&, the actual value is created +// inside DBusParamReader<...>::Invoke() and is temporary in nature, so it is +// safe to move the file descriptor out of |val|. That's why we are doing +// const_cast here. It is a bit hacky, but there is no negative side effects. +inline base::ScopedFD HackMove(const base::ScopedFD& val) { + return std::move(const_cast<base::ScopedFD&>(val)); +} +inline FileDescriptor HackMove(const FileDescriptor& val) { + return std::move(const_cast<FileDescriptor&>(val)); +} +} // namespace internal + // Extracts the parameters of |ResultTypes...| types from the message reader // and puts the values in the resulting |tuple|. Returns false on error and // provides additional error details in |error| object. diff --git a/brillo/dbus/dbus_method_invoker_unittest.cc b/brillo/dbus/dbus_method_invoker_unittest.cc index dfd10c4..34f4c6f 100644 --- a/brillo/dbus/dbus_method_invoker_unittest.cc +++ b/brillo/dbus/dbus_method_invoker_unittest.cc @@ -26,6 +26,37 @@ using dbus::MessageReader; using dbus::MessageWriter; using dbus::Response; +namespace { + +void SuccessCallback(const std::string& expected_result, + int* counter, + const std::string& actual_result) { + (*counter)++; + EXPECT_EQ(expected_result, actual_result); +} + +void SimpleSuccessCallback(int* counter, const std::string& result) { + (*counter)++; +} + +void ErrorCallback(const std::string& domain, + const std::string& code, + const std::string& message, + int* counter, + brillo::Error* error) { + (*counter)++; + ASSERT_NE(nullptr, error); + EXPECT_EQ(domain, error->GetDomain()); + EXPECT_EQ(code, error->GetCode()); + EXPECT_EQ(message, error->GetMessage()); +} + +void SimpleErrorCallback(int* counter, brillo::Error* error) { + (*counter)++; +} + +} // namespace + namespace brillo { namespace dbus_utils { @@ -95,8 +126,7 @@ class DBusMethodInvokerTest : public testing::Test { if (reader.PopFileDescriptor(&fd)) { auto response = Response::CreateEmpty(); MessageWriter writer(response.get()); - fd.CheckValidity(); - writer.AppendFileDescriptor(fd); + writer.AppendFileDescriptor(fd.get()); return response.release(); } } @@ -131,19 +161,19 @@ class DBusMethodInvokerTest : public testing::Test { return result; } - // Sends a file descriptor received over D-Bus back to the caller. - base::ScopedFD EchoFD(const base::ScopedFD& fd_in) { + // Sends a file descriptor received over D-Bus back to the caller using the + // new types. + base::ScopedFD EchoFD(int fd_in) { std::unique_ptr<dbus::Response> response = - brillo::dbus_utils::CallMethodAndBlock(mock_object_proxy_.get(), - kTestInterface, kTestMethod4, - nullptr, fd_in); + brillo::dbus_utils::CallMethodAndBlock( + mock_object_proxy_.get(), kTestInterface, kTestMethod4, + nullptr, brillo::dbus_utils::FileDescriptor{fd_in}); EXPECT_NE(nullptr, response.get()); base::ScopedFD fd_out; using brillo::dbus_utils::ExtractMethodCallResults; EXPECT_TRUE(ExtractMethodCallResults(response.get(), nullptr, &fd_out)); return fd_out; } - scoped_refptr<dbus::MockBus> bus_; scoped_refptr<dbus::MockObjectProxy> mock_object_proxy_; }; @@ -180,15 +210,18 @@ TEST_F(DBusMethodInvokerTest, TestFileDescriptors) { // Passing a file descriptor over D-Bus would effectively duplicate the fd. // So the resulting file descriptor value would be different but it still // should be valid. - base::ScopedFD fd_stdin(0); - fd_stdin.CheckValidity(); - EXPECT_NE(fd_stdin.value(), EchoFD(fd_stdin).value()); - base::ScopedFD fd_stdout(1); - fd_stdout.CheckValidity(); - EXPECT_NE(fd_stdout.value(), EchoFD(fd_stdout).value()); - base::ScopedFD fd_stderr(2); - fd_stderr.CheckValidity(); - EXPECT_NE(fd_stderr.value(), EchoFD(fd_stderr).value()); + int fd_stdin = 0; + base::ScopedFD out_fd = EchoFD(fd_stdin); + EXPECT_NE(fd_stdin, out_fd.get()); + EXPECT_TRUE(out_fd.is_valid()); + int fd_stdout = 1; + out_fd = EchoFD(fd_stdout); + EXPECT_NE(fd_stdout, out_fd.get()); + EXPECT_TRUE(out_fd.is_valid()); + int fd_stderr = 2; + out_fd = EchoFD(fd_stderr); + EXPECT_NE(fd_stderr, out_fd.get()); + EXPECT_TRUE(out_fd.is_valid()); } ////////////////////////////////////////////////////////////////////////////// @@ -246,62 +279,6 @@ class AsyncDBusMethodInvokerTest : public testing::Test { LOG(FATAL) << "Unexpected method call: " << method_call->ToString(); } - base::Callback<void(const std::string&)> SuccessCallback( - const std::string& in_result, int* in_counter) { - return base::Bind( - [](const std::string& result, - int* counter, - const std::string& actual_result) { - (*counter)++; - EXPECT_EQ(result, actual_result); - }, - in_result, - base::Unretained(in_counter)); - } - - base::Callback<void(const std::string&)> SuccessCallback(int* in_counter) { - return base::Bind( - [](int* counter, const std::string& actual_result) { - (*counter)++; - EXPECT_EQ("", actual_result); - }, - base::Unretained(in_counter)); - } - - AsyncErrorCallback ErrorCallback(int* in_counter) { - return base::Bind( - [](int* counter, brillo::Error* error) { - (*counter)++; - EXPECT_NE(nullptr, error); - EXPECT_EQ("", error->GetDomain()); - EXPECT_EQ("", error->GetCode()); - EXPECT_EQ("", error->GetMessage()); - }, - base::Unretained(in_counter)); - } - - AsyncErrorCallback ErrorCallback(const std::string& domain, - const std::string& code, - const std::string& message, - int* in_counter) { - return base::Bind( - [](const std::string& domain, - const std::string& code, - const std::string& message, - int* counter, - brillo::Error* error) { - (*counter)++; - EXPECT_NE(nullptr, error); - EXPECT_EQ(domain, error->GetDomain()); - EXPECT_EQ(code, error->GetCode()); - EXPECT_EQ(message, error->GetMessage()); - }, - domain, - code, - message, - base::Unretained(in_counter)); - } - scoped_refptr<dbus::MockBus> bus_; scoped_refptr<dbus::MockObjectProxy> mock_object_proxy_; }; @@ -313,22 +290,22 @@ TEST_F(AsyncDBusMethodInvokerTest, TestSuccess) { mock_object_proxy_.get(), kTestInterface, kTestMethod1, - base::Bind(SuccessCallback("4", &success_count)), - base::Bind(ErrorCallback(&error_count)), + base::Bind(SuccessCallback, "4", &success_count), + base::Bind(SimpleErrorCallback, &error_count), 2, 2); brillo::dbus_utils::CallMethod( mock_object_proxy_.get(), kTestInterface, kTestMethod1, - base::Bind(SuccessCallback("10", &success_count)), - base::Bind(ErrorCallback(&error_count)), + base::Bind(SuccessCallback, "10", &success_count), + base::Bind(SimpleErrorCallback, &error_count), 3, 7); brillo::dbus_utils::CallMethod( mock_object_proxy_.get(), kTestInterface, kTestMethod1, - base::Bind(SuccessCallback("-4", &success_count)), - base::Bind(ErrorCallback(&error_count)), + base::Bind(SuccessCallback, "-4", &success_count), + base::Bind(SimpleErrorCallback, &error_count), 13, -17); EXPECT_EQ(0, error_count); EXPECT_EQ(3, success_count); @@ -341,11 +318,12 @@ TEST_F(AsyncDBusMethodInvokerTest, TestFailure) { mock_object_proxy_.get(), kTestInterface, kTestMethod2, - base::Bind(SuccessCallback(&success_count)), - base::Bind(ErrorCallback(brillo::errors::dbus::kDomain, - "org.MyError", - "My error message", - &error_count)), + base::Bind(SimpleSuccessCallback, &success_count), + base::Bind(ErrorCallback, + brillo::errors::dbus::kDomain, + "org.MyError", + "My error message", + &error_count), 2, 2); EXPECT_EQ(1, error_count); EXPECT_EQ(0, success_count); diff --git a/brillo/dbus/dbus_object.cc b/brillo/dbus/dbus_object.cc index 59350a5..512cd6f 100644 --- a/brillo/dbus/dbus_object.cc +++ b/brillo/dbus/dbus_object.cc @@ -16,6 +16,23 @@ namespace brillo { namespace dbus_utils { +namespace { + +void SetupDefaultPropertyHandlers(DBusInterface* prop_interface, + ExportedPropertySet* property_set) { + prop_interface->AddSimpleMethodHandler(dbus::kPropertiesGetAll, + base::Unretained(property_set), + &ExportedPropertySet::HandleGetAll); + prop_interface->AddSimpleMethodHandlerWithError( + dbus::kPropertiesGet, base::Unretained(property_set), + &ExportedPropertySet::HandleGet); + prop_interface->AddSimpleMethodHandlerWithError( + dbus::kPropertiesSet, base::Unretained(property_set), + &ExportedPropertySet::HandleSet); +} + +} // namespace + ////////////////////////////////////////////////////////////////////////////// DBusInterface::DBusInterface(DBusObject* dbus_object, @@ -29,6 +46,11 @@ void DBusInterface::AddProperty(const std::string& property_name, interface_name_, property_name, prop_base); } +void DBusInterface::RemoveProperty(const std::string& property_name) { + dbus_object_->property_set_.UnregisterProperty(interface_name_, + property_name); +} + void DBusInterface::ExportAsync( ExportedObjectManager* object_manager, dbus::Bus* /* bus */, @@ -152,7 +174,21 @@ void DBusInterface::AddSignalImpl( DBusObject::DBusObject(ExportedObjectManager* object_manager, const scoped_refptr<dbus::Bus>& bus, const dbus::ObjectPath& object_path) - : property_set_(bus.get()), bus_(bus), object_path_(object_path) { + : DBusObject::DBusObject(object_manager, + bus, + object_path, + base::Bind(&SetupDefaultPropertyHandlers)) {} + +DBusObject::DBusObject( + ExportedObjectManager* object_manager, + const scoped_refptr<dbus::Bus>& bus, + const dbus::ObjectPath& object_path, + PropertyHandlerSetupCallback property_handler_setup_callback) + : property_set_(bus.get()), + bus_(bus), + object_path_(object_path), + property_handler_setup_callback_( + std::move(property_handler_setup_callback)) { if (object_manager) object_manager_ = object_manager->AsWeakPtr(); } @@ -183,6 +219,21 @@ DBusInterface* DBusObject::FindInterface( return (itf_iter == interfaces_.end()) ? nullptr : itf_iter->second.get(); } +void DBusObject::RemoveInterface(const std::string& interface_name) { + auto itf_iter = interfaces_.find(interface_name); + CHECK(itf_iter != interfaces_.end()) + << "Interface " << interface_name << " has not been added."; + interfaces_.erase(itf_iter); +} + +void DBusObject::ExportInterfaceAsync( + const std::string& interface_name, + const AsyncEventSequencer::CompletionAction& completion_callback) { + AddOrGetInterface(interface_name) + ->ExportAsync(object_manager_.get(), bus_.get(), exported_object_, + object_path_, completion_callback); +} + void DBusObject::RegisterAsync( const AsyncEventSequencer::CompletionAction& completion_callback) { VLOG(1) << "Registering D-Bus object '" << object_path_.value() << "'."; @@ -190,21 +241,7 @@ void DBusObject::RegisterAsync( scoped_refptr<AsyncEventSequencer> sequencer(new AsyncEventSequencer()); exported_object_ = bus_->GetExportedObject(object_path_); - // Add the org.freedesktop.DBus.Properties interface to the object. - DBusInterface* prop_interface = AddOrGetInterface(dbus::kPropertiesInterface); - prop_interface->AddSimpleMethodHandler( - dbus::kPropertiesGetAll, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleGetAll); - prop_interface->AddSimpleMethodHandlerWithError( - dbus::kPropertiesGet, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleGet); - prop_interface->AddSimpleMethodHandlerWithError( - dbus::kPropertiesSet, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleSet); - property_set_.OnPropertiesInterfaceExported(prop_interface); + RegisterPropertiesInterface(); // Export interface methods for (const auto& pair : interfaces_) { @@ -225,21 +262,7 @@ void DBusObject::RegisterAndBlock() { CHECK(exported_object_ == nullptr) << "Object already registered."; exported_object_ = bus_->GetExportedObject(object_path_); - // Add the org.freedesktop.DBus.Properties interface to the object. - DBusInterface* prop_interface = AddOrGetInterface(dbus::kPropertiesInterface); - prop_interface->AddSimpleMethodHandler( - dbus::kPropertiesGetAll, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleGetAll); - prop_interface->AddSimpleMethodHandlerWithError( - dbus::kPropertiesGet, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleGet); - prop_interface->AddSimpleMethodHandlerWithError( - dbus::kPropertiesSet, - base::Unretained(&property_set_), - &ExportedPropertySet::HandleSet); - property_set_.OnPropertiesInterfaceExported(prop_interface); + RegisterPropertiesInterface(); // Export interface methods for (const auto& pair : interfaces_) { @@ -275,5 +298,11 @@ bool DBusObject::SendSignal(dbus::Signal* signal) { return false; } +void DBusObject::RegisterPropertiesInterface() { + DBusInterface* prop_interface = AddOrGetInterface(dbus::kPropertiesInterface); + property_handler_setup_callback_.Run(prop_interface, &property_set_); + property_set_.OnPropertiesInterfaceExported(prop_interface); +} + } // namespace dbus_utils } // namespace brillo diff --git a/brillo/dbus/dbus_object.h b/brillo/dbus/dbus_object.h index 6cd34dd..61c954f 100644 --- a/brillo/dbus/dbus_object.h +++ b/brillo/dbus/dbus_object.h @@ -372,6 +372,9 @@ class BRILLO_EXPORT DBusInterface final { void AddProperty(const std::string& property_name, ExportedPropertyBase* prop_base); + // Unregisters a D-Bus property. + void RemoveProperty(const std::string& property_name); + // Registers a D-Bus signal that has a specified number and types (|Args|) of // arguments. Returns a weak pointer to the DBusSignal object which can be // used to send the signal on this interface when needed: @@ -507,6 +510,9 @@ class BRILLO_EXPORT DBusInterface final { // by this object. class BRILLO_EXPORT DBusObject { public: + using PropertyHandlerSetupCallback = base::Callback<void( + DBusInterface* prop_interface, ExportedPropertySet* property_set)>; + // object_manager - ExportedObjectManager instance that notifies D-Bus // listeners of a new interface being claimed and property // changes on those interfaces. @@ -514,6 +520,17 @@ class BRILLO_EXPORT DBusObject { DBusObject(ExportedObjectManager* object_manager, const scoped_refptr<dbus::Bus>& bus, const dbus::ObjectPath& object_path); + + // property_handler_setup_callback - To be called when setting up property + // method handlers. Clients can register + // their own custom property method handlers + // (GetAll/Get/Set) by passing in this + // callback. + DBusObject(ExportedObjectManager* object_manager, + const scoped_refptr<dbus::Bus>& bus, + const dbus::ObjectPath& object_path, + PropertyHandlerSetupCallback property_handler_setup_callback); + virtual ~DBusObject(); // Returns an proxy handler for the interface |interface_name|. If the @@ -524,6 +541,16 @@ class BRILLO_EXPORT DBusObject { // interface registered by this name. DBusInterface* FindInterface(const std::string& interface_name) const; + // Removes the previously added proxy handler for the interface + // |interface_name|. + void RemoveInterface(const std::string& interface_name); + + // Exports a proxy handler for the interface |interface_name|. If the + // interface proxy does not exist yet, it will be automatically created. + void ExportInterfaceAsync( + const std::string& interface_name, + const AsyncEventSequencer::CompletionAction& completion_callback); + // Registers the object instance with D-Bus. This is an asynchronous call // that will call |completion_callback| when the object and all of its // interfaces are registered. @@ -555,6 +582,9 @@ class BRILLO_EXPORT DBusObject { scoped_refptr<dbus::Bus> GetBus() { return bus_; } private: + // Add the org.freedesktop.DBus.Properties interface to the object. + void RegisterPropertiesInterface(); + // A map of all the interfaces added to this object. std::map<std::string, std::unique_ptr<DBusInterface>> interfaces_; // Exported property set for properties registered with the interfaces @@ -568,6 +598,8 @@ class BRILLO_EXPORT DBusObject { dbus::ObjectPath object_path_; // D-Bus object instance once this object is successfully exported. dbus::ExportedObject* exported_object_ = nullptr; // weak; owned by |bus_|. + // Sets up property method handlers. + PropertyHandlerSetupCallback property_handler_setup_callback_; friend class DBusInterface; DISALLOW_COPY_AND_ASSIGN(DBusObject); diff --git a/brillo/dbus/dbus_object_unittest.cc b/brillo/dbus/dbus_object_unittest.cc index eaec3d8..932a5c8 100644 --- a/brillo/dbus/dbus_object_unittest.cc +++ b/brillo/dbus/dbus_object_unittest.cc @@ -43,6 +43,8 @@ const char kTestMethod_NoOp[] = "NoOp"; const char kTestMethod_WithMessage[] = "TestWithMessage"; const char kTestMethod_WithMessageAsync[] = "TestWithMessageAsync"; +const char kTestInterface4[] = "org.chromium.Test.LateInterface"; + struct Calc { int Add(int x, int y) { return x + y; } int Negate(int x) { return -x; } @@ -89,6 +91,10 @@ void TestWithMessageAsync( response->Return(message->GetSender()); } +void OnInterfaceExported(bool success) { + // Does nothing. +} + } // namespace class DBusObjectTest : public ::testing::Test { @@ -315,6 +321,35 @@ TEST_F(DBusObjectTest, TestWithMessageAsync) { EXPECT_EQ(sender, message); } +TEST_F(DBusObjectTest, TestRemovedInterface) { + // Removes the interface to be tested. + dbus_object_->RemoveInterface(kTestInterface3); + + const std::string sender{":1.2345"}; + dbus::MethodCall method_call(kTestInterface3, kTestMethod_WithMessage); + method_call.SetSerial(123); + method_call.SetSender(sender); + auto response = testing::CallMethod(*dbus_object_, &method_call); + // The response should contain error UnknownInterface since the interface has + // been intentionally removed. + EXPECT_EQ(DBUS_ERROR_UNKNOWN_INTERFACE, response->GetErrorName()); +} + +TEST_F(DBusObjectTest, TestInterfaceExportedLate) { + // Registers a new interface late. + dbus_object_->ExportInterfaceAsync(kTestInterface4, + base::Bind(&OnInterfaceExported)); + + const std::string sender{":1.2345"}; + dbus::MethodCall method_call(kTestInterface4, kTestMethod_WithMessage); + method_call.SetSerial(123); + method_call.SetSender(sender); + auto response = testing::CallMethod(*dbus_object_, &method_call); + // The response should contain error UnknownMethod rather than + // UnknownInterface since the interface has been registered late. + EXPECT_EQ(DBUS_ERROR_UNKNOWN_METHOD, response->GetErrorName()); +} + TEST_F(DBusObjectTest, TooFewParams) { dbus::MethodCall method_call(kTestInterface1, kTestMethod_Add); method_call.SetSerial(123); diff --git a/brillo/dbus/dbus_signal_handler.h b/brillo/dbus/dbus_signal_handler.h index 9c3246c..15cdae1 100644 --- a/brillo/dbus/dbus_signal_handler.h +++ b/brillo/dbus/dbus_signal_handler.h @@ -5,6 +5,7 @@ #ifndef LIBBRILLO_BRILLO_DBUS_DBUS_SIGNAL_HANDLER_H_ #define LIBBRILLO_BRILLO_DBUS_DBUS_SIGNAL_HANDLER_H_ +#include <functional> #include <string> #include <brillo/bind_lambda.h> @@ -58,10 +59,11 @@ void ConnectToSignal( }; // Register our stub handler with D-Bus ObjectProxy. - object_proxy->ConnectToSignal(interface_name, - signal_name, - base::Bind(dbus_signal_callback, signal_callback), - on_connected_callback); + object_proxy->ConnectToSignal( + interface_name, + signal_name, + base::Bind(dbus_signal_callback, signal_callback), + on_connected_callback); } } // namespace dbus_utils diff --git a/brillo/dbus/exported_object_manager.h b/brillo/dbus/exported_object_manager.h index 01dab5b..ea68f33 100644 --- a/brillo/dbus/exported_object_manager.h +++ b/brillo/dbus/exported_object_manager.h @@ -108,6 +108,13 @@ class BRILLO_EXPORT ExportedObjectManager const scoped_refptr<dbus::Bus>& GetBus() const { return bus_; } + // Due to D-Bus forwarding, clients may need to access the underlying + // DBusObject to handle signals/methods. + // TODO(sonnysasaka): Refactor this accessor into a stricter API once we know + // what D-Bus forwarding needs when it's completed, without exposing + // DBusObject directly. + brillo::dbus_utils::DBusObject* dbus_object() { return &dbus_object_; }; + private: BRILLO_PRIVATE ObjectMap HandleGetManagedObjects(); diff --git a/brillo/dbus/exported_property_set.cc b/brillo/dbus/exported_property_set.cc index 8d6ae65..018843e 100644 --- a/brillo/dbus/exported_property_set.cc +++ b/brillo/dbus/exported_property_set.cc @@ -54,6 +54,17 @@ void ExportedPropertySet::RegisterProperty( exported_property->SetUpdateCallback(cb); } +void ExportedPropertySet::UnregisterProperty(const std::string& interface_name, + const std::string& property_name) { + bus_->AssertOnOriginThread(); + auto& prop_map = properties_[interface_name]; + auto prop_iter = prop_map.find(property_name); + CHECK(prop_iter != prop_map.end()) + << "Property '" << property_name << "' doesn't exist"; + prop_iter->second->ClearUpdateCallback(); + prop_map.erase(prop_iter); +} + VariantDictionary ExportedPropertySet::HandleGetAll( const std::string& interface_name) { bus_->AssertOnOriginThread(); @@ -160,6 +171,10 @@ void ExportedPropertyBase::SetUpdateCallback(const OnUpdateCallback& cb) { on_update_callback_ = cb; } +void ExportedPropertyBase::ClearUpdateCallback() { + on_update_callback_.Reset(); +} + void ExportedPropertyBase::SetAccessMode( ExportedPropertyBase::Access access_mode) { access_mode_ = access_mode; diff --git a/brillo/dbus/exported_property_set.h b/brillo/dbus/exported_property_set.h index f10511f..971e932 100644 --- a/brillo/dbus/exported_property_set.h +++ b/brillo/dbus/exported_property_set.h @@ -71,6 +71,9 @@ class BRILLO_EXPORT ExportedPropertyBase { // interface of the exported object. virtual void SetUpdateCallback(const OnUpdateCallback& cb); + // Clears the update callback that was previously set with SetUpdateCallback. + virtual void ClearUpdateCallback(); + // Returns the contained value as Any. virtual brillo::Any GetValue() const = 0; @@ -111,6 +114,10 @@ class BRILLO_EXPORT ExportedPropertySet { const std::string& property_name, ExportedPropertyBase* exported_property); + // Unregisters a property from this exported property set. + void UnregisterProperty(const std::string& interface_name, + const std::string& property_name); + // D-Bus methods for org.freedesktop.DBus.Properties interface. VariantDictionary HandleGetAll(const std::string& interface_name); bool HandleGet(brillo::ErrorPtr* error, @@ -208,7 +215,7 @@ class ExportedProperty : public ExportedPropertyBase { if (!validator_.is_null() && !validator_.Run(error, value.Get<T>())) { return false; } - value_ = value.Get<T>(); + SetValue(value.Get<T>()); return true; } diff --git a/brillo/dbus/exported_property_set_unittest.cc b/brillo/dbus/exported_property_set_unittest.cc index 9233b4a..93aceb4 100644 --- a/brillo/dbus/exported_property_set_unittest.cc +++ b/brillo/dbus/exported_property_set_unittest.cc @@ -336,6 +336,18 @@ TEST_F(ExportedPropertySetTest, GetExtraArgs) { AssertMethodReturnsError(&method_call); } +TEST_F(ExportedPropertySetTest, GetRemovedProperty) { + DBusInterface* itf1 = p_->dbus_object_.AddOrGetInterface(kTestInterface1); + itf1->RemoveProperty(kBoolPropName); + + auto response = GetPropertyOnInterface(kTestInterface1, kBoolPropName); + ASSERT_EQ(DBUS_ERROR_UNKNOWN_PROPERTY, response->GetErrorName()); + + // Signal should not be emitted for removed property. + EXPECT_CALL(*mock_exported_object_, SendSignal(_)).Times(0); + p_->bool_prop_.SetValue(true); +} + TEST_F(ExportedPropertySetTest, GetWorksWithBool) { auto response = GetPropertyOnInterface(kTestInterface1, kBoolPropName); dbus::MessageReader reader(response.get()); diff --git a/brillo/dbus/file_descriptor.h b/brillo/dbus/file_descriptor.h new file mode 100644 index 0000000..f7be44f --- /dev/null +++ b/brillo/dbus/file_descriptor.h @@ -0,0 +1,57 @@ +// Copyright 2018 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_ +#define LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_ + +#include <base/files/scoped_file.h> +#include <base/macros.h> + +namespace brillo { +namespace dbus_utils { + +// This struct wraps file descriptors to give them a type other than int. +// Implicit conversions are provided because this should be as transparent +// a wrapper as possible to match the libchrome bindings below when this +// class is used by chromeos-dbus-bindings. +// +// Because we might pass these around and the calling code neither passes +// ownership nor knows when this will be destroyed, it actually dups the FD +// so that the calling code and binding code both have a clear handle on the +// lifetimes of their respective copies of the FD. +struct FileDescriptor { + FileDescriptor() = default; + FileDescriptor(int fd) : fd(dup(fd)) {} + FileDescriptor(FileDescriptor&& other) : fd(std::move(other.fd)) {} + FileDescriptor(base::ScopedFD&& other) : fd(std::move(other)) {} + + inline FileDescriptor& operator=(int new_fd) { + fd.reset(dup(new_fd)); + return *this; + } + + FileDescriptor& operator=(FileDescriptor&& other) { + fd = std::move(other.fd); + return *this; + } + + FileDescriptor& operator=(base::ScopedFD&& other) { + fd = std::move(other); + return *this; + } + + int release() { return fd.release(); } + + int get() const { return fd.get(); } + + private: + DISALLOW_COPY_AND_ASSIGN(FileDescriptor); + + base::ScopedFD fd; +}; + +} // namespace dbus_utils +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_ |