smbios-mdr: Fix Inventory Asociations lookup bug
Testing revealed the Associations were not being looked up correctly.
In IPMI, the xyz.openbmc_project.Inventory.Item.System interface is
attached to an object an extra 2 path components deeper, "board/*"
instead of the toplevel "inventory/system" object which is later
populated with all the stuff from SMBIOS. However, this is not
necessary for objects received via gRPC. These simply attach the System
interface under their same object path, making the lookup easier in
this situation.
Tested: The Associations now correctly appear, both for IPMI, and for
gRPC. Tested both by killing the app so that it restarts, and by
rebooting the entire BMC. This exhibits different behavior, due to
order of initialization, so both code paths are tested: immediate find,
and having to set up a D-Bus matcher to find it later.
https://paste.googleplex.com/4970010400260096
Patch Tracking Bug: b/300011331
Upstream info / review: https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/66559
Upstream-Status: Submitted
Justification: Pending upstream review
In-Review: https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/66559
Change-Id: Ic56654b60376fa85ce5fa3667b1c37902d35f355
Google-Bug-Id: 301138611
Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/recipes-phosphor/smbios/smbios-mdr/0009-Refactored-MDRV2-class-to-allow-more-than-one.patch b/recipes-phosphor/smbios/smbios-mdr/0009-Refactored-MDRV2-class-to-allow-more-than-one.patch
index 80265a9..586f208 100644
--- a/recipes-phosphor/smbios/smbios-mdr/0009-Refactored-MDRV2-class-to-allow-more-than-one.patch
+++ b/recipes-phosphor/smbios/smbios-mdr/0009-Refactored-MDRV2-class-to-allow-more-than-one.patch
@@ -10,6 +10,13 @@
Also did some necessary internal cleanups to facilitate this.
+Tested: Created multiple copies of the MDRV2 object at the same time,
+it worked, all appeared on D-Bus, under distinct object paths and
+inventory paths. Destructed the MDRV2 object, it disappeared from
+D-Bus, constructed it again, it came back.
+
+https://gist.github.com/Krellan/6930bc2ed1ac16b93afcc3a12c02e545
+
Patch Tracking Bug: b/300011331
Upstream info / review: https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/66559
Upstream-Status: Submitted
@@ -22,17 +29,17 @@
%% original patch: 0009-Refactored-MDRV2-class-to-allow-more-than-one.patch
---
- include/mdrv2.hpp | 68 +++++++++++++++++++++++++------
- include/smbios_mdrv2.hpp | 16 +++-----
- include/system.hpp | 18 +++++---
- src/mdrv2.cpp | 60 +++++++++++++++------------
- src/mdrv2_main.cpp | 24 +++++------
- src/smbios-ipmi-blobs/handler.cpp | 17 +++++---
- src/system.cpp | 4 +-
- 7 files changed, 130 insertions(+), 77 deletions(-)
+ include/mdrv2.hpp | 72 ++++++++++++++++----
+ include/smbios_mdrv2.hpp | 16 ++---
+ include/system.hpp | 18 +++--
+ src/mdrv2.cpp | 105 ++++++++++++++++++++----------
+ src/mdrv2_main.cpp | 24 ++++---
+ src/smbios-ipmi-blobs/handler.cpp | 17 +++--
+ src/system.cpp | 4 +-
+ 7 files changed, 173 insertions(+), 83 deletions(-)
diff --git a/include/mdrv2.hpp b/include/mdrv2.hpp
-index 65890ed..4c1b749 100644
+index 65890ed..4ad3fb4 100644
--- a/include/mdrv2.hpp
+++ b/include/mdrv2.hpp
@@ -36,29 +36,46 @@
@@ -71,7 +78,7 @@
constexpr const int limitEntryLen = 0xff;
+// Avoid putting multiple interfaces with same name on same object
-+static std::string placeGetRecordType(const std::string& objectPath)
++static std::string getInterfaceFromObjectPath(const std::string& objectPath)
+{
+ if (objectPath != defaultObjectPath)
+ {
@@ -88,7 +95,7 @@
class MDRV2 :
sdbusplus::server::object_t<
sdbusplus::server::xyz::openbmc_project::smbios::MDRV2>
-@@ -69,14 +86,33 @@ class MDRV2 :
+@@ -69,15 +86,38 @@ class MDRV2 :
MDRV2& operator=(const MDRV2&) = delete;
MDRV2(MDRV2&&) = delete;
MDRV2& operator=(MDRV2&&) = delete;
@@ -120,15 +127,20 @@
+ sdbusplus::server::xyz::openbmc_project::smbios::MDRV2>(
+ *conn, objectPath.c_str()),
+ timer(*io), bus(conn), objServer(std::move(obj)),
-+ smbiosInterface(objServer->add_interface(placeGetRecordType(objectPath),
-+ smbiosInterfaceName)),
++ smbiosInterface(objServer->add_interface(
++ getInterfaceFromObjectPath(objectPath), smbiosInterfaceName)),
+ smbiosFilePath(std::move(filePath)),
+ smbiosObjectPath(std::move(objectPath)),
+ smbiosInventoryPath(std::move(inventoryPath))
{
++ lg2::info("SMBIOS data file path: {F}", "F", smbiosFilePath);
++ lg2::info("SMBIOS control object: {O}", "O", smbiosObjectPath);
++ lg2::info("SMBIOS inventory path: {I}", "I", smbiosInventoryPath);
++
smbiosDir.agentVersion = smbiosAgentVersion;
smbiosDir.dirVersion = 1;
-@@ -127,7 +163,8 @@ class MDRV2 :
+ smbiosDir.dirEntries = 1;
+@@ -127,7 +167,8 @@ class MDRV2 :
private:
boost::asio::steady_timer timer;
@@ -138,7 +150,7 @@
Mdr2DirStruct smbiosDir;
-@@ -151,6 +188,11 @@ class MDRV2 :
+@@ -151,6 +192,11 @@ class MDRV2 :
std::vector<std::unique_ptr<Pcie>> pcies;
std::unique_ptr<System> system;
std::shared_ptr<sdbusplus::asio::dbus_interface> smbiosInterface;
@@ -243,7 +255,7 @@
} // namespace smbios
diff --git a/src/mdrv2.cpp b/src/mdrv2.cpp
-index aba2775..d0d8d62 100644
+index aba2775..99dc302 100644
--- a/src/mdrv2.cpp
+++ b/src/mdrv2.cpp
@@ -35,7 +35,7 @@ std::vector<uint8_t> MDRV2::getDirectoryInformation(uint8_t dirIndex)
@@ -273,16 +285,33 @@
if (!smbiosFile.good())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
-@@ -395,32 +395,31 @@ uint8_t MDRV2::directoryEntries(uint8_t value)
+@@ -394,53 +394,88 @@ uint8_t MDRV2::directoryEntries(uint8_t value)
+
void MDRV2::systemInfoUpdate()
{
++ // By default, look for System interface on any system/board/* object
++ std::string mapperAncestorPath = smbiosInventoryPath;
++ std::string matchParentPath = smbiosInventoryPath + "/board/";
++ bool requireExactMatch = false;
++
++ // If customized, look for System on only that custom object
++ if (smbiosInventoryPath != defaultInventoryPath)
++ {
++ std::filesystem::path path(smbiosInventoryPath);
++
++ // Search under parent to find exact match for self
++ mapperAncestorPath = path.parent_path().string();
++ matchParentPath = mapperAncestorPath;
++ requireExactMatch = true;
++ }
++
std::string motherboardPath;
- auto method = bus.new_method_call(mapperBusName, mapperPath,
- mapperInterface, "GetSubTreePaths");
- method.append(systemInterfacePath);
+ auto method = bus->new_method_call(mapperBusName, mapperPath,
+ mapperInterface, "GetSubTreePaths");
-+ method.append(smbiosInventoryPath);
++ method.append(mapperAncestorPath);
method.append(0);
method.append(std::vector<std::string>({systemInterface}));
@@ -292,48 +321,81 @@
- sdbusplus::message_t reply = bus.call(method);
+ sdbusplus::message_t reply = bus->call(method);
reply.read(paths);
- if (paths.size() < 1)
+- if (paths.size() < 1)
++
++ size_t pathsCount = paths.size();
++ for (size_t i = 0; i < pathsCount; ++i)
++ {
++ if (requireExactMatch && (paths[i] != smbiosInventoryPath))
++ {
++ continue;
++ }
++
++ motherboardPath = std::move(paths[i]);
++ break;
++ }
++
++ if (motherboardPath.empty())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
"Failed to get system motherboard dbus path. Setting up a "
"match rule");
-+
- // Add match rule if motherboard dbus path is not yet created
+- // Add match rule if motherboard dbus path is not yet created
- static std::unique_ptr<sdbusplus::bus::match_t>
-- motherboardConfigMatch =
-- std::make_unique<sdbusplus::bus::match_t>(
++
++ if (!motherboardConfigMatch)
++ {
+ motherboardConfigMatch =
+ std::make_unique<sdbusplus::bus::match_t>(
- bus,
-- sdbusplus::bus::match::rules::interfacesAdded() +
-- sdbusplus::bus::match::rules::argNpath(
++ *bus,
+ sdbusplus::bus::match::rules::interfacesAdded() +
+ sdbusplus::bus::match::rules::argNpath(
- 0,
- "/xyz/openbmc_project/inventory/system/board/"),
-- [this](sdbusplus::message_t& msg) {
-+ motherboardConfigMatch.reset();
-+ motherboardConfigMatch = std::make_unique<sdbusplus::bus::match_t>(
-+ *bus,
-+ sdbusplus::bus::match::rules::interfacesAdded() +
-+ sdbusplus::bus::match::rules::argNpath(
-+ 0, smbiosInventoryPath + "/board/"),
-+ [this](sdbusplus::message_t& msg) {
- sdbusplus::message::object_path objectName;
- boost::container::flat_map<
- std::string,
-@@ -430,9 +429,12 @@ void MDRV2::systemInfoUpdate()
- msg.read(objectName, msgData);
- if (msgData.contains(systemInterface))
- {
++ 0, matchParentPath),
+ [this](sdbusplus::message_t& msg) {
+- sdbusplus::message::object_path objectName;
+- boost::container::flat_map<
+- std::string,
++ sdbusplus::message::object_path objectName;
+ boost::container::flat_map<
+- std::string, std::variant<std::string, uint64_t>>>
+- msgData;
+- msg.read(objectName, msgData);
+- if (msgData.contains(systemInterface))
+- {
- this->systemInfoUpdate();
-+ systemInfoUpdate();
-+ return;
- }
-- });
-+ phosphor::logging::log<phosphor::logging::level::INFO>(
-+ "Motherboard message has no system interface!");
-+ });
+- }
++ std::string,
++ boost::container::flat_map<
++ std::string, std::variant<std::string, uint64_t>>>
++ msgData;
++ msg.read(objectName, msgData);
++ if (msgData.contains(systemInterface))
++ {
++ systemInfoUpdate();
++ }
+ });
++ }
}
else
{
-@@ -462,11 +464,12 @@ void MDRV2::systemInfoUpdate()
+- motherboardPath = std::move(paths[0]);
++ lg2::info(
++ "Found Inventory anchor object for SMBIOS content {I}: {M}",
++ "I", smbiosInventoryPath, "M", motherboardPath);
+ }
+ }
+ catch (const sdbusplus::exception_t& e)
+ {
++ lg2::error(
++ "Exception while trying to find Inventory anchor object for SMBIOS content {I}: {E}",
++ "I", smbiosInventoryPath, "E", e.what());
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ "Failed to query system motherboard",
+ phosphor::logging::entry("ERROR=%s", e.what()));
+@@ -462,11 +497,12 @@ void MDRV2::systemInfoUpdate()
for (unsigned int index = 0; index < *num; index++)
{
@@ -348,7 +410,7 @@
motherboardPath));
}
else
-@@ -494,11 +497,12 @@ void MDRV2::systemInfoUpdate()
+@@ -494,11 +530,12 @@ void MDRV2::systemInfoUpdate()
for (unsigned int index = 0; index < *num; index++)
{
@@ -363,7 +425,7 @@
motherboardPath));
}
else
-@@ -526,11 +530,12 @@ void MDRV2::systemInfoUpdate()
+@@ -526,11 +563,12 @@ void MDRV2::systemInfoUpdate()
for (unsigned int index = 0; index < *num; index++)
{
@@ -378,7 +440,7 @@
motherboardPath));
}
else
-@@ -541,8 +546,9 @@ void MDRV2::systemInfoUpdate()
+@@ -541,8 +579,9 @@ void MDRV2::systemInfoUpdate()
}
system.reset();
@@ -390,7 +452,7 @@
}
std::optional<size_t> MDRV2::getTotalCpuSlot()
-@@ -768,7 +774,7 @@ std::vector<uint32_t> MDRV2::synchronizeDirectoryCommonData(uint8_t idIndex,
+@@ -768,7 +807,7 @@ std::vector<uint32_t> MDRV2::synchronizeDirectoryCommonData(uint8_t idIndex,
timer.expires_after(usec);
timer.async_wait([this](boost::system::error_code ec) {
@@ -524,5 +586,5 @@
}
lg2::info("VERSION INFO - BIOS - {VER}", "VER", result);
--
-2.42.0.459.ge4e396fd5e-goog
+2.42.0.515.g380fc7ccd1-goog
