Skip to content
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
10 changes: 10 additions & 0 deletions vpd-manager/include/ipz_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@ class IpzVpdParser : public ParserInterface
*/
int writeKeywordOnHardware(const types::WriteVpdParams i_paramsToWriteData);

/**
* @brief API to perform sanity check on EEPROM.
* It verifies it's header, vtoc and each record and their ECC.
*
* @return 0 on Success, -1 on Failure.
* @throw DataException
* @throw EccException
*/
virtual int sanityChecker() override;

private:
/**
* @brief Check ECC of VPD header.
Expand Down
9 changes: 9 additions & 0 deletions vpd-manager/include/manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,15 @@ class Manager
*/
void performVpdRecollection();

/**
* @brief API to perform sanity check on EEPROM.
*
* @param[in] i_fruPath - EEPROM path
*
* @return 0 on Success, -1 on Failure.
*/
int performVpdSanityCheck(const types::Path& i_fruPath) noexcept;

/**
* @brief Get unexpanded location code.
*
Expand Down
8 changes: 8 additions & 0 deletions vpd-manager/include/parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ class Parser
int updateVpdKeywordOnHardware(
const types::WriteVpdParams& i_paramsToWriteData);

/*
* @brief API to perform sanity check on EEPROM passed via constructor of
* parser class.
*
* @return 0 on Success, -1 on Failure.
*/
int vpdSanityCheck() noexcept;

private:
/**
* @brief Update keyword value on redundant path.
Expand Down
11 changes: 11 additions & 0 deletions vpd-manager/include/parser_interface.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@ class ParserInterface
return -1;
}

/**
* @brief Virtual API to perform sanity check on EEPROM passed via
* constructor of parser class.
*
* @return 0 on Success, -1 on Failure.
*/
virtual int sanityChecker()
{
return -1;

Choose a reason for hiding this comment

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

use constants SUCCESS or FAILURE.

Currently ipz parser class is implementing its own sanityChecker,
so other inherited classes are implementing this API, so we should safely say check is passed unless all child class has its won implementation ?

}

/**
* @brief Virtual destructor.
*/
Expand Down
34 changes: 34 additions & 0 deletions vpd-manager/src/ipz_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -996,4 +996,38 @@ bool IpzVpdParser::processInvalidRecords(
}
return l_rc;
}

int IpzVpdParser::sanityChecker()
{
try
{
logging::logMessage(

Choose a reason for hiding this comment

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

keep necessary logs only. This log is not required.

Copy link
Author

Choose a reason for hiding this comment

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

we are not sure, it ran for 1 or more eeproms( redundant), in that case this log will be a proof.

Choose a reason for hiding this comment

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

okay

"Performing sanity check for file path: " + m_vpdFilePath);
auto l_itrToVPD = m_vpdVector.cbegin();

Choose a reason for hiding this comment

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

l_itrToVpd


// Check vaidity of VHDR record
checkHeader(l_itrToVPD);

// Read the table of contents
const auto l_ptLen = readTOC(l_itrToVPD);

// Read the table of contents record, to get offsets to other records.
auto l_result = readPT(l_itrToVPD, l_ptLen);

if (!processInvalidRecords(l_result.second))
{
logging::logMessage("Failed to process invalid records for [" +
m_vpdFilePath + "]");
}
return constants::SUCCESS;
}
catch (const std::exception& l_ex)
{
logging::logMessage("Sanity Check failed for EEPROM [" + m_vpdFilePath +
"], reason: " + std::string(l_ex.what()));

return constants::FAILURE;
}
}

} // namespace vpd
38 changes: 38 additions & 0 deletions vpd-manager/src/manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ Manager::Manager(
this->performVpdRecollection();
});

iFace->register_method(
"performVpdSanityCheck", [this](const types::Path& i_fruPath) {
return this->performVpdSanityCheck(i_fruPath);
});

// Indicates FRU VPD collection for the system has not started.
iFace->register_property_rw<std::string>(
"CollectionStatus", sdbusplus::vtable::property_::emits_change,
Expand Down Expand Up @@ -640,4 +645,37 @@ void Manager::performVpdRecollection()
m_worker->performVpdRecollection();
}
}

