pldmd: move to libpldm instance ID alloc/free
Refactor the dbus_api::Requester class to be implemented in terms of
libpldm's instance ID database. To make that easier to deal with we
introduce a light-weight RAII C++ binding along with a helper class for
unit tests.
Change-Id: Ia03de8245dfb114e6266ba36dcf26ca4398a4ce0
Signed-off-by: Rashmica Gupta <rashmica@linux.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
diff --git a/fw-update/test/inventory_manager_test.cpp b/fw-update/test/inventory_manager_test.cpp
index dbf5722..1f7d142 100644
--- a/fw-update/test/inventory_manager_test.cpp
+++ b/fw-update/test/inventory_manager_test.cpp
@@ -1,6 +1,7 @@
#include "common/utils.hpp"
#include "fw-update/inventory_manager.hpp"
#include "requester/test/mock_request.hpp"
+#include "test/test_instance_id.hpp"
#include <libpldm/firmware_update.h>
@@ -14,9 +15,9 @@
{
protected:
InventoryManagerTest() :
- event(sdeventplus::Event::get_default()),
+ event(sdeventplus::Event::get_default()), instanceIdDb(),
dbusImplRequester(pldm::utils::DBusHandler::getBus(),
- "/xyz/openbmc_project/pldm"),
+ "/xyz/openbmc_project/pldm", instanceIdDb),
reqHandler(fd, event, dbusImplRequester, false, 90000, seconds(1), 2,
milliseconds(100)),
inventoryManager(reqHandler, dbusImplRequester, outDescriptorMap,
@@ -25,6 +26,7 @@
int fd = -1;
sdeventplus::Event event;
+ TestInstanceIdDb instanceIdDb;
pldm::dbus_api::Requester dbusImplRequester;
requester::Handler<requester::Request> reqHandler;
InventoryManager inventoryManager;
diff --git a/fw-update/test/meson.build b/fw-update/test/meson.build
index 9b3cfd6..ee1bdb1 100644
--- a/fw-update/test/meson.build
+++ b/fw-update/test/meson.build
@@ -5,8 +5,7 @@
'../device_updater.cpp',
'../update_manager.cpp',
'../../common/utils.cpp',
- '../../pldmd/dbus_impl_requester.cpp',
- '../../pldmd/instance_id.cpp'])
+ ])
tests = [
'inventory_manager_test',
@@ -17,6 +16,7 @@
foreach t : tests
test(t, executable(t.underscorify(), t + '.cpp',
implicit_include_directories: false,
+ include_directories: '../../pldmd',
link_args: dynamic_linker,
build_rpath: get_option('oe-sdk').enabled() ? rpath : '',
dependencies: [
diff --git a/host-bmc/test/meson.build b/host-bmc/test/meson.build
index e0973ec..7ccfef7 100644
--- a/host-bmc/test/meson.build
+++ b/host-bmc/test/meson.build
@@ -1,9 +1,6 @@
host_bmc_test_src = declare_dependency(
- sources: [
- '../dbus_to_host_effecters.cpp',
- '../../pldmd/dbus_impl_requester.cpp',
- '../../pldmd/instance_id.cpp'],
- include_directories: '../../requester')
+ sources: [ '../dbus_to_host_effecters.cpp' ],
+ include_directories: '../../requester')
tests = [
'dbus_to_host_effecter_test',
diff --git a/libpldmresponder/test/libpldmresponder_base_test.cpp b/libpldmresponder/test/libpldmresponder_base_test.cpp
index 60edae3..f1ecfc3 100644
--- a/libpldmresponder/test/libpldmresponder_base_test.cpp
+++ b/libpldmresponder/test/libpldmresponder_base_test.cpp
@@ -1,5 +1,7 @@
#include "common/utils.hpp"
#include "libpldmresponder/base.hpp"
+#include "pldmd/instance_id.hpp"
+#include "test/test_instance_id.hpp"
#include <libpldm/base.h>
#include <string.h>
@@ -16,11 +18,13 @@
{
protected:
TestBaseCommands() :
- requester(pldm::utils::DBusHandler::getBus(), "/abc/def"),
+ instanceIdDb(), requester(pldm::utils::DBusHandler::getBus(),
+ "/abc/def", this->instanceIdDb),
event(sdeventplus::Event::get_default())
{}
uint8_t mctpEid = 0;
+ TestInstanceIdDb instanceIdDb;
Requester requester;
sdeventplus::Event event;
};
diff --git a/libpldmresponder/test/meson.build b/libpldmresponder/test/meson.build
index 740e307..73938b4 100644
--- a/libpldmresponder/test/meson.build
+++ b/libpldmresponder/test/meson.build
@@ -21,19 +21,13 @@
]
endif
- dep_src_files = [
- '../../pldmd/instance_id.cpp',
- '../../pldmd/dbus_impl_requester.cpp'
- ]
-dep_src = declare_dependency(sources: dep_src_files,include_directories: '../../requester')
-
foreach t : tests
test(t, executable(t.underscorify(), t + '.cpp',
implicit_include_directories: false,
+ include_directories: [ '../../requester', '../../pldmd' ],
link_args: dynamic_linker,
build_rpath: get_option('oe-sdk').enabled() ? rpath : '',
dependencies: [
- dep_src,
libpldm_dep,
libpldmresponder_dep,
libpldmutils,
diff --git a/meson.build b/meson.build
index 251aa8b..7a596ed 100644
--- a/meson.build
+++ b/meson.build
@@ -175,8 +175,6 @@
executable(
'pldmd',
'pldmd/pldmd.cpp',
- 'pldmd/dbus_impl_requester.cpp',
- 'pldmd/instance_id.cpp',
'pldmd/dbus_impl_pdr.cpp',
'fw-update/inventory_manager.cpp',
'fw-update/package_parser.cpp',
diff --git a/oem/ibm/test/libpldmresponder_oem_platform_test.cpp b/oem/ibm/test/libpldmresponder_oem_platform_test.cpp
index d0881e4..ca9dd66 100644
--- a/oem/ibm/test/libpldmresponder_oem_platform_test.cpp
+++ b/oem/ibm/test/libpldmresponder_oem_platform_test.cpp
@@ -6,6 +6,7 @@
#include "libpldmresponder/platform.hpp"
#include "oem/ibm/libpldmresponder/inband_code_update.hpp"
#include "oem/ibm/libpldmresponder/oem_ibm_handler.hpp"
+#include "test/test_instance_id.hpp"
#include <libpldm/entity.h>
#include <libpldm/entity_oem_ibm.h>
@@ -52,8 +53,10 @@
uint16_t entityInstance_ = 0;
uint8_t compSensorCnt_ = 1;
uint16_t effecterId = 0xA;
+ TestInstanceIdDb instanceIdDb;
+
sdbusplus::bus_t bus(sdbusplus::bus::new_default());
- Requester requester(bus, "/abc/def");
+ Requester requester(bus, "/abc/def", instanceIdDb);
auto event = sdeventplus::Event::get_default();
std::vector<get_sensor_state_field> stateField;
@@ -186,7 +189,9 @@
{
auto inPDRRepo = pldm_pdr_init();
sdbusplus::bus_t bus(sdbusplus::bus::new_default());
- Requester requester(bus, "/abc/def");
+ std::filesystem::path dbPath;
+ TestInstanceIdDb instanceIdDb;
+ Requester requester(bus, "/abc/def", instanceIdDb);
auto mockDbusHandler = std::make_unique<MockdBusHandler>();
auto event = sdeventplus::Event::get_default();
std::unique_ptr<CodeUpdate> mockCodeUpdate =
@@ -291,7 +296,9 @@
{
auto inPDRRepo = pldm_pdr_init();
sdbusplus::bus_t bus(sdbusplus::bus::new_default());
- Requester requester(bus, "/abc/def");
+ std::filesystem::path dbPath;
+ TestInstanceIdDb instanceIdDb;
+ Requester requester(bus, "/abc/def", instanceIdDb);
auto mockDbusHandler = std::make_unique<MockdBusHandler>();
auto event = sdeventplus::Event::get_default();
diff --git a/pldmd/dbus_impl_requester.cpp b/pldmd/dbus_impl_requester.cpp
deleted file mode 100644
index 38ddb44..0000000
--- a/pldmd/dbus_impl_requester.cpp
+++ /dev/null
@@ -1,36 +0,0 @@
-#include "dbus_impl_requester.hpp"
-
-#include "xyz/openbmc_project/Common/error.hpp"
-
-#include <iostream>
-
-using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-
-namespace pldm
-{
-namespace dbus_api
-{
-
-uint8_t Requester::getInstanceId(uint8_t eid)
-{
- if (ids.find(eid) == ids.end())
- {
- InstanceId id;
- ids.emplace(eid, InstanceId());
- }
-
- uint8_t id{};
- try
- {
- id = ids[eid].next();
- }
- catch (const std::runtime_error& e)
- {
- throw TooManyResources();
- }
-
- return id;
-}
-
-} // namespace dbus_api
-} // namespace pldm
diff --git a/pldmd/dbus_impl_requester.hpp b/pldmd/dbus_impl_requester.hpp
index a1dfcf5..2daaf18 100644
--- a/pldmd/dbus_impl_requester.hpp
+++ b/pldmd/dbus_impl_requester.hpp
@@ -1,6 +1,7 @@
#pragma once
#include "instance_id.hpp"
+#include "xyz/openbmc_project/Common/error.hpp"
#include "xyz/openbmc_project/PLDM/Requester/server.hpp"
#include <sdbusplus/bus.hpp>
@@ -34,26 +35,53 @@
/** @brief Constructor to put object onto bus at a dbus path.
* @param[in] bus - Bus to attach to.
* @param[in] path - Path to attach at.
+ * @param[in] db - The database to use for allocating instance IDs
+ * @note will throw TooManyResources() if there were no free instance IDs
+ * Throws std::system_category().default_error_condition if there is
+ * something wrong with the instance ID database.
*/
- Requester(sdbusplus::bus_t& bus, const std::string& path) :
- RequesterIntf(bus, path.c_str()){};
+ Requester(sdbusplus::bus_t& bus, const std::string& path,
+ InstanceIdDb& db) :
+ RequesterIntf(bus, path.c_str()),
+ pldmInstanceIdDb(db){};
/** @brief Implementation for RequesterIntf.GetInstanceId */
- uint8_t getInstanceId(uint8_t eid) override;
+ uint8_t getInstanceId(uint8_t eid) override
+ {
+ int id;
+
+ // Ideally we would be able to look up the TID for a given EID. We don't
+ // have that infrastructure in place yet. So use the EID value for the
+ // TID. This is an interim step towards the PLDM requester logic moving
+ // into libpldm, and eventually this won't be needed.
+ try
+ {
+ id = pldmInstanceIdDb.next(eid);
+ }
+ catch (const std::runtime_error& e)
+ {
+ throw sdbusplus::xyz::openbmc_project::Common::Error::
+ TooManyResources();
+ }
+
+ return id;
+ }
/** @brief Mark an instance id as unused
* @param[in] eid - MCTP eid to which this instance id belongs
* @param[in] instanceId - PLDM instance id to be freed
- * @note will throw std::out_of_range if instanceId > 31
+ * @note will throw std::runtime_error if the instance ID was not
+ * previously allocated.
+ * Throws std::system_category().default_error_condition if there is
+ * something wrong with the instance ID database.
*/
void markFree(uint8_t eid, uint8_t instanceId)
{
- ids[eid].markFree(instanceId);
+ pldmInstanceIdDb.free(eid, instanceId);
}
private:
- /** @brief EID to PLDM Instance ID map */
- std::map<uint8_t, InstanceId> ids;
+ InstanceIdDb& pldmInstanceIdDb;
};
} // namespace dbus_api
diff --git a/pldmd/instance_id.cpp b/pldmd/instance_id.cpp
deleted file mode 100644
index 52cd1d6..0000000
--- a/pldmd/instance_id.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-#include "instance_id.hpp"
-
-#include <stdexcept>
-
-namespace pldm
-{
-
-uint8_t InstanceId::next()
-{
- uint8_t idx = 0;
- while (idx < id.size() && id.test(idx))
- {
- ++idx;
- }
-
- if (idx == id.size())
- {
- throw std::runtime_error("No free instance ids");
- }
-
- id.set(idx);
- return idx;
-}
-
-} // namespace pldm
diff --git a/pldmd/instance_id.hpp b/pldmd/instance_id.hpp
index 4f88c43..1f003fa 100644
--- a/pldmd/instance_id.hpp
+++ b/pldmd/instance_id.hpp
@@ -1,34 +1,99 @@
#pragma once
-#include <bitset>
+#include "libpldm/instance-id.h"
+
+#include <phosphor-logging/lg2.hpp>
+
+#include <cerrno>
#include <cstdint>
+#include <exception>
+#include <string>
+#include <system_error>
+
+PHOSPHOR_LOG2_USING;
namespace pldm
{
-constexpr size_t maxInstanceIds = 32;
-
/** @class InstanceId
* @brief Implementation of PLDM instance id as per DSP0240 v1.0.0
*/
-class InstanceId
+class InstanceIdDb
{
public:
- /** @brief Get next unused instance id
- * @return - PLDM instance id
+ InstanceIdDb()
+ {
+ int rc = pldm_instance_db_init_default(&pldmInstanceIdDb);
+ if (rc)
+ {
+ throw std::system_category().default_error_condition(rc);
+ }
+ }
+
+ /** @brief Constructor
+ *
+ * @param[in] path - instance ID database path
*/
- uint8_t next();
+ InstanceIdDb(const std::string& path)
+ {
+ int rc = pldm_instance_db_init(&pldmInstanceIdDb, path.c_str());
+ if (rc)
+ {
+ throw std::system_category().default_error_condition(rc);
+ }
+ }
+
+ ~InstanceIdDb()
+ {
+ int rc = pldm_instance_db_destroy(pldmInstanceIdDb);
+ if (rc)
+ {
+ error("pldm_instance_db_destroy failed, rc= {RC}", "RC", rc);
+ }
+ }
+
+ /** @brief Allocate an instance ID for the given terminus
+ * @param[in] tid - the terminus ID the instance ID is associated with
+ * @return - PLDM instance id or -EAGAIN if there are no available instance
+ * IDs
+ */
+ uint8_t next(uint8_t tid)
+ {
+ uint8_t id;
+ int rc = pldm_instance_id_alloc(pldmInstanceIdDb, tid, &id);
+
+ if (rc == -EAGAIN)
+ {
+ throw std::runtime_error("No free instance ids");
+ }
+
+ if (rc)
+ {
+ throw std::system_category().default_error_condition(rc);
+ }
+
+ return id;
+ }
/** @brief Mark an instance id as unused
+ * @param[in] tid - the terminus ID the instance ID is associated with
* @param[in] instanceId - PLDM instance id to be freed
*/
- void markFree(uint8_t instanceId)
+ void free(uint8_t tid, uint8_t instanceId)
{
- id.set(instanceId, false);
+ int rc = pldm_instance_id_free(pldmInstanceIdDb, tid, instanceId);
+ if (rc == -EINVAL)
+ {
+ throw std::runtime_error("Invalid instance ID");
+ }
+ if (rc)
+ {
+ throw std::system_category().default_error_condition(rc);
+ }
}
private:
- std::bitset<maxInstanceIds> id;
+ struct pldm_instance_db* pldmInstanceIdDb = nullptr;
};
} // namespace pldm
diff --git a/pldmd/pldmd.cpp b/pldmd/pldmd.cpp
index 9f5bbaa..5da52f0 100644
--- a/pldmd/pldmd.cpp
+++ b/pldmd/pldmd.cpp
@@ -2,6 +2,7 @@
#include "common/utils.hpp"
#include "dbus_impl_requester.hpp"
#include "fw-update/manager.hpp"
+#include "instance_id.hpp"
#include "invoker.hpp"
#include "requester/handler.hpp"
#include "requester/mctp_endpoint_discovery.hpp"
@@ -196,7 +197,10 @@
auto& bus = pldm::utils::DBusHandler::getBus();
sdbusplus::server::manager_t objManager(bus,
"/xyz/openbmc_project/software");
- dbus_api::Requester dbusImplReq(bus, "/xyz/openbmc_project/pldm");
+
+ InstanceIdDb instanceIdDb;
+ dbus_api::Requester dbusImplReq(bus, "/xyz/openbmc_project/pldm",
+ instanceIdDb);
Invoker invoker{};
requester::Handler<requester::Request> reqHandler(
diff --git a/requester/test/handler_test.cpp b/requester/test/handler_test.cpp
index 5de1055..69facdb 100644
--- a/requester/test/handler_test.cpp
+++ b/requester/test/handler_test.cpp
@@ -3,6 +3,7 @@
#include "mock_request.hpp"
#include "pldmd/dbus_impl_requester.hpp"
#include "requester/handler.hpp"
+#include "test/test_instance_id.hpp"
#include <libpldm/base.h>
@@ -22,14 +23,15 @@
{
protected:
HandlerTest() :
- event(sdeventplus::Event::get_default()),
+ event(sdeventplus::Event::get_default()), instanceIdDb(),
dbusImplReq(pldm::utils::DBusHandler::getBus(),
- "/xyz/openbmc_project/pldm")
+ "/xyz/openbmc_project/pldm", instanceIdDb)
{}
int fd = 0;
mctp_eid_t eid = 0;
sdeventplus::Event event;
+ TestInstanceIdDb instanceIdDb;
pldm::dbus_api::Requester dbusImplReq;
/** @brief This function runs the sd_event_run in a loop till all the events
@@ -78,6 +80,7 @@
fd, event, dbusImplReq, false, 90000, seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
+ EXPECT_EQ(instanceId, 0);
auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
@@ -88,10 +91,7 @@
reqHandler.handleResponse(eid, instanceId, 0, 0, responsePtr,
sizeof(response));
- // handleResponse() will free the instance ID after calling the response
- // handler, so the same instance ID is granted next as well
EXPECT_EQ(validResponse, true);
- EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
}
TEST_F(HandlerTest, singleRequestInstanceIdTimerExpired)
@@ -100,6 +100,7 @@
fd, event, dbusImplReq, false, 90000, seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
+ EXPECT_EQ(instanceId, 0);
auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
@@ -108,9 +109,6 @@
// Waiting for 500ms so that the instance ID expiry callback is invoked
waitEventExpiry(milliseconds(500));
- // cleanup() will free the instance ID after calling the response
- // handler will no response, so the same instance ID is granted next
- EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
EXPECT_EQ(nullResponse, true);
}
@@ -120,6 +118,7 @@
fd, event, dbusImplReq, false, 90000, seconds(2), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
+ EXPECT_EQ(instanceId, 0);
auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
@@ -127,6 +126,7 @@
pldm::Request requestNxt{};
auto instanceIdNxt = dbusImplReq.getInstanceId(eid);
+ EXPECT_EQ(instanceIdNxt, 1);
rc = reqHandler.registerRequest(
eid, instanceIdNxt, 0, 0, std::move(requestNxt),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
@@ -149,5 +149,4 @@
EXPECT_EQ(validResponse, true);
EXPECT_EQ(callbackCount, 2);
- EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
}
diff --git a/requester/test/meson.build b/requester/test/meson.build
index 1e63997..b7a929f 100644
--- a/requester/test/meson.build
+++ b/requester/test/meson.build
@@ -1,10 +1,3 @@
-requester_inc = include_directories('../../')
-test_src = declare_dependency(
- sources: [
- '../../pldmd/dbus_impl_requester.cpp',
- '../../pldmd/instance_id.cpp'],
- include_directories:requester_inc)
-
tests = [
'handler_test',
'request_test',
@@ -13,6 +6,7 @@
foreach t : tests
test(t, executable(t.underscorify(), t + '.cpp',
implicit_include_directories: false,
+ include_directories: [ '../../', '../../pldmd' ],
link_args: dynamic_linker,
build_rpath: get_option('oe-sdk').enabled() ? rpath : '',
dependencies: [
@@ -24,6 +18,6 @@
phosphor_logging_dep,
sdbusplus,
sdeventplus,
- test_src]),
+ ]),
workdir: meson.current_source_dir())
endforeach
diff --git a/test/meson.build b/test/meson.build
index 04bbfdc..1ae20c8 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -1,11 +1,8 @@
pldmd_inc = include_directories('../')
test_src = declare_dependency(
- sources: [
- '../pldmd/instance_id.cpp'],
include_directories:pldmd_inc)
tests = [
- 'pldmd_instanceid_test',
'pldmd_registration_test',
]
diff --git a/test/pldmd_instanceid_test.cpp b/test/pldmd_instanceid_test.cpp
deleted file mode 100644
index 4a1f9f6..0000000
--- a/test/pldmd_instanceid_test.cpp
+++ /dev/null
@@ -1,39 +0,0 @@
-#include "pldmd/instance_id.hpp"
-
-#include <stdexcept>
-
-#include <gtest/gtest.h>
-
-using namespace pldm;
-
-TEST(InstanceId, testNext)
-{
- InstanceId id;
- ASSERT_EQ(id.next(), 0);
- ASSERT_EQ(id.next(), 1);
-}
-
-TEST(InstanceId, testAllUsed)
-{
- InstanceId id;
- for (size_t i = 0; i < maxInstanceIds; ++i)
- {
- ASSERT_EQ(id.next(), i);
- }
- EXPECT_THROW(id.next(), std::runtime_error);
-}
-
-TEST(InstanceId, testMarkfree)
-{
- InstanceId id;
- for (size_t i = 0; i < maxInstanceIds; ++i)
- {
- ASSERT_EQ(id.next(), i);
- }
- id.markFree(5);
- ASSERT_EQ(id.next(), 5);
- id.markFree(0);
- ASSERT_EQ(id.next(), 0);
- EXPECT_THROW(id.next(), std::runtime_error);
- EXPECT_THROW(id.markFree(32), std::out_of_range);
-}
diff --git a/test/test_instance_id.hpp b/test/test_instance_id.hpp
new file mode 100644
index 0000000..6979e56
--- /dev/null
+++ b/test/test_instance_id.hpp
@@ -0,0 +1,38 @@
+#pragma once
+
+#include "instance_id.hpp"
+
+static constexpr uintmax_t pldmMaxInstanceIds = 32;
+
+class TestInstanceIdDb : public pldm::InstanceIdDb
+{
+ public:
+ TestInstanceIdDb() : TestInstanceIdDb(createDb()) {}
+
+ ~TestInstanceIdDb()
+ {
+ std::filesystem::remove(dbPath);
+ };
+
+ private:
+ static std::filesystem::path createDb()
+ {
+ static const char dbTmpl[] = "/tmp/db.XXXXXX";
+ char dbName[sizeof(dbTmpl)] = {};
+
+ ::strncpy(dbName, dbTmpl, sizeof(dbName));
+ ::close(::mkstemp(dbName));
+
+ std::filesystem::path dbPath(dbName);
+ std::filesystem::resize_file(
+ dbPath, static_cast<uintmax_t>(PLDM_MAX_TIDS) * pldmMaxInstanceIds);
+
+ return dbPath;
+ };
+
+ TestInstanceIdDb(std::filesystem::path dbPath) :
+ InstanceIdDb(dbPath), dbPath(dbPath)
+ {}
+
+ std::filesystem::path dbPath;
+};