Refactor the verbose tracing in PLDM
1. Add verbose tracing in requester code
2. Change the prefix of verbose tracing to "Tx: " and "Rx: "
3. Remove explicit verbose tracing flag in libpldmresponder
4. Change the prefix of pldmtool verbose tracing to "pldmtool: Tx: "
and "pldmtool: Rx: "
Signed-off-by: Tom Joseph <rushtotom@gmail.com>
Change-Id: Ie2c6b323e32e0828ed5ecaeb3e61943a98a2f089
diff --git a/common/utils.cpp b/common/utils.cpp
index 31449dc..7f97320 100644
--- a/common/utils.cpp
+++ b/common/utils.cpp
@@ -492,10 +492,18 @@
return PLDM_INVALID_EFFECTER_ID;
}
-void printBuffer(const std::vector<uint8_t>& buffer, bool pldmVerbose)
+void printBuffer(bool isTx, const std::vector<uint8_t>& buffer)
{
- if (pldmVerbose && !buffer.empty())
+ if (!buffer.empty())
{
+ if (isTx)
+ {
+ std::cout << "Tx: ";
+ }
+ else
+ {
+ std::cout << "Rx: ";
+ }
std::ostringstream tempStream;
for (int byte : buffer)
{
diff --git a/common/utils.hpp b/common/utils.hpp
index 5bc99cd..72170fb 100644
--- a/common/utils.hpp
+++ b/common/utils.hpp
@@ -29,6 +29,8 @@
namespace fs = std::filesystem;
using Json = nlohmann::json;
+constexpr bool Tx = true;
+constexpr bool Rx = false;
/** @struct CustomFD
*
@@ -344,12 +346,13 @@
/** @brief Print the buffer
*
- * @param[in] buffer - Buffer to print
- * @param[in] pldmVerbose -verbosity flag - true/false
+ * @param[in] isTx - True if the buffer is an outgoing PLDM message, false if
+ the buffer is an incoming PLDM message
+ * @param[in] buffer - Buffer to print
*
* @return - None
*/
-void printBuffer(const std::vector<uint8_t>& buffer, bool pldmVerbose);
+void printBuffer(bool isTx, const std::vector<uint8_t>& buffer);
/** @brief Convert the buffer to std::string
*
diff --git a/host-bmc/dbus_to_host_effecters.cpp b/host-bmc/dbus_to_host_effecters.cpp
index 2ed96c0..2486c64 100644
--- a/host-bmc/dbus_to_host_effecters.cpp
+++ b/host-bmc/dbus_to_host_effecters.cpp
@@ -269,19 +269,6 @@
return rc;
}
- if (verbose)
- {
- if (requestMsg.size())
- {
- std::ostringstream tempStream;
- for (int byte : requestMsg)
- {
- tempStream << std::setfill('0') << std::setw(2) << std::hex
- << byte << " ";
- }
- std::cout << tempStream.str() << std::endl;
- }
- }
auto setStateEffecterStatesRespHandler =
[](mctp_eid_t /*eid*/, const pldm_msg* response, size_t respMsgLen) {
if (response == nullptr || !respMsgLen)
diff --git a/host-bmc/dbus_to_host_effecters.hpp b/host-bmc/dbus_to_host_effecters.hpp
index e274540..3dd8c38 100644
--- a/host-bmc/dbus_to_host_effecters.hpp
+++ b/host-bmc/dbus_to_host_effecters.hpp
@@ -76,17 +76,14 @@
* @param[in] dbusHandler - D-bus Handler
* @param[in] jsonPath - path for the json file
* @param[in] handler - PLDM request handler
- * @param[in] verbose - verbosity
*/
explicit HostEffecterParser(
pldm::dbus_api::Requester* requester, int fd, const pldm_pdr* repo,
pldm::utils::DBusHandler* const dbusHandler,
const std::string& jsonPath,
- pldm::requester::Handler<pldm::requester::Request>* handler,
- bool verbose = false) :
+ pldm::requester::Handler<pldm::requester::Request>* handler) :
requester(requester),
- sockFd(fd), pdrRepo(repo), dbusHandler(dbusHandler), handler(handler),
- verbose(verbose)
+ sockFd(fd), pdrRepo(repo), dbusHandler(dbusHandler), handler(handler)
{
try
{
@@ -182,7 +179,6 @@
const pldm::utils::DBusHandler* dbusHandler; //!< D-bus Handler
/** @brief PLDM request handler */
pldm::requester::Handler<pldm::requester::Request>* handler;
- bool verbose; //!< verbose flag
};
} // namespace host_effecters
diff --git a/host-bmc/host_pdr_handler.cpp b/host-bmc/host_pdr_handler.cpp
index b8b362f..e09989a 100644
--- a/host-bmc/host_pdr_handler.cpp
+++ b/host-bmc/host_pdr_handler.cpp
@@ -31,12 +31,11 @@
int mctp_fd, uint8_t mctp_eid, sdeventplus::Event& event, pldm_pdr* repo,
const std::string& eventsJsonsDir, pldm_entity_association_tree* entityTree,
pldm_entity_association_tree* bmcEntityTree, Requester& requester,
- pldm::requester::Handler<pldm::requester::Request>* handler, bool verbose) :
+ pldm::requester::Handler<pldm::requester::Request>* handler) :
mctp_fd(mctp_fd),
mctp_eid(mctp_eid), event(event), repo(repo),
stateSensorHandler(eventsJsonsDir), entityTree(entityTree),
- bmcEntityTree(bmcEntityTree), requester(requester), handler(handler),
- verbose(verbose)
+ bmcEntityTree(bmcEntityTree), requester(requester), handler(handler)
{
fs::path hostFruJson(fs::path(HOST_JSONS_DIR) / fruJson);
if (fs::exists(hostFruJson))
@@ -147,11 +146,6 @@
std::cerr << "Failed to encode_get_pdr_req, rc = " << rc << std::endl;
return;
}
- if (verbose)
- {
- std::cout << "Sending Msg:" << std::endl;
- printBuffer(requestMsg, verbose);
- }
rc = handler->registerRequest(
mctp_eid, instanceId, PLDM_PLATFORM, PLDM_GET_PDR,
@@ -395,11 +389,6 @@
std::vector<uint8_t> responsePDRMsg;
responsePDRMsg.resize(respMsgLen + sizeof(pldm_msg_hdr));
memcpy(responsePDRMsg.data(), response, respMsgLen + sizeof(pldm_msg_hdr));
- if (verbose)
- {
- std::cout << "Receiving Msg:" << std::endl;
- printBuffer(responsePDRMsg, verbose);
- }
if (rc != PLDM_SUCCESS)
{
std::cerr << "Failed to decode_get_pdr_resp, rc = " << rc << std::endl;
@@ -560,10 +549,6 @@
requester.markFree(mctp_eid, instanceId);
return;
}
- if (verbose)
- {
- printBuffer(requestMsg, verbose);
- }
auto getPLDMVersionHandler = [this](mctp_eid_t /*eid*/,
const pldm_msg* response,
diff --git a/host-bmc/host_pdr_handler.hpp b/host-bmc/host_pdr_handler.hpp
index ccafa66..6e8e03d 100644
--- a/host-bmc/host_pdr_handler.hpp
+++ b/host-bmc/host_pdr_handler.hpp
@@ -103,8 +103,7 @@
pldm_entity_association_tree* entityTree,
pldm_entity_association_tree* bmcEntityTree,
pldm::dbus_api::Requester& requester,
- pldm::requester::Handler<pldm::requester::Request>* handler,
- bool verbose = false);
+ pldm::requester::Handler<pldm::requester::Request>* handler);
/** @brief fetch PDRs from host firmware. See @class.
* @param[in] recordHandles - list of record handles pointing to host's
@@ -265,7 +264,6 @@
* PlatformEventMessage command request.
*/
HostStateSensorMap sensorMap;
- bool verbose;
/** @brief whether response received from Host */
bool responseReceived;
diff --git a/pldmd/pldmd.cpp b/pldmd/pldmd.cpp
index 63f13e1..9114eaf 100644
--- a/pldmd/pldmd.cpp
+++ b/pldmd/pldmd.cpp
@@ -171,7 +171,7 @@
Invoker invoker{};
requester::Handler<requester::Request> reqHandler(sockfd, event,
- dbusImplReq);
+ dbusImplReq, verbose);
#ifdef LIBPLDMRESPONDER
using namespace pldm::state_sensor;
@@ -196,8 +196,7 @@
{
hostPDRHandler = std::make_shared<HostPDRHandler>(
sockfd, hostEID, event, pdrRepo.get(), EVENTS_JSONS_DIR,
- entityTree.get(), bmcEntityTree.get(), dbusImplReq, &reqHandler,
- verbose);
+ entityTree.get(), bmcEntityTree.get(), dbusImplReq, &reqHandler);
// HostFirmware interface needs access to hostPDR to know if host
// is running
dbusImplHost.setHostPdrObj(hostPDRHandler);
@@ -205,7 +204,7 @@
hostEffecterParser =
std::make_unique<pldm::host_effecters::HostEffecterParser>(
&dbusImplReq, sockfd, pdrRepo.get(), dbusHandler.get(),
- HOST_JSONS_DIR, &reqHandler, verbose);
+ HOST_JSONS_DIR, &reqHandler);
dbusToPLDMEventHandler = std::make_unique<DbusToPLDMEvent>(
sockfd, hostEID, dbusImplReq, &reqHandler);
}
@@ -318,9 +317,9 @@
{
if (verbose)
{
- std::cout << "Received Msg" << std::endl;
- printBuffer(requestMsg, verbose);
+ printBuffer(Rx, requestMsg);
}
+
if (MCTP_MSG_TYPE_PLDM != requestMsg[1])
{
// Skip this message and continue.
@@ -336,8 +335,7 @@
{
if (verbose)
{
- std::cout << "Sending Msg" << std::endl;
- printBuffer(*response, verbose);
+ printBuffer(Tx, *response);
}
iov[0].iov_base = &requestMsg[0];
diff --git a/pldmtool/pldm_cmd_helper.cpp b/pldmtool/pldm_cmd_helper.cpp
index 0bb2941..92979c0 100644
--- a/pldmtool/pldm_cmd_helper.cpp
+++ b/pldmtool/pldm_cmd_helper.cpp
@@ -193,8 +193,11 @@
pldmVerbose = true;
}
- Logger(pldmVerbose, "Request Message:", "");
- printBuffer(requestMsg, pldmVerbose);
+ if (pldmVerbose)
+ {
+ std::cout << "pldmtool: ";
+ printBuffer(Tx, requestMsg);
+ }
if (mctp_eid != PLDM_ENTITY_ID)
{
@@ -211,18 +214,24 @@
requestMsg.size() - 2, &responseMessage,
&responseMessageSize);
- Logger(pldmVerbose, "Response Message:", "");
responseMsg.resize(responseMessageSize);
memcpy(responseMsg.data(), responseMessage, responseMsg.size());
free(responseMessage);
- printBuffer(responseMsg, pldmVerbose);
+ if (pldmVerbose)
+ {
+ std::cout << "pldmtool: ";
+ printBuffer(Rx, responseMsg);
+ }
}
else
{
mctpSockSendRecv(requestMsg, responseMsg, mctpVerbose);
- Logger(pldmVerbose, "Response Message:", "");
- printBuffer(responseMsg, pldmVerbose);
+ if (pldmVerbose)
+ {
+ std::cout << "pldmtool: ";
+ printBuffer(Rx, responseMsg);
+ }
responseMsg.erase(responseMsg.begin(),
responseMsg.begin() + 2 /* skip the mctp header */);
}
diff --git a/requester/handler.hpp b/requester/handler.hpp
index 2af4ade..6290526 100644
--- a/requester/handler.hpp
+++ b/requester/handler.hpp
@@ -91,19 +91,21 @@
* @param[in] fd - fd of MCTP communications socket
* @param[in] event - reference to PLDM daemon's main event loop
* @param[in] requester - reference to Requester object
+ * @param[in] verbose - verbose tracing flag
* @param[in] instanceIdExpiryInterval - instance ID expiration interval
* @param[in] numRetries - number of request retries
* @param[in] responseTimeOut - time to wait between each retry
*/
explicit Handler(
int fd, sdeventplus::Event& event, pldm::dbus_api::Requester& requester,
+ bool verbose,
std::chrono::seconds instanceIdExpiryInterval =
std::chrono::seconds(INSTANCE_ID_EXPIRATION_INTERVAL),
uint8_t numRetries = static_cast<uint8_t>(NUMBER_OF_REQUEST_RETRIES),
std::chrono::milliseconds responseTimeOut =
std::chrono::milliseconds(RESPONSE_TIME_OUT)) :
fd(fd),
- event(event), requester(requester),
+ event(event), requester(requester), verbose(verbose),
instanceIdExpiryInterval(instanceIdExpiryInterval),
numRetries(numRetries), responseTimeOut(responseTimeOut)
{}
@@ -162,7 +164,8 @@
};
auto request = std::make_unique<RequestInterface>(
- fd, eid, event, std::move(requestMsg), numRetries, responseTimeOut);
+ fd, eid, event, std::move(requestMsg), numRetries, responseTimeOut,
+ verbose);
auto timer = std::make_unique<phosphor::Timer>(
event.get(), instanceIdExpiryCallBack);
@@ -237,15 +240,16 @@
int fd; //!< file descriptor of MCTP communications socket
sdeventplus::Event& event; //!< reference to PLDM daemon's main event loop
pldm::dbus_api::Requester& requester; //!< reference to Requester object
+ bool verbose; //!< verbose tracing flag
std::chrono::seconds
instanceIdExpiryInterval; //!< Instance ID expiration interval
uint8_t numRetries; //!< number of request retries
std::chrono::milliseconds
responseTimeOut; //!< time to wait between each retry
- /** @brief Container for storing the details of the PLDM request message,
- * handler for the corresponding PLDM response and the timer object
- * for the Instance ID expiration
+ /** @brief Container for storing the details of the PLDM request
+ * message, handler for the corresponding PLDM response and the
+ * timer object for the Instance ID expiration
*/
using RequestValue =
std::tuple<std::unique_ptr<RequestInterface>, ResponseHandler,
diff --git a/requester/request.hpp b/requester/request.hpp
index f1a91d7..24e6942 100644
--- a/requester/request.hpp
+++ b/requester/request.hpp
@@ -4,6 +4,7 @@
#include "libpldm/requester/pldm.h"
#include "common/types.hpp"
+#include "common/utils.hpp"
#include <sdbusplus/timer.hpp>
#include <sdeventplus/event.hpp>
@@ -144,18 +145,20 @@
* @param[in] requestMsg - PLDM request message
* @param[in] numRetries - number of request retries
* @param[in] timeout - time to wait between each retry in milliseconds
+ * @param[in] verbose - verbose tracing flag
*/
explicit Request(int fd, mctp_eid_t eid, sdeventplus::Event& event,
pldm::Request&& requestMsg, uint8_t numRetries,
- std::chrono::milliseconds timeout) :
+ std::chrono::milliseconds timeout, bool verbose) :
RequestRetryTimer(event, numRetries, timeout),
- fd(fd), eid(eid), requestMsg(std::move(requestMsg))
+ fd(fd), eid(eid), requestMsg(std::move(requestMsg)), verbose(verbose)
{}
private:
int fd; //!< file descriptor of MCTP communications socket
mctp_eid_t eid; //!< endpoint ID of the remote MCTP endpoint
pldm::Request requestMsg; //!< PLDM request message
+ bool verbose; //!< verbose tracing flag
/** @brief Sends the PLDM request message on the socket
*
@@ -163,6 +166,10 @@
*/
int send() const
{
+ if (verbose)
+ {
+ pldm::utils::printBuffer(pldm::utils::Tx, requestMsg);
+ }
auto rc = pldm_send(eid, fd, requestMsg.data(), requestMsg.size());
if (rc < 0)
{
diff --git a/requester/test/handler_test.cpp b/requester/test/handler_test.cpp
index aa0655f..144e5f5 100644
--- a/requester/test/handler_test.cpp
+++ b/requester/test/handler_test.cpp
@@ -74,7 +74,7 @@
TEST_F(HandlerTest, singleRequestResponseScenario)
{
- Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq,
+ Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq, false,
seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
@@ -96,7 +96,7 @@
TEST_F(HandlerTest, singleRequestInstanceIdTimerExpired)
{
- Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq,
+ Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq, false,
seconds(1), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
@@ -116,7 +116,7 @@
TEST_F(HandlerTest, multipleRequestResponseScenario)
{
- Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq,
+ Handler<NiceMock<MockRequest>> reqHandler(fd, event, dbusImplReq, false,
seconds(2), 2, milliseconds(100));
pldm::Request request{};
auto instanceId = dbusImplReq.getInstanceId(eid);
diff --git a/requester/test/mock_request.hpp b/requester/test/mock_request.hpp
index 8b51a03..355609d 100644
--- a/requester/test/mock_request.hpp
+++ b/requester/test/mock_request.hpp
@@ -16,7 +16,7 @@
public:
MockRequest(int /*fd*/, mctp_eid_t /*eid*/, sdeventplus::Event& event,
pldm::Request&& /*requestMsg*/, uint8_t numRetries,
- std::chrono::milliseconds responseTimeOut) :
+ std::chrono::milliseconds responseTimeOut, bool /*verbose*/) :
RequestRetryTimer(event, numRetries, responseTimeOut)
{}
diff --git a/requester/test/request_test.cpp b/requester/test/request_test.cpp
index 1b41ff6..5239c12 100644
--- a/requester/test/request_test.cpp
+++ b/requester/test/request_test.cpp
@@ -49,7 +49,7 @@
TEST_F(RequestIntfTest, 0Retries100msTimeout)
{
MockRequest request(fd, eid, event, std::move(requestMsg), 0,
- milliseconds(100));
+ milliseconds(100), false);
EXPECT_CALL(request, send())
.Times(Exactly(1))
.WillOnce(Return(PLDM_SUCCESS));
@@ -60,7 +60,7 @@
TEST_F(RequestIntfTest, 2Retries100msTimeout)
{
MockRequest request(fd, eid, event, std::move(requestMsg), 2,
- milliseconds(100));
+ milliseconds(100), false);
// 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();
@@ -71,7 +71,7 @@
TEST_F(RequestIntfTest, 9Retries100msTimeoutRequestStoppedAfter1sec)
{
MockRequest request(fd, eid, event, std::move(requestMsg), 9,
- milliseconds(100));
+ milliseconds(100), false);
// send() will be called a total of 10 times, the original plus 9 retries.
// In a ideal scenario send() would have been called 10 times in 1 sec (when
// the timer is stopped) with a timeout of 100ms. Because there are delays
@@ -94,7 +94,7 @@
TEST_F(RequestIntfTest, 2Retries100msTimeoutsendReturnsError)
{
MockRequest request(fd, eid, event, std::move(requestMsg), 2,
- milliseconds(100));
+ milliseconds(100), false);
EXPECT_CALL(request, send()).Times(Exactly(1)).WillOnce(Return(PLDM_ERROR));
auto rc = request.start();
EXPECT_EQ(rc, PLDM_ERROR);