int Manager::performVpdSanityCheck(const types::Path& i_fruPath) noexcept
{
try
{
if (i_fruPath.empty())
{
throw std::runtime_error("Given FRU path is empty");
}

nlohmann::json l_sysCfgJsonObj{};

Choose a reason for hiding this comment

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

we can move this after if condition, as there is no use in case if condition gets executed.


if (m_worker.get() == nullptr)
{
throw std::runtime_error(
"Worker object not found. Can't perform VPD Sanity check.");
}
l_sysCfgJsonObj = m_worker->getSysCfgJsonObj();

std::shared_ptr<Parser> l_parserObj =
std::make_shared<Parser>(i_fruPath, l_sysCfgJsonObj);

return l_parserObj->vpdSanityCheck();
}
catch (const std::exception& l_exception)
{
logging::logMessage("Sanity check failed for file[" + i_fruPath +
"], reason: " + std::string(l_exception.what()));

return constants::FAILURE;
}
}

} // namespace vpd
60 changes: 60 additions & 0 deletions vpd-manager/src/parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -346,4 +346,64 @@ int Parser::updateVpdKeywordOnHardware(
return l_bytesUpdatedOnHardware;
}

int Parser::vpdSanityCheck() noexcept
{
int l_rc = constants::SUCCESS;

Choose a reason for hiding this comment

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

Variable can be moved inside try, as there is no use outside of the try-catch block.

uint16_t l_errCode = 0;

Choose a reason for hiding this comment

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

move the scope of this variable, just before where it gets consumed.

try
{
std::shared_ptr<vpd::ParserInterface> l_parser = getVpdParserInstance();

if (l_parser->sanityChecker() == constants::FAILURE)
{
logging::logMessage("Sanity checker failed for " + m_vpdFilePath);
l_rc = constants::FAILURE;
}
logging::logMessage("Sanity checker Passed for " + m_vpdFilePath);

// Collect redundant FRU path
const auto l_redundantPath =
jsonUtility::getRedundantEepromPathFromJson(
m_parsedJson, m_vpdFilePath, l_errCode);
if (l_errCode)
{
logging::logMessage(
"Failed to get redundant EEPROM path for FRU [" +
m_vpdFilePath +
"], error : " + vpdSpecificUtility::getErrCodeMsg(l_errCode));
return l_rc;

Choose a reason for hiding this comment

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

Are we returning success, in case get redundant eeprom path call fails ?

}

if (l_redundantPath.empty())
{
logging::logMessage(

Choose a reason for hiding this comment

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

no need of this log, as most of the FRUs' don't have redundant EEPROM path, returning from this API is enough.

"No redundant path found for FRU [" + m_vpdFilePath + "]");
return l_rc;
}
// Read the VPD data into a vector.
vpdSpecificUtility::getVpdDataInVector(l_redundantPath, m_vpdVector,
m_vpdStartOffset);

// This will detect the type of parser required.
std::shared_ptr<vpd::ParserInterface> l_parser1 =

Choose a reason for hiding this comment

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

why l_parser1 as we dont have multiple parser objects here.
we can just say l_parserObj

ParserFactory::getParser(m_vpdVector, l_redundantPath,
m_vpdStartOffset);

if (l_parser1->sanityChecker() == constants::FAILURE)
{
logging::logMessage("Sanity checker failed for " + l_redundantPath);
l_rc = constants::FAILURE;

Choose a reason for hiding this comment

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

as there is no return from inside if condition, both fail and success log will be printed, in case sanity check failed for the FRU.
return from here.

}
logging::logMessage("Sanity checker Passed for " + l_redundantPath);

return l_rc;
}
catch (const std::exception& l_exception)
{
logging::logMessage("Sanity Check failed for EEPROM [" + m_vpdFilePath +
"], reason: " + std::string(l_exception.what()));
return constants::FAILURE;
}
}

} // namespace vpd
14 changes: 14 additions & 0 deletions vpd-tool/include/vpd_tool.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -371,5 +371,19 @@ class VpdTool
* the directory from the filesystem
*/
void clearVpdDumpDir() const noexcept;

/**
* @brief API validates record against the ECC for that record.
*
* This API calls performVpdSanityCheck dbus API to verify the record and
* ECC of user provided EEPROM path. This performVpdSanityCheck dbus API
* internally checks for redundant EEPROM for this FRU. If found , it
* verifies the same for redundant one also.
*
* @param[in] i_vpdFilePath - EEPROM path to perform sanity check
*
* @return 0 on Success, -1 on Failure.
*/
int performSanityCheck(const std::string& i_vpdFilePath) noexcept;
};
} // namespace vpd
45 changes: 45 additions & 0 deletions vpd-tool/src/vpd_tool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1558,4 +1558,49 @@ void VpdTool::clearVpdDumpDir() const noexcept
}
}

