-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keyword VPD: Read from hardware and update the same on D-bus #457
base: P11_Dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,19 @@ class KeywordVpdParser : public ParserInterface | |
*/ | ||
types::VPDMapVariant parse(); | ||
|
||
/** | ||
* @brief API to read keyword's value from hardware | ||
* | ||
* @param[in] i_paramsToReadData - Data required to perform read | ||
* | ||
* @throw | ||
* sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument | ||
* | ||
* @return On success return the value read. On failure throw exception. | ||
*/ | ||
types::DbusVariantType | ||
readKeywordFromHardware(const types::ReadVpdParams i_paramsToReadData); | ||
|
||
private: | ||
/** | ||
* @brief Parse the VPD data and emplace them as pair into the Map. | ||
|
@@ -88,6 +101,19 @@ class KeywordVpdParser : public ParserInterface | |
*/ | ||
void checkNextBytesValidity(uint8_t numberOfBytes); | ||
|
||
/** | ||
* @brief API to iterate through the VPD vector to find the given keyword. | ||
* | ||
* This API iterates through VPD vector using m_vpdIterator and finds the | ||
* given keyword. m_vpdIterator points to the keyword name if find is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its findKeyword API, why "m_vpdIterator" should be modified ? And also we can't parse() or populateVpdMap() on this object as 'm_vpdIterator' is moved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
why can't you move again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the API name is findKeyword(), getter API's shouldn't modify anything.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a getter API. this just checks if the keyword is found or not. since we have m_iterator as a member variable which is common to any member of the class, this API uses it to iterate through the vector. Remember findKeyword() is not modifying the content of the vector. It is using the iterator variable to iterate through the vector. If m_iterator shouldn't be modified, then give me the purpose of having it as a non-const class member. Also this API acts as a common API to perform both read and write keyword. PR for write keyword #459 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Am telling you, if you wish to perform parse/populateVPDMap - you can move the iterator wherever you want to. |
||
* successful. | ||
* | ||
* @param[in] i_keyword - Keyword name. | ||
* | ||
* @return 0 On successful find, -1 on failure. | ||
*/ | ||
int findKeyword(const std::string& i_keyword); | ||
|
||
/*Vector of keyword VPD data*/ | ||
const types::BinaryVector& m_keywordVpdVector; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,4 +133,97 @@ void KeywordVpdParser::checkNextBytesValidity(uint8_t i_numberOfBytes) | |
} | ||
} | ||
|
||
types::DbusVariantType KeywordVpdParser::readKeywordFromHardware( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make it simple, In this way we no need to implement reading logic here . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that won't be simple when there is a read keyword request from D-bus. as for each and every request, populateVpdMap() creates the complete map of keyword-value pairs which is not required in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends on the where the keywond, it can be found at the beginning or can be found at the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't need a map with all keyword values for my case. so why should I use populateVpdMap() |
||
const types::ReadVpdParams i_paramsToReadData) | ||
{ | ||
types::Keyword l_keyword; | ||
|
||
if (const types::Keyword* l_kwData = | ||
std::get_if<types::Keyword>(&i_paramsToReadData)) | ||
{ | ||
l_keyword = *l_kwData; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to check is this value is empty or not . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
else | ||
{ | ||
logging::logMessage("Given VPD type is not supported."); | ||
throw types::DbusInvalidArgument(); | ||
} | ||
|
||
if (l_keyword.empty()) | ||
{ | ||
logging::logMessage("Given an empty keyword name."); | ||
throw types::DbusInvalidArgument(); | ||
} | ||
|
||
// Iterate through VPD vector to find the keyword | ||
if (findKeyword(l_keyword) != 0) | ||
{ | ||
logging::logMessage("Keyword " + l_keyword + " not found."); | ||
throw types::DbusInvalidArgument(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why DBUs error thrown here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the request is from Dbus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is KeywordVpdParser class, Reader can't know its request from DBus. We should keep API's generic and shouldn't bind to one use case. Future needs may come from different place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IPZ read/write also has similar logging when it fails. So let's discuss this as part of error logging story. |
||
} | ||
|
||
// Skip bytes representing the keyword name | ||
std::advance(m_vpdIterator, constants::TWO_BYTES); | ||
|
||
// Get size of the keyword | ||
const auto l_keywordSize = *m_vpdIterator; | ||
|
||
// Skip bytes representing the size of the keyword | ||
std::advance(m_vpdIterator, constants::ONE_BYTE); | ||
|
||
// Read the keyword's value and return | ||
return types::DbusVariantType{types::BinaryVector( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't do operation on return statement, just return the value instead found from the operation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do the operation and send the final value in the return statement, Dont do multiple things in return statement. What I realised from this is
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In my case, I don't need to construct an object for std::vector and std::variant just to return it back to the caller. In place construction optimises the memory usage. |
||
m_vpdIterator, std::ranges::next(m_vpdIterator, l_keywordSize, | ||
m_keywordVpdVector.cend()))}; | ||
} | ||
|
||
int KeywordVpdParser::findKeyword(const std::string& i_keyword) | ||
{ | ||
m_vpdIterator = m_keywordVpdVector.begin(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we check i_keyword is empty or not before doing further operations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
|
||
// Skip Keyword VPD's start tag | ||
std::advance(m_vpdIterator, sizeof(constants::KW_VPD_START_TAG)); | ||
|
||
// Get size of the header | ||
uint16_t l_dataSize = getKwDataSize(); | ||
|
||
// Skip bytes which represents the size of header + Skip header data | ||
std::advance(m_vpdIterator, constants::TWO_BYTES + l_dataSize); | ||
|
||
// Skip Keyword VPD pair's start tag | ||
std::advance(m_vpdIterator, constants::ONE_BYTE); | ||
|
||
// Get total size of keyword value pairs | ||
auto l_totalSize = getKwDataSize(); | ||
|
||
if (l_totalSize <= 0) | ||
{ | ||
// Keyword not found | ||
return -1; | ||
} | ||
|
||
// Skip bytes which represents the total size of kw-value pairs | ||
std::advance(m_vpdIterator, constants::TWO_BYTES); | ||
|
||
while (l_totalSize > 0) | ||
{ | ||
// Get keyword name | ||
std::string l_keywordName(m_vpdIterator, | ||
m_vpdIterator + constants::TWO_BYTES); | ||
|
||
if (l_keywordName == i_keyword) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use STR_CMP_SUCCESS here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not == ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can do same thing in number of ways, For string comparison we are using compare() api only in the current code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If == still comes under C++ standard, then why we should restrict using it? |
||
{ | ||
// Keyword successfully found | ||
return 0; | ||
} | ||
std::advance(m_vpdIterator, constants::TWO_BYTES); | ||
size_t l_kwSize = *m_vpdIterator; | ||
|
||
std::advance(m_vpdIterator, constants::ONE_BYTE + l_kwSize); | ||
l_totalSize -= constants::TWO_BYTES + constants::ONE_BYTE + l_kwSize; | ||
} | ||
|
||
// Keyword not found | ||
return -1; | ||
} | ||
} // namespace vpd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why "readKeywordFromHardware"
as parser itself is on hardware, should it be "readKeyword" ?
or this parser class reads keyword from DBus as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the method overriden from parser_interface class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I know,
but are we going to introduce readKeywordFrom DBus in the future ?. I wanted to understand the reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am not sure about the future :)
either IBM/any other company can implement readKeywordFromDBus.
Now If we focus on present, this API reads keyword's value from hardware. that's it.