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:Write keyword's value on hardware #459

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
33 changes: 31 additions & 2 deletions include/keyword_vpd_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ class KeywordVpdParser : public ParserInterface
*
* @param kwVpdVector - move it to object's m_keywordVpdVector
*/
KeywordVpdParser(const types::BinaryVector& kwVpdVector) :
KeywordVpdParser(const types::BinaryVector& kwVpdVector,
const std::string& vpdFilePath) :
m_keywordVpdVector(kwVpdVector),
m_vpdIterator(m_keywordVpdVector.begin())
m_vpdIterator(m_keywordVpdVector.begin()), m_vpdFilePath(vpdFilePath)
{}

/**
Expand All @@ -41,6 +42,18 @@ class KeywordVpdParser : public ParserInterface
*/
types::VPDMapVariant parse();

/**
* @brief API to write keyword's value on hardware.
*
* @param[in] i_paramsToWriteData - Data required to perform write.
*
* @throw sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument.
*
* @return On success returns number of bytes written on hardware, On
* failure throws exception.
*/
int writeKeywordOnHardware(const types::WriteVpdParams i_paramsToWriteData);

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

/*Iterator to VPD data*/
types::BinaryVector::const_iterator m_vpdIterator;

// Holds the VPD file path
const std::string& m_vpdFilePath;
};
} // namespace vpd
77 changes: 77 additions & 0 deletions src/keyword_vpd_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "exceptions.hpp"
#include "logger.hpp"

#include <fstream>
#include <iostream>
#include <numeric>
#include <string>
Expand Down Expand Up @@ -133,4 +134,80 @@ void KeywordVpdParser::checkNextBytesValidity(uint8_t i_numberOfBytes)
}
}

int KeywordVpdParser::findKeyword(const std::string& i_keyword)
{
(void)i_keyword;
return 0;
}

int KeywordVpdParser::writeKeywordOnHardware(
const types::WriteVpdParams i_paramsToWriteData)
{
types::Keyword l_keywordName;
types::BinaryVector l_keywordData;

// Extract keyword and value from i_paramsToWriteData
if (const types::KwData* l_kwData =
std::get_if<types::KwData>(&i_paramsToWriteData))
{
l_keywordName = std::get<0>(*l_kwData);
l_keywordData = std::get<1>(*l_kwData);
}
else
{
logging::logMessage("Given VPD type is not supported");
throw types::DbusInvalidArgument();
}

if (l_keywordData.size() == 0)
{
logging::logMessage("Given keyword's data is of length 0");

Choose a reason for hiding this comment

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

What are the possibilities of a keyword value being size 0?

Copy link
Author

Choose a reason for hiding this comment

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

User by mistake sending an empty buffer to update on VPD.

throw types::DbusInvalidArgument();
}

// Iterate through VPD vector to find the keyword
if (findKeyword(l_keywordName) != 0)

Choose a reason for hiding this comment

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

Where is this defined? Can this be done soon after line 161, so that you don't need to check the keyword data size if this keyword is not a valid one

Copy link
Author

Choose a reason for hiding this comment

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

The l_keywordData is the user given keyword's data to update on EEPROM.
The l_keywordName is the user given keyword's name which we are finding in the EEPROM at line 169.

{
logging::logMessage("Keyword " + l_keywordName + " not found.");
throw types::DbusInvalidArgument();
}

// Skip bytes representing the keyword name
std::advance(m_vpdIterator, constants::TWO_BYTES);

// Get size of the keyword
const auto l_keywordActualSize = *m_vpdIterator;

// If given keyword value is of greater length that it's actual length
if (l_keywordData.size() > l_keywordActualSize)
{
// Resize the given array to the right size
l_keywordData.resize(l_keywordActualSize);
l_keywordData.shrink_to_fit();

Choose a reason for hiding this comment

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

How to ensure the shrinking will get the correct value as a result? Will this not corrupt it? Should you throw here as invalid?

Copy link
Author

Choose a reason for hiding this comment

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

resize will erase the extra contents and resizes the vector to l_keywordActualSize. Here only the contents are erased but the vector capacity doesn't change.
So we use shrink_to_fit to remove the unused capacity. So it won't corrupt the vpd.

}

// Skip bytes representing the size of the keyword
std::advance(m_vpdIterator, constants::ONE_BYTE);

// Open filestream to write the value
std::fstream l_vpdFileStream;
l_vpdFileStream.exceptions(std::ifstream::badbit | std::ifstream::failbit);
l_vpdFileStream.open(m_vpdFilePath,
std::ios::in | std::ios::out | std::ios::binary);

// Get keyword value's offset
const auto l_keywordOffset = std::distance(m_keywordVpdVector.begin(),
m_vpdIterator);
// Goto keyword value's offset in filestream
l_vpdFileStream.seekp(constants::KW_VPD_DATA_START + l_keywordOffset,
std::ios::beg);

// Write given value in filestream
std::copy(l_keywordData.begin(), l_keywordData.end(),
std::ostreambuf_iterator<char>(l_vpdFileStream));

l_vpdFileStream.close();

return l_keywordData.size();
}
} // namespace vpd
3 changes: 2 additions & 1 deletion src/parser_factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ std::shared_ptr<ParserInterface>
{
logging::logMessage("KWD vpd parser selected for VPD file path " +
i_vpdFilePath);
return std::make_shared<KeywordVpdParser>(i_vpdVector);
return std::make_shared<KeywordVpdParser>(i_vpdVector,

Choose a reason for hiding this comment

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

Is this file path validated before using ?

Copy link
Author

Choose a reason for hiding this comment

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

validated at the caller's place here https://github.com/ibm-openbmc/openpower-vpd-parser/blob/P11_Dev/src/manager.cpp#L218 . But yeah we have to validate in this API as well so that we are safe even if anyother caller failed to validate.

Copy link
Author

Choose a reason for hiding this comment

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

we will create a new issue to address such validations across the repo.

i_vpdFilePath);
}

case vpdType::DDR5_DDIMM_MEMORY_VPD:
Expand Down
2 changes: 1 addition & 1 deletion test/utest_keyword_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ TEST(KeywordVpdParserTest, EmptyInput)
{
// Blank keyword VPD
types::BinaryVector emptyVector{};
KeywordVpdParser l_keywordParser(std::move(emptyVector));
KeywordVpdParser l_keywordParser(std::move(emptyVector), std::string());

EXPECT_THROW(l_keywordParser.parse(), std::exception);
}