NVMeDevice: Clean up lambdas and lifetimes setupMctpDevice() implemented the MctpEndpoint::subscribe() callbacks entirely with in-line lambda functions. General consensus is that long and/or complex lambdas are not desirable[1]. Given that we're only going to add to the complexity to mitigate MCTP endpoint recovery event cycles, unpick the lambdas by refactoring them to call back into private methods of NVMeDevice. Together with untangling the lambda implementations, we can significantly simplify the captures. We make NVMeDevice inherit std::enable_shared_from_this, after which the lambdas only require a weak pointer to NVMeDevice. From there we provide the usual NVMeDevice::create() overloads to require its instantion be wrapped in a std::shared_ptr. [1]: https://github.com/openbmc/docs/blob/master/anti-patterns.md#very-long-lambda-callbacks Change-Id: I53574da34448355eff0aea18cda744dc72e20849 Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/src/NVMeDevice.cpp b/src/NVMeDevice.cpp index 6120b3f..ce2d24d 100644 --- a/src/NVMeDevice.cpp +++ b/src/NVMeDevice.cpp
@@ -1,97 +1,106 @@ #include "NVMeDevice.hpp" -static void - setupMctpDevice(const std::shared_ptr<MctpDevice>& dev, - const std::weak_ptr<NVMeMiIntf>& weakIntf, - const std::weak_ptr<NVMeSubsystem>& weakSubsys, - const std::shared_ptr<boost::asio::steady_timer>& timer) +#include <cassert> + +void NVMeDevice::start() { - dev->setup([weakDev{std::weak_ptr(dev)}, weakIntf, weakSubsys, - timer](const std::error_code& ec, - const std::shared_ptr<MctpEndpoint>& ep) { - if (ec) + if (intf.getProtocol() == NVMeIntf::Protocol::NVMeMI) + { + setup(); + } +} + +void NVMeDevice::setup() +{ + dev->setup( + [weak{weak_from_this()}](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + if (auto self = weak.lock()) { - auto dev = weakDev.lock(); - if (!dev) - { - return; - } - // Setup failed, wait a bit and try again - timer->expires_after(std::chrono::seconds(5)); - timer->async_wait([=](const boost::system::error_code& ec) { - if (!ec) - { - setupMctpDevice(dev, weakIntf, weakSubsys, timer); - } - }); - return; - } - - ep->subscribe( - // Degraded - [weakIntf](const std::shared_ptr<MctpEndpoint>& ep) { - if (auto miIntf = weakIntf.lock()) - { - std::cout << "[" << ep->describe() << "]: Degraded\n"; - miIntf->stop(); - } - }, - // Available - [weakIntf, weakSubsys](const std::shared_ptr<MctpEndpoint>& ep) { - if (auto miIntf = weakIntf.lock()) - { - if (auto subsys = weakSubsys.lock()) - { - std::cout << subsys->getName() << " [" << ep->describe() - << "]: Available\n"; - } - miIntf->start(ep); - } - }, - // Removed - [=](const std::shared_ptr<MctpEndpoint>& ep) { - auto nvmeSubsys = weakSubsys.lock(); - auto miIntf = weakIntf.lock(); - auto dev = weakDev.lock(); - if (!nvmeSubsys || !miIntf || !dev) - { - return; - } - - std::cout << "[" << ep->describe() << "]: Removed\n"; - miIntf->stop(); - // Start polling for the return of the device - timer->expires_after(std::chrono::seconds(5)); - timer->async_wait([=](const boost::system::error_code& ec) { - if (!ec) - { - setupMctpDevice(dev, weakIntf, weakSubsys, timer); - } - }); - }); - - auto miIntf = weakIntf.lock(); - auto nvmeSubsys = weakSubsys.lock(); - if (miIntf && nvmeSubsys) - { - miIntf->start(ep); + self->finalize(ec, ep); } }); -}; - -void NVMeDevice::start(const std::shared_ptr<boost::asio::steady_timer>& timer) -{ - if (intf.getProtocol() != NVMeIntf::Protocol::NVMeMI) - { - return; - } - - setupMctpDevice(dev, - std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface()), - subsys, timer); } void NVMeDevice::stop() { subsys->stop(); } + +void NVMeDevice::finalize(const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) +{ + assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); + + if (ec) + { + restart(); + return; + } + + ep->subscribe( + [weak{weak_from_this()}](const std::shared_ptr<MctpEndpoint>& ep) { + if (auto self = weak.lock()) + { + self->degraded(ep); + } + }, + [weak{weak_from_this()}](const std::shared_ptr<MctpEndpoint>& ep) { + if (auto self = weak.lock()) + { + self->available(ep); + } + }, + // Removed + [weak{weak_from_this()}](const std::shared_ptr<MctpEndpoint>& ep) { + if (auto self = weak.lock()) + { + self->removed(ep); + } + }); + + std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())->start(ep); +} + +void NVMeDevice::restart() +{ + assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); + assert(timer); + // Setup failed, wait a bit and try again + timer->expires_after(std::chrono::seconds(5)); + timer->async_wait( + [weak{weak_from_this()}](const boost::system::error_code& ec) { + if (ec) + { + return; + } + + if (auto self = weak.lock()) + { + self->setup(); + } + }); +} + +void NVMeDevice::degraded(const std::shared_ptr<MctpEndpoint>& ep) +{ + assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); + std::cout << "[" << ep->describe() << "]: Degraded\n"; + std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())->stop(); +} + +void NVMeDevice::available(const std::shared_ptr<MctpEndpoint>& ep) +{ + assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); + std::cout << subsys->getName() << " [" << ep->describe() + << "]: Available\n"; + std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())->start(ep); +} + +void NVMeDevice::removed(const std::shared_ptr<MctpEndpoint>& ep) +{ + assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); + std::cout << "[" << ep->describe() << "]: Removed\n"; + std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())->stop(); + restart(); +}
diff --git a/src/NVMeDevice.hpp b/src/NVMeDevice.hpp index a1a0a24..4efcbc4 100644 --- a/src/NVMeDevice.hpp +++ b/src/NVMeDevice.hpp
@@ -4,19 +4,53 @@ #include "NVMeIntf.hpp" #include "NVMeSubsys.hpp" -class NVMeDevice +#include <memory> + +class NVMeDevice : public std::enable_shared_from_this<NVMeDevice> { + struct Private + {}; + public: - NVMeDevice(const std::shared_ptr<MctpDevice>& dev, NVMeIntf&& intf, + static std::shared_ptr<NVMeDevice> + create(NVMeIntf intf, const std::shared_ptr<NVMeSubsystem>& subsys) + { + return std::make_shared<NVMeDevice>(Private(), std::move(intf), subsys); + } + + static std::shared_ptr<NVMeDevice> + create(boost::asio::io_context& io, + const std::shared_ptr<MctpDevice>& dev, NVMeIntf intf, + const std::shared_ptr<NVMeSubsystem>& subsys) + { + return std::make_shared<NVMeDevice>(Private(), io, dev, std::move(intf), + subsys); + } + + NVMeDevice(Private /*unused*/, NVMeIntf intf, const std::shared_ptr<NVMeSubsystem>& subsys) : - dev(dev), intf(intf), subsys(subsys) + intf(std::move(intf)), subsys(subsys) + {} + NVMeDevice(Private /*unused*/, boost::asio::io_context& io, + const std::shared_ptr<MctpDevice>& dev, NVMeIntf intf, + const std::shared_ptr<NVMeSubsystem>& subsys) : + dev(dev), intf(std::move(intf)), subsys(subsys), timer(io) {} ~NVMeDevice() = default; - void start(const std::shared_ptr<boost::asio::steady_timer>& timer); + void start(); void stop(); private: + void setup(); + void restart(); + void degraded(const std::shared_ptr<MctpEndpoint>& ep); + void available(const std::shared_ptr<MctpEndpoint>& ep); + void removed(const std::shared_ptr<MctpEndpoint>& ep); + void finalize(const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep); + std::shared_ptr<MctpDevice> dev; NVMeIntf intf; std::shared_ptr<NVMeSubsystem> subsys; + std::optional<boost::asio::steady_timer> timer; };
diff --git a/src/NVMeSensorMain.cpp b/src/NVMeSensorMain.cpp index 7dec543..2357573 100644 --- a/src/NVMeSensorMain.cpp +++ b/src/NVMeSensorMain.cpp
@@ -38,7 +38,7 @@ #include <unordered_set> // a map with key value of {path, NVMeSubsystem} -using NVMEMap = std::map<std::string, NVMeDevice>; +using NVMEMap = std::map<std::string, std::shared_ptr<NVMeDevice>>; static NVMEMap nvmeDevices; // A map from root bus number to the Worker @@ -286,8 +286,9 @@ io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); nvmeSubsys->start(); - NVMeDevice dev{{}, std::move(nvmeIntf), nvmeSubsys}; - nvmeDevices.emplace(nvmeObjectPath, std::move(dev)); + auto nvmeDev = NVMeDevice::create(std::move(nvmeIntf), + nvmeSubsys); + nvmeDevices.try_emplace(nvmeObjectPath, nvmeDev); } catch (std::exception& ex) { @@ -355,13 +356,12 @@ io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); - NVMeDevice nvmeDev{mctpDev, std::move(nvmeIntf), nvmeSubsys}; - auto [entry, _] = nvmeDevices.emplace(nvmeObjectPath, - std::move(nvmeDev)); + auto nvmeDev = NVMeDevice::create( + io, mctpDev, std::move(nvmeIntf), nvmeSubsys); + auto [entry, _] = nvmeDevices.try_emplace(nvmeObjectPath, + nvmeDev); nvmeSubsys->start(); - auto timer = std::make_shared<boost::asio::steady_timer>( - io, std::chrono::seconds(5)); - entry->second.start(timer); + entry->second->start(); } catch (std::exception& ex) { @@ -380,7 +380,7 @@ // todo: it'd be better to only update the ones we care about for (auto& [_, nvmeDev] : nvmeDevices) { - nvmeDev.stop(); + nvmeDev->stop(); } nvmeDevices.clear(); @@ -452,7 +452,7 @@ return; } - device->second.stop(); + device->second->stop(); devices.erase(device); }