int VpdTool::performSanityCheck(const std::string& i_fruPath) noexcept
{
try
{
nlohmann::json l_parsedJson;

Choose a reason for hiding this comment

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

where this variable used ?

if (i_fruPath.empty())
{
std::cerr << "Provided empty path,please provide valid EEPROM path"

Choose a reason for hiding this comment

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

This API should just say input is empty.
std::cerr << "Empty input received" << std::endl;

<< std::endl;
return vpd::constants::FAILURE;
}

int l_returnValue;
auto l_bus = sdbusplus::bus::new_default();

auto l_method = l_bus.new_method_call(
constants::vpdManagerService, constants::vpdManagerObjectPath,
constants::vpdManagerInfName, "performVpdSanityCheck");

l_method.append(i_fruPath);
auto l_result = l_bus.call(l_method);
l_result.read(l_returnValue);

if (l_returnValue == vpd::constants::FAILURE)
{
std::cerr
<< "EEPROM sanity check has failed. Check PELs for details"

Choose a reason for hiding this comment

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

are we logging any PEL for this command. ?

<< std::endl;
return vpd::constants::FAILURE;
}

std::cout << "EEPROM sanity checking is Successful." << std::endl;

Choose a reason for hiding this comment

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

"Sanity checking is Successful" is enough.

return vpd::constants::SUCCESS;
}
catch (const std::exception& l_ex)
{
std::cerr

Choose a reason for hiding this comment

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

should it be
std::cerr << "Sanity check failed, reason: " << l_ex.what() << std::endl;

<< "EEPROM sanity check failed. Exception caught for the reason - "
<< std::endl
<< l_ex.what() << std::endl;

return vpd::constants::FAILURE;
}
}

} // namespace vpd
24 changes: 23 additions & 1 deletion vpd-tool/src/vpd_tool_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,9 @@ void updateFooter(CLI::App& i_app)
" From DBus to console in Table format: "
"vpd-tool -i -t\n"
"Force Reset:\n"
" vpd-tool --forceReset\n");
" vpd-tool --forceReset\n"
"Vpd sanity check:\n"
" vpd-tool --sanityCheck -P <EEPROM Path> \n");

Choose a reason for hiding this comment

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

It showed as new option -P, I see Souvik comment for the same.
at line 369, specified as needs l_objectOption.

So it should be
" vpd-tool --sanityCheck -O \n"

}

int main(int argc, char** argv)
Expand Down Expand Up @@ -359,6 +361,13 @@ int main(int argc, char** argv)
"--forceReset, -f, -F",
"Force collect for hardware. CAUTION: Developer only option.");

auto l_sanityCheckFlag =
l_app
.add_flag(
"--sanityCheck",
"Performs sanity check for given EEPROM path. If any redundant EEPROM is found for that FRU in system config json, it will validate that as well.")

Choose a reason for hiding this comment

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

This log will be printed under Options: in help section, we should just say what this command do precisely. Please take a look how other commands are printed when vpd-tool is executed.

Can it be
"Perform Sanity check, \nNote: In case given EEPROM has any redundant path, sanity check on redundant EEPROM will also be performed."

->needs(l_objectOption);

CLI11_PARSE(l_app, argc, argv);

if (checkOptionValuePair(l_objectOption, l_vpdPath, l_recordOption,
Expand Down Expand Up @@ -432,6 +441,19 @@ int main(int argc, char** argv)
return forceReset();
}

if (!l_sanityCheckFlag->empty())
{
if (l_objectOption->empty())

Choose a reason for hiding this comment

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

checkOptionValuePair() is already taking care of input validation.
we don't need this check, remove this.

{
std::cerr << "Please provide EEPROM path.\nUse --object/-O "
"to give EEPROM path. Refer --help."
<< std::endl;
return vpd::constants::FAILURE;
}

vpd::VpdTool l_vpdToolObj;
return l_vpdToolObj.performSanityCheck(l_vpdPath);
}
std::cout << l_app.help() << std::endl;
return vpd::constants::FAILURE;
}