NVMeSensorMain: Split out NVMeDevice NVMeDevice was defined as a type to hold references to the MI and subsystem instances across the lifetime of the device configuration provided by EM. setupMctpDevice() was a static function associated with these objects by way of taking them as parameters. A small trick here was the conversion of strong to weak pointers by way of the types specified for the formal parameters. The conversion enables the desired lifetime management of the associated DBus objects, tying them to the EM configuration (and not the lifetime of `mctpd` DBus objects). Really though, setupMctpDevice() could be better expressed as a method on NVMeDevice: This way we can handle any smart-pointer type conversions internal to the class' implementation, and applying this perspective increases the testability of the code. A final benefit is that by isolating the code we can refactor it to add features without significantly impacting its readability. Ultimately the goal is to insert a delay via a timer to mitigate stalling of the main thread due to recovery-related event-cycles. Here we purely do the code-motion that moves the implementation of setupMctpEndpoint() out of NVMeSensorMain.cpp, along with defining a method that wraps its invocation. Change-Id: I498c43561dee61462e049b9ad43c987e1754ba0c Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/src/NVMeDevice.cpp b/src/NVMeDevice.cpp new file mode 100644 index 0000000..6120b3f --- /dev/null +++ b/src/NVMeDevice.cpp
@@ -0,0 +1,97 @@ +#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) +{ + dev->setup([weakDev{std::weak_ptr(dev)}, weakIntf, weakSubsys, + timer](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + if (ec) + { + 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); + } + }); +}; + +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(); +}
diff --git a/src/NVMeDevice.hpp b/src/NVMeDevice.hpp new file mode 100644 index 0000000..a1a0a24 --- /dev/null +++ b/src/NVMeDevice.hpp
@@ -0,0 +1,22 @@ +#pragma once + +#include "MctpEndpoint.hpp" +#include "NVMeIntf.hpp" +#include "NVMeSubsys.hpp" + +class NVMeDevice +{ + public: + NVMeDevice(const std::shared_ptr<MctpDevice>& dev, NVMeIntf&& intf, + const std::shared_ptr<NVMeSubsystem>& subsys) : + dev(dev), intf(intf), subsys(subsys) + {} + ~NVMeDevice() = default; + void start(const std::shared_ptr<boost::asio::steady_timer>& timer); + void stop(); + + private: + std::shared_ptr<MctpDevice> dev; + NVMeIntf intf; + std::shared_ptr<NVMeSubsystem> subsys; +};
diff --git a/src/NVMeSensorMain.cpp b/src/NVMeSensorMain.cpp index daa2772..7dec543 100644 --- a/src/NVMeSensorMain.cpp +++ b/src/NVMeSensorMain.cpp
@@ -17,6 +17,7 @@ #include "MctpEndpoint.hpp" #include "MctpReactorDevice.hpp" #include "NVMeBasic.hpp" +#include "NVMeDevice.hpp" #include "NVMeIntf.hpp" #include "NVMeMi.hpp" #include "NVMePlugin.hpp" @@ -36,13 +37,6 @@ #include <system_error> #include <unordered_set> -struct NVMeDevice -{ - std::shared_ptr<MctpDevice> dev; - NVMeIntf intf; - std::shared_ptr<NVMeSubsystem> subsys; -}; - // a map with key value of {path, NVMeSubsystem} using NVMEMap = std::map<std::string, NVMeDevice>; static NVMEMap nvmeDevices; @@ -199,85 +193,6 @@ return false; } -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) -{ - dev->setup([weakDev{std::weak_ptr(dev)}, weakIntf, weakSubsys, - timer](const std::error_code& ec, - const std::shared_ptr<MctpEndpoint>& ep) { - if (ec) - { - 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); - } - }); -} - static void handleConfigurations( boost::asio::io_context& io, sdbusplus::asio::object_server& objectServer, std::shared_ptr<sdbusplus::asio::connection>& dbusConnection, @@ -371,7 +286,7 @@ io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); nvmeSubsys->start(); - NVMeDevice dev{{}, nvmeIntf, nvmeSubsys}; + NVMeDevice dev{{}, std::move(nvmeIntf), nvmeSubsys}; nvmeDevices.emplace(nvmeObjectPath, std::move(dev)); } catch (std::exception& ex) @@ -440,15 +355,13 @@ io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); - NVMeDevice nvmeDev{mctpDev, nvmeIntf, nvmeSubsys}; - nvmeDevices.emplace(nvmeObjectPath, std::move(nvmeDev)); + NVMeDevice nvmeDev{mctpDev, std::move(nvmeIntf), nvmeSubsys}; + auto [entry, _] = nvmeDevices.emplace(nvmeObjectPath, + std::move(nvmeDev)); nvmeSubsys->start(); auto timer = std::make_shared<boost::asio::steady_timer>( io, std::chrono::seconds(5)); - setupMctpDevice(mctpDev, - std::get<std::shared_ptr<NVMeMiIntf>>( - nvmeIntf.getInferface()), - nvmeSubsys, timer); + entry->second.start(timer); } catch (std::exception& ex) { @@ -465,12 +378,9 @@ std::shared_ptr<sdbusplus::asio::connection>& dbusConnection) { // todo: it'd be better to only update the ones we care about - for (const auto& [_, nvmeDev] : nvmeDevices) + for (auto& [_, nvmeDev] : nvmeDevices) { - if (nvmeDev.subsys) - { - nvmeDev.subsys->stop(); - } + nvmeDev.stop(); } nvmeDevices.clear(); @@ -542,7 +452,7 @@ return; } - device->second.subsys->stop(); + device->second.stop(); devices.erase(device); }
diff --git a/src/meson.build b/src/meson.build index 9c0b69d..2363a2c 100644 --- a/src/meson.build +++ b/src/meson.build
@@ -212,6 +212,7 @@ 'NVMeController.cpp', 'NVMeProgress.cpp', 'NVMeVolume.cpp', + 'NVMeDevice.cpp', 'NVMeDrive.cpp', 'NVMeError.cpp', 'NVMeStorage.cpp',