Check for missing endpoints when adding assocs
An association links 2 D-Bus object paths together, one
which is the path that has the original associations
property, and another endpoint path. It's possible that
that endpoint path doesn't exist on D-Bus when that
associations property is created.
This commit, along with upcoming ones, adds support to not
create the actual association object paths until that
endpoint path shows up on D-Bus. In addition, if that
endpoint path were to get removed from D-Bus in the future,
then the association paths should be removed until that
path is back again.
This particular commit introduces the PendingAssociations map
to track these cases, and adds support in the associationChanged
path to add associations to this map if the endpoint path isn't
on D-Bus instead of just blindly creating the association objects.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
Change-Id: I1f4bf0e02bf7a350d9e3f18c3591737289a51a39
diff --git a/src/associations.cpp b/src/associations.cpp
index 317836c..0f84d1e 100644
--- a/src/associations.cpp
+++ b/src/associations.cpp
@@ -142,6 +142,7 @@
void associationChanged(sdbusplus::asio::object_server& objectServer,
const std::vector<Association>& associations,
const std::string& path, const std::string& owner,
+ const interface_map_type& interfaceMap,
AssociationMaps& assocMaps)
{
AssociationPaths objects;
@@ -158,6 +159,15 @@
std::cerr << "Found invalid association on path " << path << "\n";
continue;
}
+
+ // Can't create this association if the endpoint isn't on D-Bus.
+ if (interfaceMap.find(endpoint) == interfaceMap.end())
+ {
+ addPendingAssociation(endpoint, reverse, path, forward, owner,
+ assocMaps);
+ continue;
+ }
+
if (forward.size())
{
objects[path + "/" + forward].emplace(endpoint);
@@ -205,24 +215,60 @@
checkAssociationEndpointRemoves(path, owner, objects, objectServer,
assocMaps);
- // Update associationOwners with the latest info
- auto a = assocMaps.owners.find(path);
- if (a != assocMaps.owners.end())
+ if (!objects.empty())
{
- auto o = a->second.find(owner);
- if (o != a->second.end())
+ // Update associationOwners with the latest info
+ auto a = assocMaps.owners.find(path);
+ if (a != assocMaps.owners.end())
{
- o->second = std::move(objects);
+ auto o = a->second.find(owner);
+ if (o != a->second.end())
+ {
+ o->second = std::move(objects);
+ }
+ else
+ {
+ a->second.emplace(owner, std::move(objects));
+ }
}
else
{
- a->second.emplace(owner, std::move(objects));
+ boost::container::flat_map<std::string, AssociationPaths> owners;
+ owners.emplace(owner, std::move(objects));
+ assocMaps.owners.emplace(path, owners);
}
}
+}
+
+void addPendingAssociation(const std::string& objectPath,
+ const std::string& type,
+ const std::string& endpointPath,
+ const std::string& endpointType,
+ const std::string& owner, AssociationMaps& assocMaps)
+{
+ Association assoc{type, endpointType, endpointPath};
+
+ auto p = assocMaps.pending.find(objectPath);
+ if (p == assocMaps.pending.end())
+ {
+ ExistingEndpoints ee;
+ ee.emplace_back(owner, std::move(assoc));
+ assocMaps.pending.emplace(objectPath, std::move(ee));
+ }
else
{
- boost::container::flat_map<std::string, AssociationPaths> owners;
- owners.emplace(owner, std::move(objects));
- assocMaps.owners.emplace(path, owners);
+ // Already waiting on this path for another association,
+ // so just add this endpoint and owner.
+ auto& endpoints = p->second;
+ auto e =
+ std::find_if(endpoints.begin(), endpoints.end(),
+ [&assoc, &owner](const auto& endpoint) {
+ return (std::get<ownerPos>(endpoint) == owner) &&
+ (std::get<assocPos>(endpoint) == assoc);
+ });
+ if (e == endpoints.end())
+ {
+ endpoints.emplace_back(owner, std::move(assoc));
+ }
}
}
diff --git a/src/associations.hpp b/src/associations.hpp
index 31e8b6a..64688bf 100644
--- a/src/associations.hpp
+++ b/src/associations.hpp
@@ -73,6 +73,7 @@
* org.openbmc.Associations
* @param[in] owner - The Dbus service having it's associatons
* changed
+ * @param[in] interfaceMap - The full interface map
* @param[in,out] assocMaps - The association maps
*
* @return Void, objectServer and assocMaps updated if needed
@@ -80,4 +81,32 @@
void associationChanged(sdbusplus::asio::object_server& objectServer,
const std::vector<Association>& associations,
const std::string& path, const std::string& owner,
+ const interface_map_type& interfaceMap,
AssociationMaps& assocMaps);
+
+/** @brief Add a pending associations entry
+ *
+ * Used when a client wants to create an association between
+ * 2 D-Bus endpoint paths, but one of the paths doesn't exist.
+ * When the path does show up in D-Bus, if there is a pending
+ * association then the real association objects can be created.
+ *
+ * @param[in] objectPath - The D-Bus object path that should be an
+ * association endpoint but doesn't exist
+ * on D-Bus.
+ * @param[in] type - The association type. Gets used in the final
+ * association path of <objectPath>/<type>.
+ * @param[in] endpointPath - The D-Bus path on the other side
+ * of the association. This path exists.
+ * @param[in] endpointType - The endpoint association type. Gets used
+ * in the final association path of
+ * <endpointPath>/<endpointType>.
+ * @param[in] owner - The service name that owns the association.
+ * @param[in,out] assocMaps - The association maps
+ */
+void addPendingAssociation(const std::string& objectPath,
+ const std::string& type,
+ const std::string& endpointPath,
+ const std::string& endpointType,
+ const std::string& owner,
+ AssociationMaps& assocMaps);
diff --git a/src/main.cpp b/src/main.cpp
index fe1ea4c..49b1344 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -128,12 +128,13 @@
};
void do_associations(sdbusplus::asio::connection* system_bus,
+ interface_map_type& interfaceMap,
sdbusplus::asio::object_server& objectServer,
const std::string& processName, const std::string& path,
const std::string& assocDefIface)
{
system_bus->async_method_call(
- [&objectServer, path, processName](
+ [&objectServer, path, processName, &interfaceMap](
const boost::system::error_code ec,
const sdbusplus::message::variant<std::vector<Association>>&
variantAssociations) {
@@ -145,7 +146,7 @@
sdbusplus::message::variant_ns::get<std::vector<Association>>(
variantAssociations);
associationChanged(objectServer, associations, path, processName,
- associationMaps);
+ interfaceMap, associationMaps);
},
processName, path, "org.freedesktop.DBus.Properties", "Get",
assocDefIface, getAssocDefPropName(assocDefIface));
@@ -200,7 +201,7 @@
if (isAssocDefIface(iface_name))
{
- do_associations(system_bus, objectServer,
+ do_associations(system_bus, interface_map, objectServer,
transaction->process_name, path,
iface_name);
}
@@ -572,7 +573,7 @@
interfacesRemovedHandler);
std::function<void(sdbusplus::message::message & message)>
- associationChangedHandler = [&server, &name_owners](
+ associationChangedHandler = [&server, &name_owners, &interface_map](
sdbusplus::message::message& message) {
std::string objectName;
boost::container::flat_map<
@@ -600,7 +601,7 @@
return;
}
associationChanged(server, associations, message.get_path(),
- well_known, associationMaps);
+ well_known, interface_map, associationMaps);
}
};
sdbusplus::bus::match::match assocChangedMatch(
diff --git a/src/processing.cpp b/src/processing.cpp
index 4d2d3c1..4125389 100644
--- a/src/processing.cpp
+++ b/src/processing.cpp
@@ -117,7 +117,7 @@
sdbusplus::message::variant_ns::get<std::vector<Association>>(
*variantAssociations);
associationChanged(server, associations, objPath.str, wellKnown,
- assocMaps);
+ interfaceMap, assocMaps);
}
}
diff --git a/src/test/associations.cpp b/src/test/associations.cpp
index ce3c9eb..e443043 100644
--- a/src/test/associations.cpp
+++ b/src/test/associations.cpp
@@ -167,6 +167,7 @@
TEST_F(TestAssociations, associationChangedEmptyEndpoint)
{
std::vector<Association> associations = {{"inventory", "error", ""}};
+ interface_map_type interfaceMap;
AssociationMaps assocMaps;
assocMaps.owners = createDefaultOwnerAssociation();
@@ -174,7 +175,7 @@
// Empty endpoint will result in deletion of corresponding assocInterface
associationChanged(*server, associations, DEFAULT_SOURCE_PATH,
- DEFAULT_DBUS_SVC, assocMaps);
+ DEFAULT_DBUS_SVC, interfaceMap, assocMaps);
// Both of these should be 0 since we have an invalid endpoint
auto intfEndpoints =
@@ -182,6 +183,8 @@
EXPECT_EQ(intfEndpoints.size(), 0);
intfEndpoints = std::get<endpointsPos>(assocMaps.ifaces[DEFAULT_REV_PATH]);
EXPECT_EQ(intfEndpoints.size(), 0);
+
+ EXPECT_EQ(assocMaps.pending.size(), 0);
}
// Add a new association with endpoint
@@ -194,8 +197,13 @@
assocMaps.owners = createDefaultOwnerAssociation();
assocMaps.ifaces = createDefaultInterfaceAssociation(server);
+ // Make it look like the assoc endpoints are on D-Bus
+ interface_map_type interfaceMap = {
+ {"/new/source/path", {{DEFAULT_DBUS_SVC, {"a"}}}},
+ {"/xyz/openbmc_project/new/endpoint", {{DEFAULT_DBUS_SVC, {"a"}}}}};
+
associationChanged(*server, associations, "/new/source/path",
- DEFAULT_DBUS_SVC, assocMaps);
+ DEFAULT_DBUS_SVC, interfaceMap, assocMaps);
// Two source paths
EXPECT_EQ(assocMaps.owners.size(), 2);
@@ -203,6 +211,9 @@
// Four interfaces
EXPECT_EQ(assocMaps.ifaces.size(), 4);
+ // Nothing pending
+ EXPECT_EQ(assocMaps.pending.size(), 0);
+
// New endpoint so assocMaps.ifaces should be same size
auto intfEndpoints =
std::get<endpointsPos>(assocMaps.ifaces[DEFAULT_FWD_PATH]);
@@ -222,12 +233,18 @@
// changed association and interface
AssociationMaps assocMaps;
+ // Make it look like the assoc endpoints are on D-Bus
+ interface_map_type interfaceMap = createDefaultInterfaceMap();
+
associationChanged(*server, associations, DEFAULT_SOURCE_PATH,
- DEFAULT_DBUS_SVC, assocMaps);
+ DEFAULT_DBUS_SVC, interfaceMap, assocMaps);
// New associations so ensure it now contains a single entry
EXPECT_EQ(assocMaps.owners.size(), 1);
+ // Nothing pending
+ EXPECT_EQ(assocMaps.pending.size(), 0);
+
// Verify corresponding assoc paths each have one endpoint in assoc
// interfaces and that those endpoints match
auto singleOwner = assocMaps.owners[DEFAULT_SOURCE_PATH];
@@ -248,12 +265,15 @@
{"inventory", "error",
"/xyz/openbmc_project/inventory/system/chassis"}};
+ // Make it look like the assoc endpoints are on D-Bus
+ interface_map_type interfaceMap = createDefaultInterfaceMap();
+
AssociationMaps assocMaps;
assocMaps.owners = createDefaultOwnerAssociation();
assocMaps.ifaces = createDefaultInterfaceAssociation(server);
associationChanged(*server, associations, DEFAULT_SOURCE_PATH, newOwner,
- assocMaps);
+ interfaceMap, assocMaps);
// New endpoint so assocOwners should be same size
EXPECT_EQ(assocMaps.owners.size(), 1);
@@ -267,6 +287,9 @@
auto a = assocMaps.owners.find(DEFAULT_SOURCE_PATH);
auto o = a->second.find(newOwner);
EXPECT_EQ(o->second.size(), 2);
+
+ // Nothing pending
+ EXPECT_EQ(assocMaps.pending.size(), 0);
}
// Add a new association to existing interface path
@@ -275,12 +298,15 @@
std::vector<Association> associations = {
{"abc", "error", "/xyz/openbmc_project/inventory/system/chassis"}};
+ // Make it look like the assoc endpoints are on D-Bus
+ interface_map_type interfaceMap = createDefaultInterfaceMap();
+
AssociationMaps assocMaps;
assocMaps.owners = createDefaultOwnerAssociation();
assocMaps.ifaces = createDefaultInterfaceAssociation(server);
associationChanged(*server, associations, DEFAULT_SOURCE_PATH,
- DEFAULT_DBUS_SVC, assocMaps);
+ DEFAULT_DBUS_SVC, interfaceMap, assocMaps);
// Should have 3 entries in AssociationInterfaces, one is just missing an
// endpoint
@@ -298,4 +324,6 @@
// Added to an existing owner path so still 1
EXPECT_EQ(assocMaps.owners.size(), 1);
+
+ EXPECT_EQ(assocMaps.pending.size(), 0);
}
diff --git a/src/test/interfaces_added.cpp b/src/test/interfaces_added.cpp
index 4f0d2c4..bee978f 100644
--- a/src/test/interfaces_added.cpp
+++ b/src/test/interfaces_added.cpp
@@ -34,7 +34,7 @@
// Verify good path of interfaces added function
TEST_F(TestInterfacesAdded, InterfacesAddedGoodPath)
{
- interface_map_type interfaceMap;
+ auto interfaceMap = createDefaultInterfaceMap();
AssociationMaps assocMaps;
auto intfAdded = createInterfacesAdded(
@@ -44,9 +44,9 @@
DEFAULT_DBUS_SVC, assocMaps, *server);
// Interface map will get the following:
- // /logging/entry/1 /logging/entry /logging/ /
+ // /logging/entry/1 /logging/entry /logging/ / system/chassis
// dump_InterfaceMapType(interfaceMap);
- EXPECT_EQ(interfaceMap.size(), 4);
+ EXPECT_EQ(interfaceMap.size(), 5);
// New association ower created so ensure it now contains a single entry
// dump_AssociationOwnersType(assocOwners);
@@ -55,11 +55,14 @@
// Ensure the 2 association interfaces were created
// dump_AssociationInterfaces(assocInterfaces);
EXPECT_EQ(assocMaps.ifaces.size(), 2);
+
+ // No pending associations
+ EXPECT_EQ(assocMaps.pending.size(), 0);
}
TEST_F(TestInterfacesAdded, OrgOpenBmcInterfacesAddedGoodPath)
{
- interface_map_type interfaceMap;
+ auto interfaceMap = createDefaultInterfaceMap();
AssociationMaps assocMaps;
auto intfAdded = createInterfacesAdded(
@@ -70,9 +73,8 @@
DEFAULT_DBUS_SVC, assocMaps, *server);
// Interface map will get the following:
- // /logging/entry/1 /logging/entry /logging/ /
- // dump_InterfaceMapType(interfaceMap);
- EXPECT_EQ(interfaceMap.size(), 4);
+ // /logging/entry/1 /logging/entry /logging/ / system/chassis
+ EXPECT_EQ(interfaceMap.size(), 5);
// New association ower created so ensure it now contains a single entry
// dump_AssociationOwnersType(assocOwners);
@@ -81,4 +83,7 @@
// Ensure the 2 association interfaces were created
// dump_AssociationInterfaces(assocInterfaces);
EXPECT_EQ(assocMaps.ifaces.size(), 2);
+
+ // No pending associations
+ EXPECT_EQ(assocMaps.pending.size(), 0);
}
diff --git a/src/test/util/association_objects.hpp b/src/test/util/association_objects.hpp
index 45b89d6..632e5ff 100644
--- a/src/test/util/association_objects.hpp
+++ b/src/test/util/association_objects.hpp
@@ -58,3 +58,14 @@
interface_map_type interfaceMap = {{path, connectionMap}};
return interfaceMap;
}
+
+// Create a default interface_map_type with 2 entries with the same
+// owner.
+interface_map_type createDefaultInterfaceMap()
+{
+ interface_map_type interfaceMap = {
+ {DEFAULT_SOURCE_PATH, {{DEFAULT_DBUS_SVC, {"a"}}}},
+ {DEFAULT_ENDPOINT, {{DEFAULT_DBUS_SVC, {"b"}}}}};
+
+ return interfaceMap;
+}
diff --git a/src/types.hpp b/src/types.hpp
index 0696f47..df7b7fa 100644
--- a/src/types.hpp
+++ b/src/types.hpp
@@ -64,10 +64,26 @@
using Association = std::tuple<std::string, std::string, std::string>;
/**
+ * PendingAssociations tracks associations that cannot be created because
+ * the endpoint (the last element of the Association tuple) doesn't exist.
+ * When that endpoint shows up on D-Bus, both association paths can then
+ * be created. Also, if a valid association has an endpoint removed from
+ * D-Bus, then a new PendingAssociations entry will be created until it
+ * reappears. It has all of the information it needs to recreate the
+ * association.
+ */
+constexpr auto ownerPos = 0;
+constexpr auto assocPos = 1;
+using ExistingEndpoint = std::tuple<std::string, Association>;
+using ExistingEndpoints = std::vector<ExistingEndpoint>;
+using PendingAssociations = std::map<std::string, ExistingEndpoints>;
+
+/**
* Keeps all association related maps together.
*/
struct AssociationMaps
{
AssociationInterfaces ifaces;
AssociationOwnersType owners;
+ PendingAssociations pending;
};