control: Correct subscribing/handling of signals
When subscribing to signals, a pointer to the signal packages should be
used for the location to store the list of signal packages. This way the
callback is bound to the pointer and not the local variable that was
then getting moved.
Also, in handling signal messages, since there could be more than one
signal package to check against the signal received, a rewind must be
done to the signal message so that additional signal packages can read
the message contents again.
Change-Id: Iddea2011d25068d6f71bdee91801537cd5994728
Signed-off-by: Matthew Barth <msbarth@us.ibm.com>
diff --git a/control/json/manager.cpp b/control/json/manager.cpp
index b0da04a..8973d7c 100644
--- a/control/json/manager.cpp
+++ b/control/json/manager.cpp
@@ -28,6 +28,8 @@
#include "sdbusplus.hpp"
#include "zone.hpp"
+#include <systemd/sd-bus.h>
+
#include <nlohmann/json.hpp>
#include <sdbusplus/bus.hpp>
#include <sdbusplus/server/manager.hpp>
@@ -655,9 +657,9 @@
}
void Manager::handleSignal(sdbusplus::message::message& msg,
- const std::vector<SignalPkg>& pkgs)
+ const std::vector<SignalPkg>* pkgs)
{
- for (auto& pkg : pkgs)
+ for (auto& pkg : *pkgs)
{
// Handle the signal callback and only run the actions if the handler
// updated the cache for the given SignalObject
@@ -669,6 +671,11 @@
std::for_each(actions.begin(), actions.end(),
[](auto& action) { action.get()->run(); });
}
+ // Only rewind message when not last package
+ if (&pkg != &pkgs->back())
+ {
+ sd_bus_message_rewind(msg.get(), true);
+ }
}
}
diff --git a/control/json/manager.hpp b/control/json/manager.hpp
index 2b913fe..310132d 100644
--- a/control/json/manager.hpp
+++ b/control/json/manager.hpp
@@ -104,11 +104,12 @@
/**
* Data associated to a subscribed signal
* Tuple constructed of:
- * std::vector<SignalPkg> = List of the signal's packages
+ * std::unique_ptr<std::vector<SignalPkg>> =
+ * Pointer to list of the signal's packages
* std::unique_ptr<sdbusplus::server::match::match> =
* Pointer to match holding the subscription to a signal
*/
-using SignalData = std::tuple<std::vector<SignalPkg>,
+using SignalData = std::tuple<std::unique_ptr<std::vector<SignalPkg>>,
std::unique_ptr<sdbusplus::server::match::match>>;
/**
@@ -411,7 +412,7 @@
* @param[in] pkgs - Signal packages associated to the signal being handled
*/
void handleSignal(sdbusplus::message::message& msg,
- const std::vector<SignalPkg>& pkgs);
+ const std::vector<SignalPkg>* pkgs);
/**
* @brief Get the sdbusplus bus object
diff --git a/control/json/triggers/signal.cpp b/control/json/triggers/signal.cpp
index fb72f89..f430bfe 100644
--- a/control/json/triggers/signal.cpp
+++ b/control/json/triggers/signal.cpp
@@ -49,28 +49,28 @@
if (signalData.empty())
{
// Signal subscription doesnt exist, add signal package and subscribe
- std::vector<SignalPkg> pkgs = {signalPkg};
- std::vector<SignalPkg> dataPkgs =
- std::vector<SignalPkg>(std::move(pkgs));
+ std::unique_ptr<std::vector<SignalPkg>> pkgs =
+ std::make_unique<std::vector<SignalPkg>>();
+ pkgs->emplace_back(std::move(signalPkg));
std::unique_ptr<sdbusplus::server::match::match> ptrMatch = nullptr;
- // TODO(ibm-openbmc/#3195) - Filter signal subscriptions to objects
- // owned by fan control?
if (!match.empty())
{
// Subscribe to signal
ptrMatch = std::make_unique<sdbusplus::server::match::match>(
mgr->getBus(), match.c_str(),
std::bind(std::mem_fn(&Manager::handleSignal), &(*mgr),
- std::placeholders::_1, dataPkgs));
+ std::placeholders::_1, pkgs.get()));
}
- signalData.emplace_back(std::move(dataPkgs), std::move(ptrMatch));
+ signalData.emplace_back(std::move(pkgs), std::move(ptrMatch));
}
else
{
// Signal subscription already exists
// Only a single signal data entry tied to each match is supported
- auto& pkgs = std::get<std::vector<SignalPkg>>(signalData.front());
- for (auto& pkg : pkgs)
+ auto& pkgs = std::get<std::unique_ptr<std::vector<SignalPkg>>>(
+ signalData.front());
+ auto sameSignal = false;
+ for (auto& pkg : *pkgs)
{
if (isSameSig(pkg))
{
@@ -81,12 +81,14 @@
actions.insert(actions.end(),
std::make_move_iterator(pkgActions.begin()),
std::make_move_iterator(pkgActions.end()));
+ sameSignal = true;
+ break;
}
- else
- {
- // Expected signal differs, add signal package
- pkgs.emplace_back(std::move(signalPkg));
- }
+ }
+ if (!sameSignal)
+ {
+ // Expected signal differs, add signal package
+ pkgs->emplace_back(std::move(signalPkg));
}
}
}