pldm: LI 1TM IBM OEM FRU Names Support #642
Conversation
host-bmc/dbus/custom_dbus.cpp
Outdated
| bool isPresent) | ||
| void CustomDBus::updateInventoryItemProperties(const std::string& path, | ||
| bool isPresent, | ||
| const std::string& prettyName) |
There was a problem hiding this comment.
make this an std::optionalstd::string& instead
host-bmc/dbus/custom_dbus.cpp
Outdated
| // Set the pretty name dbus property to the filename | ||
| // form the dbus path object | ||
| presentStatus.at(path)->prettyName(ObjectPath.filename()); | ||
| if (prettyName.empty()) |
There was a problem hiding this comment.
if it is an std::optional string, then you can just check if the value is present - rather than checking for an string which is empty(), then use it ..otherwise use the ObjectPath.filename().
host-bmc/host_pdr_handler.cpp
Outdated
| @@ -760,6 +761,12 @@ void HostPDRHandler::processHostPDRs(mctp_eid_t /*eid*/, | |||
| updateContainerId<pldm_numeric_effecter_value_pdr>( | |||
| entityTree, pdr); | |||
| } | |||
| else if (pdrHdr->type == PLDM_ENTITY_AUXILIARY_NAMES_PDR) | |||
| { | |||
| pdrTerminusHandle = 0xFFFF; | |||
There was a problem hiding this comment.
why was this needed ?
There was a problem hiding this comment.
There is no specific terminus handle for PLDM_ENTITY_AUXILIARY_NAMES_PDR. So I put it as a default value instead of 0.
host-bmc/host_pdr_handler.cpp
Outdated
| std::shared_ptr<EntityAuxiliaryNames> | ||
| HostPDRHandler::parseEntityAuxNamesPDR(std::vector<uint8_t>& pdrData) |
There was a problem hiding this comment.
this is code duplication. I think its better to use the existing code by moving the parse function into the utils.cpp
There was a problem hiding this comment.
Yes this API almost matches with https://github.com/openbmc/pldm/blob/master/platform-mc/terminus.cpp#L307 except that in our use case we are not trimming the name. As of now I am moving this to a util API.
jaypadath
force-pushed
the
oem_ibm_fru_names
branch
4 times, most recently
from
14a2f1e to
94b96b8
Compare
| decodedPdr->container.entity_instance_num, | ||
| decodedPdr->container.entity_container_id}; | ||
|
|
||
| return std::make_shared<EntityAuxiliaryNames>(key, nameStrings); |
There was a problem hiding this comment.
is there any specific reason to use shared ptr here? Can't we avoid them?
There was a problem hiding this comment.
This matches with the upstream design of parsing the entity aux names and approved by the community. I assume std::make_shared is used for catering the design of selecting the key with a combination of values.
host-bmc/host_pdr_handler.cpp
Outdated
| auto it = std::find_if( | ||
| entityAuxiliaryNamesList.begin(), entityAuxiliaryNamesList.end(), | ||
| [entity]( | ||
| const std::shared_ptr<EntityAuxiliaryNames>& entityAuxiliaryNames) { | ||
| const auto& [key, entityNames] = *entityAuxiliaryNames; | ||
| return (entityAuxiliaryNames && key.type == entity.entity_type && | ||
| key.instanceIdx == entity.entity_instance_num && | ||
| key.containerId == entity.entity_container_id && | ||
| entityNames.size()); |
There was a problem hiding this comment.
this will become simple if we use map for entityAuxiliaryNamesList.
There was a problem hiding this comment.
Same comment as above and not sure whether I will be able to upstream with a different way to parse the same PDR.
| PDRList entityAuxNamesPDRs{}; | ||
|
|
||
| /** @brief Entity Auxiliary Names list */ | ||
| std::vector<std::shared_ptr<pldm::hostbmc::utils::EntityAuxiliaryNames>> |
There was a problem hiding this comment.
I think changing the datatype of "entityAuxiliaryNamesList" to map will simplify things and avoid unnecessary calculations. Do you think otherwise?
There was a problem hiding this comment.
It would be ideally same as I need to compare against each of the PDR entries to entity key parameters and there is no special calculation used here.
| @@ -22,6 +24,34 @@ namespace hostbmc | |||
| namespace utils | |||
| { | |||
|
|
|||
| using ContainerID = uint16_t; | |||
There was a problem hiding this comment.
Can we get rid of them as those are not adding much value??
There was a problem hiding this comment.
I thought this is more readable compared to rather than just putting as uint16_t,
|
jenkins run tests please |
1 similar comment
|
jenkins run tests please |
jaypadath
force-pushed
the
oem_ibm_fru_names
branch
2 times, most recently
from
cbd5561 to
0377d9d
Compare
This commit supports the addition of OEM FRU Names to the remote end point object. Pretty name of the inventory item is updated by extracting the entity name from the Entity Auxiliary Names PDR passed from the remote end point. Tested By: 1. Verified that the decode of Entity Auxiliary Names PDR results in the extraction of pretty name string 2. Verified that the pretty name string of the inventory dbus object is getting updated with new value Signed-off-by: japadath <[email protected]>
jaypadath
force-pushed
the
oem_ibm_fru_names
branch
from
0377d9d to
1d8d857
Compare
This commit supports the addition of IBM OEM FRU Names. Pretty name of the inventory item is updated by extracting the Entity Auxiliary Names PDR passed from host.
Tested By:
Verified that the decode of Entity Auxiliary Names PDR results in the intended pretty name string.