From 72bcf045f91208f2ac74cc71051a76d0c4d0ef3f Mon Sep 17 00:00:00 2001 From: Ole Petter Date: Wed, 29 Nov 2023 10:25:07 +0100 Subject: [PATCH 1/5] refac(dbus): Wrap the DbusError resource in a smart pointer Just makes it more future proof, as we don't have to remember free it in our exit paths anylonger. Signed-off-by: Ole Petter --- .../dbus/platform/asio_libdbus/dbus.cpp | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/common/dbus/platform/asio_libdbus/dbus.cpp b/src/common/dbus/platform/asio_libdbus/dbus.cpp index 1bda414c3..d55080209 100644 --- a/src/common/dbus/platform/asio_libdbus/dbus.cpp +++ b/src/common/dbus/platform/asio_libdbus/dbus.cpp @@ -42,6 +42,13 @@ namespace log = mender::common::log; using namespace std; +void dbus_error_deleter(DBusError *e) { + if (e) { + dbus_error_free(e); + delete e; + } +} + // The code below integrates ASIO and libdbus. Or, more precisely, it uses // asio::io_context as the main/event loop for libdbus. // @@ -67,15 +74,15 @@ void HandleReply(DBusPendingCall *pending, void *data); DBusHandlerResult MsgFilter(DBusConnection *connection, DBusMessage *message, void *data); error::Error DBusPeer::InitializeConnection() { - DBusError dbus_error; - dbus_error_init(&dbus_error); - dbus_conn_.reset(dbus_bus_get_private(DBUS_BUS_SYSTEM, &dbus_error)); + std::unique_ptr dbus_error { + new DBusError, dbus_error_deleter}; + dbus_error_init(dbus_error.get()); + dbus_conn_.reset(dbus_bus_get_private(DBUS_BUS_SYSTEM, dbus_error.get())); if (!dbus_conn_) { auto err = MakeError( ConnectionError, - string("Failed to get connection to system bus: ") + dbus_error.message + "[" - + dbus_error.name + "]"); - dbus_error_free(&dbus_error); + string("Failed to get connection to system bus: ") + dbus_error.get()->message + "[" + + dbus_error.get()->name + "]"); return err; } @@ -213,13 +220,14 @@ error::Error DBusClient::RegisterSignalHandler( // function below takes care of actually invoking the right handler. const string match_rule = GetSignalMatchRule(iface, signal); - DBusError dbus_error; - dbus_error_init(&dbus_error); - dbus_bus_add_match(dbus_conn_.get(), match_rule.c_str(), &dbus_error); - if (dbus_error_is_set(&dbus_error)) { + std::unique_ptr dbus_error { + new DBusError, dbus_error_deleter}; + dbus_error_init(dbus_error.get()); + dbus_bus_add_match(dbus_conn_.get(), match_rule.c_str(), dbus_error.get()); + if (dbus_error_is_set(dbus_error.get())) { auto err = MakeError( - ConnectionError, string("Failed to register signal reception: ") + dbus_error.message); - dbus_error_free(&dbus_error); + ConnectionError, + string("Failed to register signal reception: ") + dbus_error.get()->message); return err; } AddSignalHandler(match_rule, handler); From 1548f542ad78a84ed0d0a4e036d7e2a4ee6d13d9 Mon Sep 17 00:00:00 2001 From: Ole Petter Date: Wed, 29 Nov 2023 10:26:09 +0100 Subject: [PATCH 2/5] refac(dbus): Replace all NULL instances with nullptr This just makes this consistent with the rest of our codebase. Signed-off-by: Ole Petter --- .../dbus/platform/asio_libdbus/dbus.cpp | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/common/dbus/platform/asio_libdbus/dbus.cpp b/src/common/dbus/platform/asio_libdbus/dbus.cpp index d55080209..8c9a548fc 100644 --- a/src/common/dbus/platform/asio_libdbus/dbus.cpp +++ b/src/common/dbus/platform/asio_libdbus/dbus.cpp @@ -88,17 +88,22 @@ error::Error DBusPeer::InitializeConnection() { dbus_connection_set_exit_on_disconnect(dbus_conn_.get(), FALSE); if (!dbus_connection_set_watch_functions( - dbus_conn_.get(), AddDBusWatch, RemoveDBusWatch, ToggleDBusWatch, this, NULL)) { + dbus_conn_.get(), AddDBusWatch, RemoveDBusWatch, ToggleDBusWatch, this, nullptr)) { dbus_conn_.reset(); return MakeError(ConnectionError, "Failed to set watch functions"); } if (!dbus_connection_set_timeout_functions( - dbus_conn_.get(), AddDBusTimeout, RemoveDBusTimeout, ToggleDBusTimeout, this, NULL)) { + dbus_conn_.get(), + AddDBusTimeout, + RemoveDBusTimeout, + ToggleDBusTimeout, + this, + nullptr)) { dbus_conn_.reset(); return MakeError(ConnectionError, "Failed to set timeout functions"); } - dbus_connection_set_dispatch_status_function(dbus_conn_.get(), HandleDispatch, this, NULL); + dbus_connection_set_dispatch_status_function(dbus_conn_.get(), HandleDispatch, this, nullptr); return error::NoError; } @@ -109,7 +114,7 @@ error::Error DBusClient::InitializeConnection() { return err; } - if (!dbus_connection_add_filter(dbus_conn_.get(), MsgFilter, this, NULL)) { + if (!dbus_connection_add_filter(dbus_conn_.get(), MsgFilter, this, nullptr)) { dbus_conn_.reset(); return MakeError(ConnectionError, "Failed to set message filter"); } @@ -334,14 +339,14 @@ dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { // Assign the stream_descriptor so that we have access to it in // RemoveDBusWatch() and we can delete it. - dbus_watch_set_data(w, sd.release(), NULL); + dbus_watch_set_data(w, sd.release(), nullptr); return TRUE; } static void RemoveDBusWatch(DBusWatch *w, void *data) { asio::posix::stream_descriptor *sd = static_cast(dbus_watch_get_data(w)); - dbus_watch_set_data(w, NULL, NULL); + dbus_watch_set_data(w, nullptr, nullptr); if (sd != nullptr) { sd->cancel(); delete sd; @@ -375,14 +380,14 @@ dbus_bool_t AddDBusTimeout(DBusTimeout *t, void *data) { } }); - dbus_timeout_set_data(t, timer, NULL); + dbus_timeout_set_data(t, timer, nullptr); return TRUE; } static void RemoveDBusTimeout(DBusTimeout *t, void *data) { asio::steady_timer *timer = static_cast(dbus_timeout_get_data(t)); - dbus_timeout_set_data(t, NULL, NULL); + dbus_timeout_set_data(t, nullptr, nullptr); if (timer != nullptr) { timer->cancel(); delete timer; @@ -805,7 +810,7 @@ DBusHandlerResult HandleMethodCall(DBusConnection *connection, DBusMessage *mess } } - if (!dbus_connection_send(connection, reply_msg.get(), NULL)) { + if (!dbus_connection_send(connection, reply_msg.get(), nullptr)) { // can only happen in case of no memory log::Error("Failed to send reply DBus message when handling method " + spec); return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; @@ -862,7 +867,7 @@ error::Error DBusServer::EmitSignal( return MakeError(MessageError, "Failed to add data to the signal message"); } - if (!dbus_connection_send(dbus_conn_.get(), signal_msg.get(), NULL)) { + if (!dbus_connection_send(dbus_conn_.get(), signal_msg.get(), nullptr)) { // can only happen in case of no memory return MakeError(ConnectionError, "Failed to send signal message"); } From 000e10607f660d65c07c41edc8ac21d1e45cb28c Mon Sep 17 00:00:00 2001 From: Ole Petter Date: Wed, 29 Nov 2023 10:32:47 +0100 Subject: [PATCH 3/5] chore: Spelling 'A sync' -> 'Async' Signed-off-by: Ole Petter --- src/api/auth/auth.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/auth/auth.cpp b/src/api/auth/auth.cpp index 375e8e7c8..1cd0e39ac 100644 --- a/src/api/auth/auth.cpp +++ b/src/api/auth/auth.cpp @@ -341,7 +341,7 @@ error::Error Authenticator::RequestNewToken(optional opt_ac } }); if (err != error::NoError) { - // A sync DBus error. + // Async DBus error. mlog::Error("Failed to request new token fetching: " + err.String()); token_fetch_in_progress_ = false; ExpectedAuthData ex_auth_data = expected::unexpected(err); From d21b26248587501802a9574559fecb1042f5d109 Mon Sep 17 00:00:00 2001 From: Ole Petter Date: Thu, 30 Nov 2023 11:00:09 +0100 Subject: [PATCH 4/5] refac(state-machine): Clarify the logic in the RunOne function Simply split out and name the logical parts. Signed-off-by: Ole Petter --- src/common/state_machine.hpp | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/common/state_machine.hpp b/src/common/state_machine.hpp index 15dbce29b..0fa4a3ff7 100644 --- a/src/common/state_machine.hpp +++ b/src/common/state_machine.hpp @@ -156,24 +156,24 @@ class StateMachineRunner : virtual public EventPoster { } private: - void RunOne() { - vector *> to_run; + vector *> FillRunQueueFrom(queue &event_queue) { + vector *> run_queue; for (auto machine : machines_) { if (!machine->state_entered_) { - to_run.push_back(machine->current_state_); + run_queue.push_back(machine->current_state_); machine->state_entered_ = true; } } - const size_t size = event_queue_.size(); + const size_t size = event_queue.size(); - for (size_t count = 0; to_run.empty() && count < size; count++) { + for (size_t count = 0; run_queue.empty() && count < size; count++) { bool deferred = false; - auto event = event_queue_.front(); - event_queue_.pop(); + auto event = event_queue.front(); + event_queue.pop(); - for (auto machine : machines_) { + for (const auto machine : machines_) { typename StateMachine::TransitionCondition cond { machine->current_state_, event}; if (machine->deferred_events_.find(event) != machine->deferred_events_.end()) { @@ -187,16 +187,16 @@ class StateMachineRunner : virtual public EventPoster { } auto &target = match->second; - to_run.push_back(target); + run_queue.push_back(target); machine->current_state_ = target; } - if (to_run.empty()) { + if (run_queue.empty()) { if (deferred) { // Put back in the queue to try later. This won't be tried // again during this run, due to only making `size` // attempts in the for loop. - event_queue_.push(event); + event_queue.push(event); } else { string states = common::BestAvailableTypeName(*machines_[0]->current_state_); for (size_t i = 1; i < machines_.size(); i++) { @@ -212,8 +212,14 @@ class StateMachineRunner : virtual public EventPoster { } } - if (!to_run.empty()) { - for (auto &state : to_run) { + return run_queue; + } + + void RunOne() { + vector *> run_queue = FillRunQueueFrom(event_queue_); + + if (!run_queue.empty()) { + for (auto &state : run_queue) { log::Trace("Entering state " + common::BestAvailableTypeName(*state)); state->OnEnter(ctx_, *this); } From 9cad8e7bd1ef721392c6391bc89bceb599e9dc63 Mon Sep 17 00:00:00 2001 From: Ole Petter Date: Wed, 1 Nov 2023 13:40:09 +0100 Subject: [PATCH 5/5] chore(state-machine): Panic in the case of non-handled non-deferred events This adds a sanity check to verify that there are no non-deferred events left in the event queue after the run_queue has been populated. If there was, then we would have undefined behaviour in the state-machine, and as such the best thing we can do in this instance is to panic. This is a serious logic error in our code, and as such such cause a hard-fault. Signed-off-by: Ole Petter --- src/common/state_machine.hpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/common/state_machine.hpp b/src/common/state_machine.hpp index 0fa4a3ff7..53a135fbb 100644 --- a/src/common/state_machine.hpp +++ b/src/common/state_machine.hpp @@ -215,9 +215,27 @@ class StateMachineRunner : virtual public EventPoster { return run_queue; } + + void FailIfNonDeferredEventsLeftInEventQueue(queue queue_copy) { + // Check if there are any non-deferred events in the queue - then fail if + while (not queue_copy.empty()) { + EventType event = queue_copy.front(); + queue_copy.pop(); + for (const auto machine : machines_) { + if (machine->deferred_events_.find(event) == machine->deferred_events_.end()) { + log::Fatal( + "The state machine has an unprocessed non-deferred event in the queue. This is a programming error!"); + } + } + } + } + + void RunOne() { vector *> run_queue = FillRunQueueFrom(event_queue_); + FailIfNonDeferredEventsLeftInEventQueue(event_queue_); + if (!run_queue.empty()) { for (auto &state : run_queue) { log::Trace("Entering state " + common::BestAvailableTypeName(*state));