From fbb4a23e62500452814b440e8b2bdc040a533c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20Angelovi=C4=8D?= Date: Fri, 16 Feb 2024 18:57:51 +0100 Subject: [PATCH] refactor: rename connection creation methods (#406) This PR makes things around connection factories a little more consistent and more intuitive: * createConnection() has been removed. One shall call more expressive createSystemConnection() instead to get a connection to the system bus. * createDefaultBusConnection() has been renamed to createBusConnection(), so as not to be confused with libsystemd's default_bus, which is a different thing (a reusable thread-local bus). Proxies still by default call createBusConnection() to get a connection when the connection is not provided explicitly by the caller, but now createBusConnection() does a different thing, so now the proxies connect to either session bus or system bus depending on the context (as opposed to always to system bus like before). --- ChangeLog | 19 +++++++-------- docs/using-sdbus-c++.md | 12 ++++------ include/sdbus-c++/IConnection.h | 23 ++---------------- src/Connection.cpp | 24 +++---------------- src/IConnection.h | 1 - src/Proxy.cpp | 4 ++-- .../integrationtests/DBusConnectionTests.cpp | 12 +++++----- tests/integrationtests/DBusGeneralTests.cpp | 2 +- tests/integrationtests/DBusSignalsTests.cpp | 2 +- 9 files changed, 28 insertions(+), 71 deletions(-) diff --git a/ChangeLog b/ChangeLog index a35ac23..23e111d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -242,26 +242,22 @@ v1.4.0 v2.x - Breaking changes in API/ABI/behavior: - - In *synchronous* D-Bus calls, the proxy now **always** blocks the connection for concurrent use until the call finishes (with either a received reply, - an error, or time out). If this creates a connection contention issue in your multi-threaded design, use short-lived, light-weight proxies, or call - the method in an asynchronous way. - - Signatures of callbacks `async_reply_handler`, `signal_handler`, `message_handler` and `property_set_callback` were modified to take input message objects - by value, as opposed to non-const ref. Callee assumes ownership of the message. This API is more idiomatic, cleaner and safer. + - In *synchronous* D-Bus calls, the proxy now **always** blocks the connection for concurrent use until the call finishes (with either a received reply, an error, or time out). If this creates a connection contention issue in your multi-threaded design, use short-lived, light-weight proxies, or call the method in an asynchronous way. + - Signatures of callbacks `async_reply_handler`, `signal_handler`, `message_handler` and `property_set_callback` were modified to take input message objects by value, as opposed to non-const ref. Callee assumes ownership of the message. This API is more idiomatic, cleaner and safer. - The `PollData` struct has been extended with a new data member: `eventFd`. All hooks with external event loops shall be modified to poll on this fd as well. - - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly - what before had to be obtained through `PollData::getAbsoluteTimeout()` call. + - `PollData::timeout_usec` was renamed to `PollData::timeout` and its type has been changed to `std::chrono::microseconds`. This member now holds directly what before had to be obtained through `PollData::getAbsoluteTimeout()` call. - `PollData::getRelativeTimeout()` return type was changed to `std::chrono::microseconds`. - `IConnection::processPendingRequest()` was renamed to `IConnection::processPendingEvent()`. - `Variant` constructor is now explicit. - `IProxy::getCurrentlyProcessedMessage()` now returns `Message` by value instead of a raw pointer to it. The caller assumes ownership of the message. - Object D-Bus API registration is now done through `IObject::addVTable()` method. The registration holds immediately; no `finishRegistration()` call is needed anymore. - - Subscription to signals has been simplified. The subscription is active right after the `registerSignalHandler`/`uponSignal()` call. No need for the final - call to `finishRegistration()`. `IProxy::muteSignal()` has been removed in favor of the RAII-based slot object returned by the slot-returning variant of the - registration method. Destroying the slot object means unsubscribing from the signal. + - Subscription to signals has been simplified. The subscription is active right after the `registerSignalHandler`/`uponSignal()` call. No need for the final call to `finishRegistration()`. `IProxy::muteSignal()` has been removed in favor of the RAII-based slot object returned by the slot-returning variant of the registration method. Destroying the slot object means unsubscribing from the signal. - `request_slot` tag was renamed to `return_slot`. - - `[[nodiscard]]` attribute has been added to relevant API methods. - `ProxyInterfaces::getObjectPath()` was removed, it can be replaced with `ProxyInterfaces::getProxy().getObjectPath()`. - `AdaptorInterfaces::getObjectPath()` was removed, it can be replaced with `AdaptorInterfaces::getObject().getObjectPath()`. + - `createConnection()` has been removed, to create a connection to the system bus use `createSystemConnection()` instead. + - `createDefaultBusConnection()` has been renamed to `createBusConnection()`. + - `Proxy`s now by default call `createBusConnection()` to get a connection when the connection is not provided explicitly by the caller, so they connect to either the session bus or the system bus depending on the context (as opposed to always to the system bus like before). - Callbacks taking `const sdbus::Error* error` were changed to take `std::optional`, which better expresses the intent and meaning. - Types and methods marked deprecated in sdbus-c++ v1 were removed completely. - CMake options got `SDBUSCPP_` prefix for better usability and minimal risk of conflicts in downstream CMake projects. @@ -271,4 +267,5 @@ v2.x - Fix for external event loops in which the event loop thread ID was not correctly initialized (now fixed and simplified by not needing the thread ID anymore) - Introduce native integration for sd-event - Add method to get currently processed message also to `IConnection` +- `[[nodiscard]]` attribute has been added to relevant API methods. - Other simplifications, improvements and fixes springing out from the above refactoring diff --git a/docs/using-sdbus-c++.md b/docs/using-sdbus-c++.md index 76f4430..a790aa5 100644 --- a/docs/using-sdbus-c++.md +++ b/docs/using-sdbus-c++.md @@ -390,7 +390,7 @@ int main(int argc, char *argv[]) } ``` -In simple cases, we don't need to create D-Bus connection explicitly for our proxies. Unless a connection is provided to a proxy object explicitly via factory parameter, the proxy will create a connection of his own (unless it is a light-weight, short-lived proxy created with `dont_run_event_loop_thread_t`), and it will be a system bus connection. This is the case in the example above. (This approach is not scalable and resource-saving if we have plenty of proxies; see section [Working with D-Bus connections](#working-with-d-bus-connections-in-sdbus-c) for elaboration.) So, in the example, we create a proxy for object `/org/sdbuscpp/concatenator` publicly available at bus `org.sdbuscpp.concatenator`. We register handlers for signals we are interested in (if any). +In simple cases, we don't need to create D-Bus connection explicitly for our proxies. We either pass a connection object to the proxy upon creation, or otherwise the proxy will create a connection of his own (to either the session bus or the system bus, depending on the context, see `man sd_bus_open`). This is the case in the example above. (This approach is not scalable and resource-saving if we have plenty of proxies; see section [Working with D-Bus connections](#working-with-d-bus-connections-in-sdbus-c) for elaboration.) So, in the example, we create a proxy for object `/org/sdbuscpp/concatenator` publicly available at bus `org.sdbuscpp.concatenator`. We register handlers for signals we are interested in (if any). The callback for a D-Bus signal handler on this level is any callable of signature `void(sdbus::Signal signal)`. The one and only parameter `signal` is the incoming signal message. We need to deserialize arguments from it, and then we can do our business logic with it. @@ -404,10 +404,8 @@ Please note that we can create and destroy D-Bus object proxies dynamically, at There are several factory methods to create a bus connection object in sdbus-c++: -* `createConnection()` - opens a connection to the system bus -* `createConnection(const std::string& name)` - opens a connection with the given name to the system bus -* `createDefaultBusConnection()` - opens a connection to the session bus when in a user context, and a connection to the system bus, otherwise -* `createDefaultBusConnection(const std::string& name)` - opens a connection with the given name to the session bus when in a user context, and a connection with the given name to the system bus, otherwise +* `createBusConnection()` - opens a connection to the session bus when in a user context, and a connection to the system bus, otherwise +* `createBusConnection(const std::string& name)` - opens a connection with the given name to the session bus when in a user context, and a connection with the given name to the system bus, otherwise * `createSystemBusConnection()` - opens a connection to the system bus * `createSystemBusConnection(const std::string& name)` - opens a connection with the given name to the system bus * `createSessionBusConnection()` - opens a connection to the session bus @@ -467,7 +465,7 @@ On the **client** side we likewise need a connection -- just that unlike on the * when created **without** `dont_run_event_loop_thread_t` tag, the proxy **will start** a dedicated event loop thread on that connection; * or, when created **with** `dont_run_event_loop_thread_t` tag, the proxy will start **no** event loop thread on that connection. - * Or we don't care about connnections at all (proxy factory overloads with no connection parameter). Under the hood, the proxy creates its own *system bus* connection. Additionally: + * Or we don't care about connnections at all (proxy factory overloads with no connection parameter). Under the hood, the proxy creates its own connection, to either the session bus (when in a user context) or the system bus otherwise. Additionally: * when created **without** `dont_run_event_loop_thread_t` tag, the proxy **will start** a dedicated event loop thread on that connection; * or, when created **with** `dont_run_event_loop_thread_t` tag, the proxy will start **no** event loop thread on that connection. @@ -926,7 +924,7 @@ protected: > **_Tip_:** By inheriting from `sdbus::ProxyInterfaces`, we get access to the protected `getProxy()` method. We can call this method inside our proxy implementation class to access the underlying `IProxy` object. -In the above example, a proxy is created that creates and maintains its own system bus connection. However, there are `ProxyInterfaces` class template constructor overloads that also take the connection from the user as the first parameter, and pass that connection over to the underlying proxy. The connection instance is used by all interfaces listed in the `ProxyInterfaces` template parameter list. +In the above example, a proxy is created that creates and maintains its own bus connection (the bus is either session bus or system bus depending on the context, see `man sd_bus_open`). However, there are `ProxyInterfaces` class template constructor overloads that also take the connection from the user as the first parameter, and pass that connection over to the underlying proxy. The connection instance is used by all interfaces listed in the `ProxyInterfaces` template parameter list. Note however that there are multiple `ProxyInterfaces` constructor overloads, and they differ in how the proxy behaves towards the D-Bus connection. These overloads precisely map the `sdbus::createProxy` overloads, as they are actually implemented on top of them. See [Proxy and D-Bus connection](#Proxy-and-D-Bus-connection) for more info. We can even create a `IProxy` instance on our own, and inject it into our proxy class -- there is a constructor overload for it in `ProxyInterfaces`. This can help if we need to provide mocked implementations in our unit tests. diff --git a/include/sdbus-c++/IConnection.h b/include/sdbus-c++/IConnection.h index c3e1e08..74c32c3 100644 --- a/include/sdbus-c++/IConnection.h +++ b/include/sdbus-c++/IConnection.h @@ -395,25 +395,6 @@ namespace sdbus { return setMethodCallTimeout(microsecs.count()); } - /*! - * @brief Creates/opens D-Bus system bus connection - * - * @return Connection instance - * - * @throws sdbus::Error in case of failure - */ - [[nodiscard]] std::unique_ptr createConnection(); - - /*! - * @brief Creates/opens D-Bus system bus connection with a name - * - * @param[in] name Name to request on the connection after its opening - * @return Connection instance - * - * @throws sdbus::Error in case of failure - */ - [[nodiscard]] std::unique_ptr createConnection(const std::string& name); - /*! * @brief Creates/opens D-Bus session bus connection when in a user context, and a system bus connection, otherwise. * @@ -421,7 +402,7 @@ namespace sdbus { * * @throws sdbus::Error in case of failure */ - [[nodiscard]] std::unique_ptr createDefaultBusConnection(); + [[nodiscard]] std::unique_ptr createBusConnection(); /*! * @brief Creates/opens D-Bus session bus connection with a name when in a user context, and a system bus connection with a name, otherwise. @@ -431,7 +412,7 @@ namespace sdbus { * * @throws sdbus::Error in case of failure */ - [[nodiscard]] std::unique_ptr createDefaultBusConnection(const std::string& name); + [[nodiscard]] std::unique_ptr createBusConnection(const std::string& name); /*! * @brief Creates/opens D-Bus system bus connection diff --git a/src/Connection.cpp b/src/Connection.cpp index 5bd329c..4d9b1ad 100644 --- a/src/Connection.cpp +++ b/src/Connection.cpp @@ -872,14 +872,6 @@ int IConnection::PollData::getPollTimeout() const namespace sdbus::internal { -std::unique_ptr createConnection() -{ - auto connection = sdbus::createConnection(); - SCOPE_EXIT{ connection.release(); }; - auto connectionInternal = dynamic_cast(connection.get()); - return std::unique_ptr(connectionInternal); -} - std::unique_ptr createPseudoConnection() { auto interface = std::make_unique(); @@ -892,25 +884,15 @@ namespace sdbus { using internal::Connection; -std::unique_ptr createConnection() -{ - return createSystemBusConnection(); -} - -std::unique_ptr createConnection(const std::string& name) -{ - return createSystemBusConnection(name); -} - -std::unique_ptr createDefaultBusConnection() +std::unique_ptr createBusConnection() { auto interface = std::make_unique(); return std::make_unique(std::move(interface), Connection::default_bus); } -std::unique_ptr createDefaultBusConnection(const std::string& name) +std::unique_ptr createBusConnection(const std::string& name) { - auto conn = createDefaultBusConnection(); + auto conn = createBusConnection(); conn->requestName(name); return conn; } diff --git a/src/IConnection.h b/src/IConnection.h index 7e15c94..be481df 100644 --- a/src/IConnection.h +++ b/src/IConnection.h @@ -98,7 +98,6 @@ namespace sdbus::internal { , void* userData ) = 0; }; - [[nodiscard]] std::unique_ptr createConnection(); [[nodiscard]] std::unique_ptr createPseudoConnection(); } diff --git a/src/Proxy.cpp b/src/Proxy.cpp index fb19c01..e341751 100644 --- a/src/Proxy.cpp +++ b/src/Proxy.cpp @@ -310,7 +310,7 @@ std::unique_ptr createProxy( std::unique_ptr&& conne std::unique_ptr createProxy( std::string destination , std::string objectPath ) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); auto sdbusConnection = std::unique_ptr(dynamic_cast(connection.release())); assert(sdbusConnection != nullptr); @@ -324,7 +324,7 @@ std::unique_ptr createProxy( std::string destination , std::string objectPath , dont_run_event_loop_thread_t ) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); auto sdbusConnection = std::unique_ptr(dynamic_cast(connection.release())); assert(sdbusConnection != nullptr); diff --git a/tests/integrationtests/DBusConnectionTests.cpp b/tests/integrationtests/DBusConnectionTests.cpp index 7ed993c..af9c1cb 100644 --- a/tests/integrationtests/DBusConnectionTests.cpp +++ b/tests/integrationtests/DBusConnectionTests.cpp @@ -48,12 +48,12 @@ using namespace sdbus::test; TEST(Connection, CanBeDefaultConstructed) { - ASSERT_NO_THROW(auto con = sdbus::createConnection()); + ASSERT_NO_THROW(auto con = sdbus::createBusConnection()); } TEST(Connection, CanRequestRegisteredDbusName) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); ASSERT_NO_THROW(connection->requestName(BUS_NAME)) << "Perhaps you've forgotten to copy `org.sdbuscpp.integrationtests.conf` file to `/etc/dbus-1/system.d` directory before running the tests?"; @@ -61,13 +61,13 @@ TEST(Connection, CanRequestRegisteredDbusName) TEST(Connection, CannotRequestNonregisteredDbusName) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); ASSERT_THROW(connection->requestName("some.random.not.supported.dbus.name"), sdbus::Error); } TEST(Connection, CanReleasedRequestedName) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); connection->requestName(BUS_NAME); ASSERT_NO_THROW(connection->releaseName(BUS_NAME)); @@ -75,13 +75,13 @@ TEST(Connection, CanReleasedRequestedName) TEST(Connection, CannotReleaseNonrequestedName) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); ASSERT_THROW(connection->releaseName("some.random.nonrequested.name"), sdbus::Error); } TEST(Connection, CanEnterAndLeaveInternalEventLoop) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); connection->requestName(BUS_NAME); std::thread t([&](){ connection->enterEventLoop(); }); diff --git a/tests/integrationtests/DBusGeneralTests.cpp b/tests/integrationtests/DBusGeneralTests.cpp index c7b318b..51ac470 100644 --- a/tests/integrationtests/DBusGeneralTests.cpp +++ b/tests/integrationtests/DBusGeneralTests.cpp @@ -53,7 +53,7 @@ using ADirectConnection = TestFixtureWithDirectConnection; TEST(AdaptorAndProxy, CanBeConstructedSuccesfully) { - auto connection = sdbus::createConnection(); + auto connection = sdbus::createBusConnection(); connection->requestName(BUS_NAME); ASSERT_NO_THROW(TestAdaptor adaptor(*connection, OBJECT_PATH)); diff --git a/tests/integrationtests/DBusSignalsTests.cpp b/tests/integrationtests/DBusSignalsTests.cpp index c850e9b..33399b7 100644 --- a/tests/integrationtests/DBusSignalsTests.cpp +++ b/tests/integrationtests/DBusSignalsTests.cpp @@ -70,7 +70,7 @@ TYPED_TEST(SdbusTestObject, EmitsSimpleSignalToMultipleProxiesSuccesfully) TYPED_TEST(SdbusTestObject, ProxyDoesNotReceiveSignalFromOtherBusName) { auto otherBusName = BUS_NAME + "2"; - auto connection2 = sdbus::createConnection(otherBusName); + auto connection2 = sdbus::createBusConnection(otherBusName); auto adaptor2 = std::make_unique(*connection2, OBJECT_PATH); adaptor2->emitSimpleSignal();