requester: Enhance the PLDM requester code
- Free the instance id on error paths
- Make the conditional checks consistent
- Make the unit tests robust
Signed-off-by: Tom Joseph <rushtotom@gmail.com>
Change-Id: Ieb70da2820ab818c36f5bf874a602fb090659836
diff --git a/requester/handler.hpp b/requester/handler.hpp
index 2082f69..319cb8e 100644
--- a/requester/handler.hpp
+++ b/requester/handler.hpp
@@ -171,8 +171,9 @@
event.get(), instanceIdExpiryCallBack);
auto rc = request->start();
- if (rc != PLDM_SUCCESS)
+ if (rc)
{
+ requester.markFree(eid, instanceId);
std::cerr << "Failure to send the PLDM request message"
<< "\n";
return rc;
@@ -184,6 +185,7 @@
}
catch (const std::runtime_error& e)
{
+ requester.markFree(eid, instanceId);
std::cerr << "Failed to start the instance ID expiry timer. RC = "
<< e.what() << "\n";
return PLDM_ERROR;
diff --git a/requester/request.hpp b/requester/request.hpp
index 4097e10..2d74147 100644
--- a/requester/request.hpp
+++ b/requester/request.hpp
@@ -58,7 +58,7 @@
int start()
{
auto rc = send();
- if (rc != PLDM_SUCCESS)
+ if (rc)
{
return rc;
}
diff --git a/requester/test/handler_test.cpp b/requester/test/handler_test.cpp
index 7230b08..aa0655f 100644
--- a/requester/test/handler_test.cpp
+++ b/requester/test/handler_test.cpp
@@ -78,9 +78,10 @@
seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
- reqHandler.registerRequest(
+ auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+ EXPECT_EQ(rc, PLDM_SUCCESS);
pldm::Response response(sizeof(pldm_msg_hdr) + sizeof(uint8_t));
auto responsePtr = reinterpret_cast<const pldm_msg*>(response.data());
@@ -89,8 +90,8 @@
// handleResponse() will free the instance ID after calling the response
// handler, so the same instance ID is granted next as well
- ASSERT_EQ(validResponse, true);
- ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+ EXPECT_EQ(validResponse, true);
+ EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
}
TEST_F(HandlerTest, singleRequestInstanceIdTimerExpired)
@@ -99,17 +100,18 @@
seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
- reqHandler.registerRequest(
+ auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+ EXPECT_EQ(rc, PLDM_SUCCESS);
// 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
- ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
- ASSERT_EQ(nullResponse, true);
+ EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+ EXPECT_EQ(nullResponse, true);
}
TEST_F(HandlerTest, multipleRequestResponseScenario)
@@ -118,22 +120,24 @@
seconds(2), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
- reqHandler.registerRequest(
+ auto rc = reqHandler.registerRequest(
eid, instanceId, 0, 0, std::move(request),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+ EXPECT_EQ(rc, PLDM_SUCCESS);
pldm::Request requestNxt{};
auto instanceIdNxt = dbusImplReq.getInstanceId(eid);
- reqHandler.registerRequest(
+ rc = reqHandler.registerRequest(
eid, instanceIdNxt, 0, 0, std::move(requestNxt),
std::move(std::bind_front(&HandlerTest::pldmResponseCallBack, this)));
+ EXPECT_EQ(rc, PLDM_SUCCESS);
pldm::Response response(sizeof(pldm_msg_hdr) + sizeof(uint8_t));
auto responsePtr = reinterpret_cast<const pldm_msg*>(response.data());
reqHandler.handleResponse(eid, instanceIdNxt, 0, 0, responsePtr,
sizeof(response));
- ASSERT_EQ(validResponse, true);
- ASSERT_EQ(callbackCount, 1);
+ EXPECT_EQ(validResponse, true);
+ EXPECT_EQ(callbackCount, 1);
validResponse = false;
// Waiting for 500ms and handle the response for the first request, to
@@ -143,7 +147,7 @@
reqHandler.handleResponse(eid, instanceId, 0, 0, responsePtr,
sizeof(response));
- ASSERT_EQ(validResponse, true);
- ASSERT_EQ(callbackCount, 2);
- ASSERT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
+ EXPECT_EQ(validResponse, true);
+ EXPECT_EQ(callbackCount, 2);
+ EXPECT_EQ(instanceId, dbusImplReq.getInstanceId(eid));
}
\ No newline at end of file
diff --git a/requester/test/request_test.cpp b/requester/test/request_test.cpp
index 6471d9e..1b41ff6 100644
--- a/requester/test/request_test.cpp
+++ b/requester/test/request_test.cpp
@@ -54,7 +54,7 @@
.Times(Exactly(1))
.WillOnce(Return(PLDM_SUCCESS));
auto rc = request.start();
- ASSERT_EQ(rc, PLDM_SUCCESS);
+ EXPECT_EQ(rc, PLDM_SUCCESS);
}
TEST_F(RequestIntfTest, 2Retries100msTimeout)
@@ -64,7 +64,7 @@
// send() is called a total of 3 times, the original plus two retries
EXPECT_CALL(request, send()).Times(3).WillRepeatedly(Return(PLDM_SUCCESS));
auto rc = request.start();
- ASSERT_EQ(rc, PLDM_SUCCESS);
+ EXPECT_EQ(rc, PLDM_SUCCESS);
waitEventExpiry(milliseconds(500));
}
@@ -82,7 +82,7 @@
.Times(Between(5, 10))
.WillRepeatedly(Return(PLDM_SUCCESS));
auto rc = request.start();
- ASSERT_EQ(rc, PLDM_SUCCESS);
+ EXPECT_EQ(rc, PLDM_SUCCESS);
auto requestStopCallback = [&](void) { request.stop(); };
phosphor::Timer timer(event.get(), requestStopCallback);
@@ -97,5 +97,5 @@
milliseconds(100));
EXPECT_CALL(request, send()).Times(Exactly(1)).WillOnce(Return(PLDM_ERROR));
auto rc = request.start();
- ASSERT_EQ(rc, PLDM_ERROR);
+ EXPECT_EQ(rc, PLDM_ERROR);
}