psusensor: Check if the object is still valid in the callback functions
There is a chance that the service will crash if the sensor threshold
is changed. The root cause is that threshold changing will trigger
the deletion of sensors object, when the callback handler is executed
after objects destroyed, the variables of objects are invalid.
Adding a weak_ptr in the callback functions to check if the object
is destroyed already, only access the variables if it is valid.
Tested:
Change/query the PSU sensor threshold using ipmi commands
to trigger the sensor re-creation, the threshold config is correct,
and there is no any psusensor crash log.
Signed-off-by: Yong Li <yong.b.li@linux.intel.com>
Change-Id: Ib8169aeffde18eeaf1a2491d205a808ea15a27d9
diff --git a/src/PSUEvent.cpp b/src/PSUEvent.cpp
index 1e228fe..c8107bd 100644
--- a/src/PSUEvent.cpp
+++ b/src/PSUEvent.cpp
@@ -65,9 +65,12 @@
std::string eventPSUName = eventName + psuName;
for (const auto& path : pathList.second)
{
- events[eventPSUName].emplace_back(std::make_unique<PSUSubEvent>(
+ auto p = std::make_shared<PSUSubEvent>(
eventInterface, path, conn, io, eventName, eventName, assert,
- combineEvent, state, psuName));
+ combineEvent, state, psuName);
+ p->setupRead();
+
+ events[eventPSUName].emplace_back(p);
asserts.emplace_back(assert);
states.emplace_back(state);
}
@@ -85,9 +88,12 @@
std::string eventPSUName = groupEventName + psuName;
for (const auto& path : pathList.second)
{
- events[eventPSUName].emplace_back(std::make_unique<PSUSubEvent>(
+ auto p = std::make_shared<PSUSubEvent>(
eventInterface, path, conn, io, groupEventName,
- groupPathList.first, assert, combineEvent, state, psuName));
+ groupPathList.first, assert, combineEvent, state, psuName);
+ p->setupRead();
+ events[eventPSUName].emplace_back(p);
+
asserts.emplace_back(assert);
states.emplace_back(state);
}
@@ -135,10 +141,11 @@
std::shared_ptr<std::set<std::string>> asserts,
std::shared_ptr<std::set<std::string>> combineEvent,
std::shared_ptr<bool> state, const std::string& psuName) :
- eventInterface(eventInterface),
- asserts(asserts), combineEvent(combineEvent), assertState(state),
- errCount(0), path(path), eventName(eventName), waitTimer(io), inputDev(io),
- psuName(psuName), groupEventName(groupEventName), systemBus(conn)
+ std::enable_shared_from_this<PSUSubEvent>(),
+ eventInterface(eventInterface), asserts(asserts),
+ combineEvent(combineEvent), assertState(state), errCount(0), path(path),
+ eventName(eventName), waitTimer(io), inputDev(io), psuName(psuName),
+ groupEventName(groupEventName), systemBus(conn)
{
fd = open(path.c_str(), O_RDONLY);
if (fd < 0)
@@ -170,15 +177,25 @@
fanName = fanName.substr(0, fanNamePos);
}
}
- setupRead();
}
void PSUSubEvent::setupRead(void)
{
+ std::shared_ptr<boost::asio::streambuf> buffer =
+ std::make_shared<boost::asio::streambuf>();
+ std::weak_ptr<PSUSubEvent> weakRef = weak_from_this();
+
boost::asio::async_read_until(
- inputDev, readBuf, '\n',
- [&](const boost::system::error_code& ec,
- std::size_t /*bytes_transfered*/) { handleResponse(ec); });
+ inputDev, *buffer, '\n',
+ [weakRef, buffer](const boost::system::error_code& ec,
+ std::size_t /*bytes_transfered*/) {
+ std::shared_ptr<PSUSubEvent> self = weakRef.lock();
+ if (self)
+ {
+ self->readBuf = buffer;
+ self->handleResponse(ec);
+ }
+ });
}
PSUSubEvent::~PSUSubEvent()
@@ -189,11 +206,12 @@
void PSUSubEvent::handleResponse(const boost::system::error_code& err)
{
- if (err == boost::system::errc::bad_file_descriptor)
+ if ((err == boost::system::errc::bad_file_descriptor) ||
+ (err == boost::asio::error::misc_errors::not_found))
{
return;
}
- std::istream responseStream(&readBuf);
+ std::istream responseStream(readBuf.get());
if (!err)
{
std::string response;
@@ -226,12 +244,18 @@
}
lseek(fd, 0, SEEK_SET);
waitTimer.expires_from_now(boost::posix_time::milliseconds(eventPollMs));
- waitTimer.async_wait([&](const boost::system::error_code& ec) {
+
+ std::weak_ptr<PSUSubEvent> weakRef = weak_from_this();
+ waitTimer.async_wait([weakRef](const boost::system::error_code& ec) {
+ std::shared_ptr<PSUSubEvent> self = weakRef.lock();
if (ec == boost::asio::error::operation_aborted)
{
return;
}
- setupRead();
+ if (self)
+ {
+ self->setupRead();
+ }
});
}
diff --git a/src/PSUSensor.cpp b/src/PSUSensor.cpp
index 2996b6f..28eec4f 100644
--- a/src/PSUSensor.cpp
+++ b/src/PSUSensor.cpp
@@ -45,8 +45,8 @@
size_t tSize) :
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration, objectType, max, min),
- objServer(objectServer), inputDev(io), waitTimer(io), path(path),
- errCount(0), sensorFactor(factor)
+ std::enable_shared_from_this<PSUSensor>(), objServer(objectServer),
+ inputDev(io), waitTimer(io), path(path), errCount(0), sensorFactor(factor)
{
if constexpr (DEBUG)
{
@@ -96,14 +96,12 @@
association = objectServer.add_interface(dbusPath, association::interface);
createInventoryAssoc(conn, association, configurationPath);
- setupRead();
}
PSUSensor::~PSUSensor()
{
waitTimer.cancel();
inputDev.close();
- objServer.remove_interface(association);
objServer.remove_interface(sensorInterface);
objServer.remove_interface(thresholdInterfaceWarning);
objServer.remove_interface(thresholdInterfaceCritical);
@@ -112,20 +110,31 @@
void PSUSensor::setupRead(void)
{
+ std::shared_ptr<boost::asio::streambuf> buffer =
+ std::make_shared<boost::asio::streambuf>();
+ std::weak_ptr<PSUSensor> weakRef = weak_from_this();
boost::asio::async_read_until(
- inputDev, readBuf, '\n',
- [&](const boost::system::error_code& ec,
- std::size_t /*bytes_transfered*/) { handleResponse(ec); });
+ inputDev, *buffer, '\n',
+ [weakRef, buffer](const boost::system::error_code& ec,
+ std::size_t /*bytes_transfered*/) {
+ std::shared_ptr<PSUSensor> self = weakRef.lock();
+ if (self)
+ {
+ self->readBuf = buffer;
+ self->handleResponse(ec);
+ }
+ });
}
void PSUSensor::handleResponse(const boost::system::error_code& err)
{
- if (err == boost::system::errc::bad_file_descriptor)
+ if ((err == boost::system::errc::bad_file_descriptor) ||
+ (err == boost::asio::error::misc_errors::not_found))
{
std::cerr << "Bad file descriptor from\n";
return;
}
- std::istream responseStream(&readBuf);
+ std::istream responseStream(readBuf.get());
if (!err)
{
std::string response;
@@ -163,13 +172,19 @@
lseek(fd, 0, SEEK_SET);
waitTimer.expires_from_now(boost::posix_time::milliseconds(sensorPollMs));
- waitTimer.async_wait([&](const boost::system::error_code& ec) {
+
+ std::weak_ptr<PSUSensor> weakRef = weak_from_this();
+ waitTimer.async_wait([weakRef](const boost::system::error_code& ec) {
+ std::shared_ptr<PSUSensor> self = weakRef.lock();
if (ec == boost::asio::error::operation_aborted)
{
std::cerr << "Failed to reschedule\n";
return;
}
- setupRead();
+ if (self)
+ {
+ self->setupRead();
+ }
});
}
diff --git a/src/PSUSensorMain.cpp b/src/PSUSensorMain.cpp
index 90c7950..1b363e1 100644
--- a/src/PSUSensorMain.cpp
+++ b/src/PSUSensorMain.cpp
@@ -55,7 +55,7 @@
namespace fs = std::filesystem;
-static boost::container::flat_map<std::string, std::unique_ptr<PSUSensor>>
+static boost::container::flat_map<std::string, std::shared_ptr<PSUSensor>>
sensors;
static boost::container::flat_map<std::string, std::unique_ptr<PSUCombineEvent>>
combineEvents;
@@ -732,12 +732,12 @@
<< "\"\n";
}
- sensors[sensorName] = std::make_unique<PSUSensor>(
+ sensors[sensorName] = std::make_shared<PSUSensor>(
sensorPathStr, sensorType, objectServer, dbusConnection, io,
sensorName, std::move(sensorThresholds), *interfacePath,
findSensorType->second, factor, psuProperty->maxReading,
psuProperty->minReading, labelHead, thresholdConfSize);
-
+ sensors[sensorName]->setupRead();
++numCreated;
if constexpr (DEBUG)
{