Move to libpldm APIs #38
Conversation
|
I created one commit for both instance ID API and transport API changes. |
Lakshmi-y
force-pushed
the
libpldm-mctp
branch
2 times, most recently
from
e0f5858
to
74350bc
Compare
| /** @brief Close the PLDM file */ | ||
| void pldmClose(); |
There was a problem hiding this comment.
duplicate already at line 49
There was a problem hiding this comment.
I guess Lakshmi fixed this or I don't see what you mean.
| int retCode = encode_new_file_req( | ||
| pldmInstanceId, pldmDumpType, dumpId, dumpSize, | ||
| reinterpret_cast<pldm_msg*>(newFileAvailReqMsg.data())); | ||
| if (retCode != PLDM_SUCCESS) | ||
| { | ||
| freePLDMInstanceID(pldmInstanceId, mctpEndPointId); |
There was a problem hiding this comment.
RAII, enclose pldminstance id and mctp endpoint id in an internal c++ class object and when that goes out of scope it will call freePLDMInstanceID
| if (retCode != PLDM_REQUESTER_SUCCESS) | ||
| { | ||
| freePLDMInstanceID(pldmInstanceId, mctpEndPointId); | ||
| pldmClose(); |
There was a problem hiding this comment.
Please see Andrew Jeffrey's comments on https://gerrit.openbmc.org/c/openbmc/phosphor-debug-collector/+/63889 where he explained that RAII doesn't make sense for the instance IDs
| retCode = openPLDM(); | ||
| if (retCode < 0) | ||
| { | ||
| freePLDMInstanceID(pldmInstanceId, mctpEndPointId); |
There was a problem hiding this comment.
shouldn't we call pldmClose here?
There was a problem hiding this comment.
shouldn't we call pldmClose here?
No, since the openPLDM call failed.
|
|
||
| auto method = bus.new_method_call(objectMapperName, objectMapperPath, | ||
| objectMapperName, "GetObject"); | ||
| void initPLDMInstanceIdDb() |
There was a problem hiding this comment.
mixing c++ with C functions, why can't we have initPLDMInstanceIdDb, destroyPLDMInstanceIdDb as private methods of the PLDMInstanceManager
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| rc = pldm_instance_id_alloc(pldmInstanceIdDb, tid, &instanceID); |
There was a problem hiding this comment.
we try only once? or should it be in a loop ?
There was a problem hiding this comment.
As far as I understand, it is not likely to succeed a second time, so no point retrying.
| elog<NotAllowed>( | ||
| Reason("Required host dump action via pldm is not allowed because " | ||
| "pldmTransport already setup")); |
There was a problem hiding this comment.
this code will throw exception and return from this method, i guess this is not your intention
There was a problem hiding this comment.
I believe this is checking to make sure pldmTransport is not set up already, so it is intentional to throw exception here.
| "allowed due to pldm_open failed")); | ||
| log<level::ERR>( | ||
| fmt::format("openPLDM failed, errno({}), FD({})", e, fd).c_str()); | ||
| elog<NotAllowed>( |
There was a problem hiding this comment.
can't we use Unavailable here, every where we are using NotAllowed
| "tid to eid mapping, errno={}/{}", | ||
| errno, strerror(errno)) | ||
| .c_str()); | ||
| pldmClose(); |
There was a problem hiding this comment.
On what? It doesn't appear that the eid/tid needs to be cleaned up here, but perhaps at a higher level.
| int rc = pldm_transport_mctp_demux_init(&mctpDemux); | ||
| if (rc) | ||
| { | ||
| log<level::ERR>(std::format("openMctpDemuxTransport: Failed to init " |
There was a problem hiding this comment.
don't we need pldmClose here
There was a problem hiding this comment.
No, since pldmClose will clean up mctpDemux, but here that is failed to initialize.
… APIs Replace the current APIs with libpldm APIs for allocating instance IDs directly. This eliminates the need for remote D-Bus calls to the pldm daemon. Replace pldm transport APIs with libpldm pldm_transport APIs. This change migrates the application off the deprecated "requester" APIs in libpldm. Since we currently lack the infrastructure to obtain the correct TIDs, use the EID as the TID in the EID-to-TID mapping to maintain the current functionality. Change-Id: I348d54d9b5db47ef706cf7935e01ab8e7a55a594 Signed-off-by: Lakshmi Yadlapati <[email protected]>
libpldm provides APIs for allocating instance IDs directly, which eliminates the need for remote dbus calls to the pldm daemon. Refactor the code to use these APIs and eliminate all the dbus operations.
Replace pldm transport APIs with libpldm pldm_transport APIs to remove the dependency on pldm.
This change removes the dependency on pldm by utilizing the standardized libpldm APIs for transport operations, improving maintainability and compatibility.
We don't currently have the infrastructure in place to get the correct TIDs, so to keep everything working as before use the EID as the TID in the EID-to-TID mapping.
Change-Id: I348d54d9b5db47ef706cf7935e01ab8e7a55a594