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/include/PSUEvent.hpp b/include/PSUEvent.hpp
index f5bc155..50f6359 100644
--- a/include/PSUEvent.hpp
+++ b/include/PSUEvent.hpp
@@ -23,7 +23,7 @@
#include <string>
#include <vector>
-class PSUSubEvent
+class PSUSubEvent : public std::enable_shared_from_this<PSUSubEvent>
{
public:
PSUSubEvent(std::shared_ptr<sdbusplus::asio::dbus_interface> eventInterface,
@@ -40,6 +40,7 @@
std::shared_ptr<std::set<std::string>> asserts;
std::shared_ptr<std::set<std::string>> combineEvent;
std::shared_ptr<bool> assertState;
+ void setupRead(void);
private:
int value = 0;
@@ -49,8 +50,7 @@
std::string eventName;
std::string groupEventName;
boost::asio::deadline_timer waitTimer;
- boost::asio::streambuf readBuf;
- void setupRead(void);
+ std::shared_ptr<boost::asio::streambuf> readBuf;
void handleResponse(const boost::system::error_code& err);
void updateValue(const int& newValue);
void beep(const uint8_t& beepPriority);
@@ -84,7 +84,7 @@
sdbusplus::asio::object_server& objServer;
std::shared_ptr<sdbusplus::asio::dbus_interface> eventInterface;
boost::container::flat_map<std::string,
- std::vector<std::unique_ptr<PSUSubEvent>>>
+ std::vector<std::shared_ptr<PSUSubEvent>>>
events;
std::vector<std::shared_ptr<std::set<std::string>>> asserts;
std::vector<std::shared_ptr<bool>> states;
diff --git a/include/PSUSensor.hpp b/include/PSUSensor.hpp
index 98ec3d7..811250d 100644
--- a/include/PSUSensor.hpp
+++ b/include/PSUSensor.hpp
@@ -8,7 +8,7 @@
#include <sdbusplus/asio/object_server.hpp>
#include <string>
-class PSUSensor : public Sensor
+class PSUSensor : public Sensor, public std::enable_shared_from_this<PSUSensor>
{
public:
PSUSensor(const std::string& path, const std::string& objectType,
@@ -20,16 +20,16 @@
std::string& sensorTypeName, unsigned int factor, double max,
double min, const std::string& label, size_t tSize);
~PSUSensor();
+ void setupRead(void);
private:
sdbusplus::asio::object_server& objServer;
boost::asio::posix::stream_descriptor inputDev;
boost::asio::deadline_timer waitTimer;
- boost::asio::streambuf readBuf;
+ std::shared_ptr<boost::asio::streambuf> readBuf;
std::string path;
size_t errCount;
unsigned int sensorFactor;
- void setupRead(void);
void handleResponse(const boost::system::error_code& err);
void checkThresholds(void) override;
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)
{