MctpEndpoint: Integrate remove behavior into MctpdDevice
9498075d1375 ("Revert "MctpEndpoint: Introduce StaticEntity container
class"") outlines the need for a different design-point than we
set out on in ac513c858a9e ("MctpEndpoint: Introduce StaticEntity
container class"). The latter commit externalised the handling of
InterfacesRemoved events from the MctpDevice and MctpEndpoint classes
to use the signal as input to resume polling via a timer invoking
MctpDevice::setup(). This felt a little untidy at the time, but
manageable.
The improved understanding of the needs that drove the revert above
triggered me to reconsider this separation. Better encapsulation
of behavior is achieved by integrating the handling of endpoint
InterfacesRemoved signals into MctpDevice. MctpDevice is then
self-contained with respect to managing the lifecycle of its
MctpEndpoint. From here we can integrate handling of MctpDevice into
NVMeSensorMain such that its MctpEndpoint's remove event callback
triggers the required polling.
Change-Id: I7d0d69d7235c19ebfbdef83fe1094719fd82f2fd
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/src/MctpEndpoint.cpp b/src/MctpEndpoint.cpp
index 8de48a4..8c0d300 100644
--- a/src/MctpEndpoint.cpp
+++ b/src/MctpEndpoint.cpp
@@ -4,6 +4,7 @@
#include <boost/system/detail/errc.hpp>
#include <sdbusplus/asio/connection.hpp>
+#include <sdbusplus/bus/match.hpp>
#include <sdbusplus/exception.hpp>
#include <sdbusplus/message.hpp>
#include <sdbusplus/message/native_types.hpp>
@@ -26,32 +27,69 @@
interface(interface), physaddr(physaddr)
{}
+void MctpdDevice::onEndpointInterfacesRemoved(
+ const std::weak_ptr<MctpdDevice>& weak, const std::string& objpath,
+ sdbusplus::message_t& msg)
+{
+ auto path = msg.unpack<sdbusplus::message::object_path>();
+ if (path.str != objpath)
+ {
+ return;
+ }
+
+ auto removedIfaces = msg.unpack<std::set<std::string>>();
+ if (!removedIfaces.contains(mctpdEndpointControlInterface))
+ {
+ return;
+ }
+
+ if (auto self = weak.lock())
+ {
+ self->endpointRemoved();
+ }
+}
+
+void MctpdDevice::finaliseEndpoint(
+ const std::string& objpath, uint8_t eid, int network,
+ std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&& action)
+{
+ using namespace sdbusplus::bus::match;
+ const auto matchSpec = std::string(rules::interfacesRemoved())
+ .append(rules::argNpath(0, objpath));
+ removeMatch = std::make_unique<sdbusplus::bus::match_t>(
+ *connection, matchSpec,
+ std::bind_front(MctpdDevice::onEndpointInterfacesRemoved,
+ weak_from_this(), objpath));
+ endpoint = std::make_shared<MctpdEndpoint>(shared_from_this(), connection,
+ objpath, network, eid);
+ action({}, endpoint);
+}
+
void MctpdDevice::setup(
std::function<void(const std::error_code& ec,
const std::shared_ptr<MctpEndpoint>& ep)>&& action)
{
+ auto onSetup = [weak{weak_from_this()}, action{std::move(action)}](
+ const boost::system::error_code& ec, uint8_t eid,
+ int network, const std::string& objpath,
+ bool allocated [[maybe_unused]]) mutable {
+ if (ec)
+ {
+ action(ec, {});
+ return;
+ }
+
+ if (auto self = weak.lock())
+ {
+ self->finaliseEndpoint(objpath, eid, network, std::move(action));
+ }
+ };
try
{
- connection->async_method_call(
- [weak{weak_from_this()}, action](
- const boost::system::error_code& ec, uint8_t eid, int network,
- const std::string& objpath, bool allocated [[maybe_unused]]) {
- if (ec)
- {
- /* XXX What error does mctpd actually provide? */
- action(ec, {});
- return;
- }
-
- if (auto self = weak.lock())
- {
- self->endpoint = std::make_shared<MctpdEndpoint>(
- self, self->connection, objpath, network, eid);
- action(static_cast<const std::error_code&>(ec), self->endpoint);
- }
- },
- mctpdBusName, mctpdControlPath, mctpdControlInterface,
- "SetupEndpoint", interface, physaddr);
+ connection->async_method_call(onSetup, mctpdBusName, mctpdControlPath,
+ mctpdControlInterface, "SetupEndpoint",
+ interface, physaddr);
}
catch (const sdbusplus::exception::SdBusError& err)
{
@@ -61,12 +99,21 @@
}
}
-void MctpdDevice::removed()
+void MctpdDevice::endpointRemoved()
+{
+ if (endpoint)
+ {
+ removeMatch.reset();
+ endpoint->removed();
+ endpoint.reset();
+ }
+}
+
+void MctpdDevice::remove()
{
if (endpoint)
{
endpoint->remove();
- endpoint.reset();
}
}
@@ -122,16 +169,16 @@
{
if (connectivity == "Degraded")
{
- if (degraded)
+ if (notifyDegraded)
{
- degraded();
+ notifyDegraded(shared_from_this());
}
}
else if (connectivity == "Available")
{
- if (available)
+ if (notifyAvailable)
{
- available();
+ notifyAvailable(shared_from_this());
}
}
else
@@ -151,9 +198,8 @@
return mctp.eid;
}
-void MctpdEndpoint::subscribe(std::function<void()>&& degraded,
- std::function<void()>&& available,
- std::function<void()>&& removed)
+void MctpdEndpoint::subscribe(Event&& degraded, Event&& available,
+ Event&& removed)
{
const auto matchType = std::string("type='signal'");
const auto matchMember = std::string("member='PropertiesChanged'");
@@ -170,9 +216,9 @@
.append(",")
.append(arg0Namespace);
- this->degraded = degraded;
- this->available = available;
- this->removed = removed;
+ this->notifyDegraded = degraded;
+ this->notifyAvailable = available;
+ this->notifyRemoved = removed;
try
{
@@ -205,9 +251,9 @@
}
catch (const sdbusplus::exception::SdBusError& err)
{
- this->degraded = nullptr;
- this->available = nullptr;
- this->removed = nullptr;
+ this->notifyDegraded = nullptr;
+ this->notifyAvailable = nullptr;
+ this->notifyRemoved = nullptr;
std::throw_with_nested(
MctpException("Failed to register connectivity signal match"));
}
@@ -241,9 +287,31 @@
void MctpdEndpoint::remove()
{
- if (removed)
+ try
{
- removed();
+ connection->async_method_call(
+ [self{shared_from_this()}](const boost::system::error_code& ec) {
+ if (ec)
+ {
+ std::cerr << "Failed to remove endpoint [" << self->describe()
+ << "]" << std::endl;
+ return;
+ }
+ },
+ mctpdBusName, objpath.str, mctpdEndpointControlInterface, "Remove");
+ }
+ catch (const sdbusplus::exception::SdBusError& err)
+ {
+ std::throw_with_nested(
+ MctpException("Failed schedule endpoint removal"));
+ }
+}
+
+void MctpdEndpoint::removed()
+{
+ if (notifyRemoved)
+ {
+ notifyRemoved(shared_from_this());
}
}
diff --git a/src/MctpEndpoint.hpp b/src/MctpEndpoint.hpp
index aa9b912..770fcb6 100644
--- a/src/MctpEndpoint.hpp
+++ b/src/MctpEndpoint.hpp
@@ -45,6 +45,7 @@
class MctpEndpoint
{
public:
+ using Event = std::function<void(const std::shared_ptr<MctpEndpoint>& ep)>;
virtual ~MctpEndpoint() = default;
/**
@@ -72,9 +73,8 @@
* @param removed The callback to execute when the MCTP layer indicates the
* endpoint has been removed.
*/
- virtual void subscribe(std::function<void()>&& degraded,
- std::function<void()>&& available,
- std::function<void()>&& removed) = 0;
+ virtual void subscribe(Event&& degraded, Event&& available,
+ Event&& removed) = 0;
/**
* @brief Configures the Maximum Transmission Unit (MTU) for the route to
@@ -93,6 +93,11 @@
std::function<void(const std::error_code& ec)>&& completed) = 0;
/**
+ * @brief Remove the endpoint from its associated network
+ */
+ virtual void remove() = 0;
+
+ /**
* @brief Request that the MCTP layer attempt to recover communication with
* an unresponsive endpoint.
*
@@ -145,12 +150,9 @@
action) = 0;
/**
- * @brief Indicate that the device has been removed.
- *
- * May be called prior to destruction to address any tear-down required
- * without the constraints of a destructor.
+ * @brief Remove the device and any associated endpoint from the MCTP stack.
*/
- virtual void removed() = 0;
+ virtual void remove() = 0;
/**
* @return A formatted string representing the device in terms of its
@@ -185,23 +187,25 @@
int network() const override;
uint8_t eid() const override;
- void subscribe(std::function<void()>&& degraded,
- std::function<void()>&& available,
- std::function<void()>&& removed) override;
+ void subscribe(Event&& degraded, Event&& available,
+ Event&& removed) override;
void setMtu(
uint32_t mtu,
std::function<void(const std::error_code& ec)>&& completed) override;
void recover() override;
+ void remove() override;
std::string describe() const override;
/**
* @brief Indicate the endpoint has been removed
*
- * Called from the implementation of MctpdDevice::removed() for resource
- * cleanup prior to destruction.
+ * Called from the implementation of MctpdDevice for resource cleanup
+ * prior to destruction. Resource cleanup is delegated by invoking the
+ * notifyRemoved() callback. As the actions may be abitrary we avoid
+ * invoking notifyRemoved() in the destructor.
*/
- void remove();
+ void removed();
private:
std::shared_ptr<MctpDevice> device;
@@ -212,9 +216,9 @@
int network;
uint8_t eid;
} mctp;
- std::function<void()> available;
- std::function<void()> degraded;
- std::function<void()> removed;
+ Event notifyAvailable;
+ Event notifyDegraded;
+ Event notifyRemoved;
std::optional<sdbusplus::bus::match_t> connectivityMatch;
void onMctpEndpointChange(sdbusplus::message_t& msg);
@@ -246,14 +250,26 @@
void setup(std::function<void(const std::error_code& ec,
const std::shared_ptr<MctpEndpoint>& ep)>&&
action) override;
- void removed() override;
+ void remove() override;
std::string describe() const override = 0;
private:
+ static void
+ onEndpointInterfacesRemoved(const std::weak_ptr<MctpdDevice>& weak,
+ const std::string& objpath,
+ sdbusplus::message_t& msg);
+
std::shared_ptr<sdbusplus::asio::connection> connection;
const std::string interface;
const std::vector<uint8_t> physaddr;
std::shared_ptr<MctpdEndpoint> endpoint;
+ std::unique_ptr<sdbusplus::bus::match_t> removeMatch;
+
+ void finaliseEndpoint(
+ const std::string& objpath, uint8_t eid, int network,
+ std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&& action);
+ void endpointRemoved();
};
/**
