Skip to content
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

Open
wants to merge 1 commit into
base: P11_Dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions include/keyword_vpd_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

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 ?

Copy link
Author

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.

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.

Copy link
Author

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.


private:
/**
* @brief Parse the VPD data and emplace them as pair into the Map.
Expand Down Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its findKeyword API, why "m_vpdIterator" should be modified ?
instead you can return the pointer of the found keyword(or the position), incase if you need this data.

And also we can't parse() or populateVpdMap() on this object as 'm_vpdIterator' is moved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also we can't parse() or populateVpdMap() on this object as 'm_vpdIterator' is moved.

why can't you move again?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the API name is findKeyword(), getter API's shouldn't modify anything.

why can't you move again?
is this is a suggestion or asking we can move it. ?

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't you move again?
is this is a suggestion or asking we can move it. ?

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;

Expand Down
93 changes: 93 additions & 0 deletions src/keyword_vpd_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,97 @@ void KeywordVpdParser::checkNextBytesValidity(uint8_t i_numberOfBytes)
}
}

types::DbusVariantType KeywordVpdParser::readKeywordFromHardware(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make it simple,
use populateVpdMap() which gets the keyword map for us.
use this map to get the particular keyword value.

In this way we no need to implement reading logic here .

Copy link
Author

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check is this value is empty or not .

Copy link
Author

Choose a reason for hiding this comment

The 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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why DBUs error thrown here ?
its h/w operation right .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the request is from Dbus

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is KeywordVpdParser class, Reader can't know its request from DBus.
Request can come from any where, we can't use DBus error here.

We should keep API's generic and shouldn't bind to one use case. Future needs may come from different place.

Copy link
Author

Choose a reason for hiding this comment

The 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(

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Choose a reason for hiding this comment

The 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.
This is the review comment got it for my PR.

What I realised from this is

  1. It avoids any possible error calculations made at return statement.
  2. For developer testing or in future code debugging, you can add print statement to print the value for debugging.
  3. in case we need logging, we can easily add without rewriting the return statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It avoids any possible error calculations made at return statement.
    it's developer's responsibility to check on the calculations. if there are any errors we can fix it with defect. Having a separate variable doesn't guarantee that the calculation will be error free.

  2. For developer testing or in future code debugging, you can add print statement to print the value for debugging.
    Code doesn't stop you to add print statements for debugging.

  3. in case we need logging, we can easily add without rewriting the return statement.
    Now in this case is it required to log a message with variant<vector<uint8_t> contents?

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();

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use STR_CMP_SUCCESS here ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not == ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do same thing in number of ways,
if we are following certain things it should be followed every where.

For string comparison we are using compare() api only in the current code.
As I couldn't recall when this is introduced, we can get clarity from the team

Copy link
Author

Choose a reason for hiding this comment

The 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
28 changes: 28 additions & 0 deletions src/parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,34 @@ int Parser::updateVpdKeyword(const types::WriteVpdParams& i_paramsToWriteData)
return -1;
}
}
else if (const types::KwData* l_kwData =
std::get_if<types::KwData>(&i_paramsToWriteData))
{
l_interfaceName = constants::kwdVpdInf;
l_propertyName = std::get<0>(*l_kwData);

try
{
// Read keyword's value from hardware to write the same on
// D-bus.
std::shared_ptr<ParserInterface> l_vpdParserInstance =
getVpdParserInstance();
logging::logMessage("Performing VPD read on " + m_vpdFilePath);
l_keywordValue = l_vpdParserInstance->readKeywordFromHardware(
types::ReadVpdParams(l_propertyName));
}
catch (const std::exception& l_exception)
{
// Unable to read keyword's value from hardware.
logging::logMessage(
"Error while reading keyword's value from hadware path " +
m_vpdFilePath +
", error: " + std::string(l_exception.what()));

// TODO: Log PEL
return -1;
}
}
else
{
// Input parameter type provided isn't compatible to perform update.
Expand Down