Refactor MDRV2 class to allow more than one
Refactored the main MDRV2 class to allow more than one object of this
class to exist at the same time. All hardcoded paths have been made
parameters to the constructor, so that they can be varied at runtime as
needed, to avoid overlap.
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
Change-Id: Icd1ebf50086b526cf0cff149eb8ddc59da78f0a9
Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/include/mdrv2.hpp b/include/mdrv2.hpp
index c2cf038..4e431f4 100644
--- a/include/mdrv2.hpp
+++ b/include/mdrv2.hpp
@@ -36,29 +36,46 @@
#include <sdbusplus/timer.hpp>
#include <xyz/openbmc_project/Smbios/MDR_V2/server.hpp>
-sdbusplus::asio::object_server& getObjectServer(void);
+#include <filesystem>
+#include <memory>
-using RecordVariant =
- std::variant<std::string, uint64_t, uint32_t, uint16_t, uint8_t>;
namespace phosphor
{
namespace smbios
{
-static constexpr const char* mdrV2Path = "/xyz/openbmc_project/Smbios/MDR_V2";
-static constexpr const char* smbiosPath = "/xyz/openbmc_project/Smbios";
+using RecordVariant =
+ std::variant<std::string, uint64_t, uint32_t, uint16_t, uint8_t>;
+
+static constexpr const char* defaultObjectPath =
+ "/xyz/openbmc_project/Smbios/MDR_V2";
static constexpr const char* smbiosInterfaceName =
"xyz.openbmc_project.Smbios.GetRecordType";
static constexpr const char* mapperBusName = "xyz.openbmc_project.ObjectMapper";
static constexpr const char* mapperPath = "/xyz/openbmc_project/object_mapper";
static constexpr const char* mapperInterface =
"xyz.openbmc_project.ObjectMapper";
-static constexpr const char* systemInterfacePath =
+static constexpr const char* defaultInventoryPath =
"/xyz/openbmc_project/inventory/system";
static constexpr const char* systemInterface =
"xyz.openbmc_project.Inventory.Item.System";
constexpr const int limitEntryLen = 0xff;
+// Avoid putting multiple interfaces with same name on same object
+static std::string placeGetRecordType(const std::string& objectPath)
+{
+ if (objectPath != defaultObjectPath)
+ {
+ // Place GetRecordType interface on object itself, not parent
+ return objectPath;
+ }
+
+ std::filesystem::path path(objectPath);
+
+ // As there is only one default, safe to place it on the common parent
+ return path.parent_path().string();
+}
+
class MDRV2 :
sdbusplus::server::object_t<
sdbusplus::server::xyz::openbmc_project::smbios::MDRV2>
@@ -69,15 +86,38 @@
MDRV2& operator=(const MDRV2&) = delete;
MDRV2(MDRV2&&) = delete;
MDRV2& operator=(MDRV2&&) = delete;
- ~MDRV2() = default;
- MDRV2(sdbusplus::bus_t& bus, const char* path,
- boost::asio::io_context& io) :
- sdbusplus::server::object_t<
- sdbusplus::server::xyz::openbmc_project::smbios::MDRV2>(bus, path),
- timer(io), bus(bus), smbiosInterface(getObjectServer().add_interface(
- smbiosPath, smbiosInterfaceName))
+ virtual ~MDRV2()
{
+ if (smbiosInterface)
+ {
+ if (objServer)
+ {
+ // Must manually undo add_interface()
+ objServer->remove_interface(smbiosInterface);
+ }
+ }
+ }
+
+ MDRV2(std::shared_ptr<boost::asio::io_context> io,
+ std::shared_ptr<sdbusplus::asio::connection> conn,
+ std::shared_ptr<sdbusplus::asio::object_server> obj,
+ std::string filePath, std::string objectPath,
+ std::string inventoryPath) :
+ sdbusplus::server::object_t<
+ 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)),
+ 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 = smbiosDirVersion;
smbiosDir.dirEntries = 1;
@@ -127,7 +167,8 @@
private:
boost::asio::steady_timer timer;
- sdbusplus::bus_t& bus;
+ std::shared_ptr<sdbusplus::asio::connection> bus;
+ std::shared_ptr<sdbusplus::asio::object_server> objServer;
Mdr2DirStruct smbiosDir;
@@ -151,6 +192,11 @@
std::vector<std::unique_ptr<Pcie>> pcies;
std::unique_ptr<System> system;
std::shared_ptr<sdbusplus::asio::dbus_interface> smbiosInterface;
+
+ std::string smbiosFilePath;
+ std::string smbiosObjectPath;
+ std::string smbiosInventoryPath;
+ std::unique_ptr<sdbusplus::bus::match_t> motherboardConfigMatch;
};
} // namespace smbios
diff --git a/include/smbios_mdrv2.hpp b/include/smbios_mdrv2.hpp
index e77abf4..a730061 100644
--- a/include/smbios_mdrv2.hpp
+++ b/include/smbios_mdrv2.hpp
@@ -19,9 +19,9 @@
#include <phosphor-logging/elog-errors.hpp>
#include <array>
+#include <string>
-static constexpr const char* mdrType2File = "/var/lib/smbios/smbios2";
-static constexpr const char* smbiosPath = "/var/lib/smbios";
+static constexpr const char* mdrDefaultFile = "/var/lib/smbios/smbios2";
static constexpr uint16_t mdrSMBIOSSize = 32 * 1024;
@@ -157,17 +157,13 @@
uint64_t structTableAddr;
} __attribute__((packed));
-static constexpr const char* cpuPath =
- "/xyz/openbmc_project/inventory/system/chassis/motherboard/cpu";
+static constexpr const char* cpuSuffix = "/chassis/motherboard/cpu";
-static constexpr const char* dimmPath =
- "/xyz/openbmc_project/inventory/system/chassis/motherboard/dimm";
+static constexpr const char* dimmSuffix = "/chassis/motherboard/dimm";
-static constexpr const char* pciePath =
- "/xyz/openbmc_project/inventory/system/chassis/motherboard/pcieslot";
+static constexpr const char* pcieSuffix = "/chassis/motherboard/pcieslot";
-static constexpr const char* systemPath =
- "/xyz/openbmc_project/inventory/system/chassis/motherboard/bios";
+static constexpr const char* systemSuffix = "/chassis/motherboard/bios";
constexpr std::array<SMBIOSVersion, 3> supportedSMBIOSVersions{
SMBIOSVersion{3, 2}, SMBIOSVersion{3, 3}, SMBIOSVersion{3, 5}};
diff --git a/include/system.hpp b/include/system.hpp
index 4753a01..ac75a0c 100644
--- a/include/system.hpp
+++ b/include/system.hpp
@@ -17,6 +17,7 @@
#pragma once
#include "smbios_mdrv2.hpp"
+#include <sdbusplus/asio/connection.hpp>
#include <xyz/openbmc_project/Common/UUID/server.hpp>
#include <xyz/openbmc_project/Inventory/Decorator/Revision/server.hpp>
@@ -40,15 +41,17 @@
System(System&&) = default;
System& operator=(System&&) = default;
- System(sdbusplus::bus_t& bus, const std::string& objPath,
- uint8_t* smbiosTableStorage) :
+ System(std::shared_ptr<sdbusplus::asio::connection> bus,
+ std::string objPath, uint8_t* smbiosTableStorage,
+ std::string filePath) :
sdbusplus::server::object_t<
sdbusplus::server::xyz::openbmc_project::common::UUID>(
- bus, objPath.c_str()),
+ *bus, objPath.c_str()),
sdbusplus::server::object_t<sdbusplus::server::xyz::openbmc_project::
inventory::decorator::Revision>(
- bus, objPath.c_str()),
- bus(bus), path(objPath), storage(smbiosTableStorage)
+ *bus, objPath.c_str()),
+ bus(std::move(bus)), path(std::move(objPath)),
+ storage(smbiosTableStorage), smbiosFilePath(std::move(filePath))
{
std::string input = "0";
uuid(input);
@@ -58,7 +61,8 @@
std::string uuid(std::string value) override;
std::string version(std::string value) override;
- sdbusplus::bus_t& bus;
+
+ std::shared_ptr<sdbusplus::asio::connection> bus;
private:
/** @brief Path of the group instance */
@@ -108,6 +112,8 @@
uint8_t skuNum;
uint8_t family;
} __attribute__((packed));
+
+ std::string smbiosFilePath;
};
} // namespace smbios
diff --git a/src/mdrv2.cpp b/src/mdrv2.cpp
index aba2775..99dc302 100644
--- a/src/mdrv2.cpp
+++ b/src/mdrv2.cpp
@@ -35,7 +35,7 @@
{
std::vector<uint8_t> responseDir;
- std::ifstream smbiosFile(mdrType2File, std::ios_base::binary);
+ std::ifstream smbiosFile(smbiosFilePath, std::ios_base::binary);
if (!smbiosFile.good())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
@@ -216,7 +216,7 @@
"Read data from flash error - Invalid data point");
return false;
}
- std::ifstream smbiosFile(mdrType2File, std::ios_base::binary);
+ std::ifstream smbiosFile(smbiosFilePath, std::ios_base::binary);
if (!smbiosFile.good())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
@@ -377,7 +377,7 @@
uint8_t MDRV2::directoryEntries(uint8_t value)
{
- std::ifstream smbiosFile(mdrType2File, std::ios_base::binary);
+ std::ifstream smbiosFile(smbiosFilePath, std::ios_base::binary);
if (!smbiosFile.good())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
@@ -394,53 +394,88 @@
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(mapperAncestorPath);
method.append(0);
method.append(std::vector<std::string>({systemInterface}));
try
{
std::vector<std::string> paths;
- sdbusplus::message_t reply = bus.call(method);
+ sdbusplus::message_t reply = bus->call(method);
reply.read(paths);
- 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
- static std::unique_ptr<sdbusplus::bus::match_t>
+
+ if (!motherboardConfigMatch)
+ {
motherboardConfigMatch =
std::make_unique<sdbusplus::bus::match_t>(
- bus,
+ *bus,
sdbusplus::bus::match::rules::interfacesAdded() +
sdbusplus::bus::match::rules::argNpath(
- 0,
- "/xyz/openbmc_project/inventory/system/board/"),
+ 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();
- }
+ 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
{
- 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 @@
for (unsigned int index = 0; index < *num; index++)
{
- std::string path = cpuPath + std::to_string(index);
+ std::string path = smbiosInventoryPath + cpuSuffix +
+ std::to_string(index);
if (index + 1 > cpus.size())
{
cpus.emplace_back(std::make_unique<phosphor::smbios::Cpu>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ *bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
motherboardPath));
}
else
@@ -494,11 +530,12 @@
for (unsigned int index = 0; index < *num; index++)
{
- std::string path = dimmPath + std::to_string(index);
+ std::string path = smbiosInventoryPath + dimmSuffix +
+ std::to_string(index);
if (index + 1 > dimms.size())
{
dimms.emplace_back(std::make_unique<phosphor::smbios::Dimm>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ *bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
motherboardPath));
}
else
@@ -526,11 +563,12 @@
for (unsigned int index = 0; index < *num; index++)
{
- std::string path = pciePath + std::to_string(index);
+ std::string path = smbiosInventoryPath + pcieSuffix +
+ std::to_string(index);
if (index + 1 > pcies.size())
{
pcies.emplace_back(std::make_unique<phosphor::smbios::Pcie>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ *bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
motherboardPath));
}
else
@@ -541,8 +579,9 @@
}
system.reset();
- system = std::make_unique<System>(
- bus, systemPath, smbiosDir.dir[smbiosDirIndex].dataStorage);
+ system = std::make_unique<System>(bus, smbiosInventoryPath + systemSuffix,
+ smbiosDir.dir[smbiosDirIndex].dataStorage,
+ smbiosFilePath);
}
std::optional<size_t> MDRV2::getTotalCpuSlot()
@@ -768,7 +807,7 @@
timer.expires_after(usec);
timer.async_wait([this](boost::system::error_code ec) {
- if (ec || this == nullptr)
+ if (ec)
{
phosphor::logging::log<phosphor::logging::level::ERR>(
"Timer Error!");
diff --git a/src/mdrv2_main.cpp b/src/mdrv2_main.cpp
index 06a51cb..8dad1d1 100644
--- a/src/mdrv2_main.cpp
+++ b/src/mdrv2_main.cpp
@@ -22,26 +22,24 @@
#include <sdbusplus/asio/connection.hpp>
#include <sdbusplus/asio/object_server.hpp>
-boost::asio::io_context io;
-auto connection = std::make_shared<sdbusplus::asio::connection>(io);
-auto objServer = sdbusplus::asio::object_server(connection);
-
-sdbusplus::asio::object_server& getObjectServer(void)
+int main()
{
- return objServer;
-}
+ auto io = std::make_shared<boost::asio::io_context>();
+ auto connection = std::make_shared<sdbusplus::asio::connection>(*io);
+ auto objServer =
+ std::make_shared<sdbusplus::asio::object_server>(connection);
-int main(void)
-{
- sdbusplus::bus_t& bus = static_cast<sdbusplus::bus_t&>(*connection);
- sdbusplus::server::manager_t objManager(bus,
+ sdbusplus::server::manager_t objManager(*connection,
"/xyz/openbmc_project/inventory");
- bus.request_name("xyz.openbmc_project.Smbios.MDR_V2");
+ connection->request_name("xyz.openbmc_project.Smbios.MDR_V2");
- phosphor::smbios::MDRV2 mdrV2(bus, phosphor::smbios::mdrV2Path, io);
+ auto mdrV2 = std::make_shared<phosphor::smbios::MDRV2>(
+ io, connection, objServer, mdrDefaultFile,
+ phosphor::smbios::defaultObjectPath,
+ phosphor::smbios::defaultInventoryPath);
- io.run();
+ io->run();
return 0;
}
diff --git a/src/smbios-ipmi-blobs/handler.cpp b/src/smbios-ipmi-blobs/handler.cpp
index 3763762..fb8111a 100644
--- a/src/smbios-ipmi-blobs/handler.cpp
+++ b/src/smbios-ipmi-blobs/handler.cpp
@@ -15,6 +15,7 @@
#include <algorithm>
#include <cstdint>
#include <ctime>
+#include <filesystem>
#include <fstream>
#include <memory>
#include <string>
@@ -34,7 +35,7 @@
bool status = false;
sdbusplus::bus_t bus = sdbusplus::bus_t(ipmid_get_sd_bus_connection());
sdbusplus::message_t method =
- bus.new_method_call(mdrV2Service, phosphor::smbios::mdrV2Path,
+ bus.new_method_call(mdrV2Service, phosphor::smbios::defaultObjectPath,
mdrV2Interface, "AgentSynchronizeData");
try
@@ -48,7 +49,8 @@
"Error Sync data with service",
phosphor::logging::entry("ERROR=%s", e.what()),
phosphor::logging::entry("SERVICE=%s", mdrV2Service),
- phosphor::logging::entry("PATH=%s", phosphor::smbios::mdrV2Path));
+ phosphor::logging::entry("PATH=%s",
+ phosphor::smbios::defaultObjectPath));
return false;
}
@@ -191,24 +193,27 @@
/* Clear the commit_error bit. */
blobPtr->state &= ~blobs::StateFlags::commit_error;
+ std::string defaultDir =
+ std::filesystem::path(mdrDefaultFile).parent_path();
+
MDRSMBIOSHeader mdrHdr;
mdrHdr.dirVer = mdrDirVersion;
mdrHdr.mdrType = mdrTypeII;
mdrHdr.timestamp = std::time(nullptr);
mdrHdr.dataSize = blobPtr->buffer.size();
- if (access(smbiosPath, F_OK) == -1)
+ if (access(defaultDir.c_str(), F_OK) == -1)
{
- int flag = mkdir(smbiosPath, S_IRWXU);
+ int flag = mkdir(defaultDir.c_str(), S_IRWXU);
if (flag != 0)
{
phosphor::logging::log<phosphor::logging::level::ERR>(
- "create folder failed for writting smbios file");
+ "create folder failed for writing smbios file");
blobPtr->state |= blobs::StateFlags::commit_error;
return false;
}
}
- std::ofstream smbiosFile(mdrType2File,
+ std::ofstream smbiosFile(mdrDefaultFile,
std::ios_base::binary | std::ios_base::trunc);
if (!smbiosFile.good())
{
diff --git a/src/system.cpp b/src/system.cpp
index c0713a6..a838577 100644
--- a/src/system.cpp
+++ b/src/system.cpp
@@ -133,7 +133,7 @@
if (std::find_if(tempS.begin(), tempS.end(),
[](char ch) { return !isprint(ch); }) != tempS.end())
{
- std::ofstream smbiosFile(mdrType2File, std::ios_base::trunc);
+ std::ofstream smbiosFile(smbiosFilePath, std::ios_base::trunc);
if (!smbiosFile.good())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
@@ -149,7 +149,7 @@
}
result = tempS;
- setProperty(bus, biosActiveObjPath, biosVersionIntf, biosVersionProp,
+ setProperty(*bus, biosActiveObjPath, biosVersionIntf, biosVersionProp,
result);
}
lg2::info("VERSION INFO - BIOS - {VER}", "VER", result);