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.

Sign up for GitHub

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

vpd-tool: Refactoring more option in fixSystemVPD #591

Open
wants to merge 1 commit into
base: 1110
Choose a base branch
from
Open

vpd-tool: Refactoring more option in fixSystemVPD #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions vpd-tool/include/tool_constants.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,7 @@ constexpr auto assetInf = "xyz.openbmc_project.Inventory.Decorator.Asset";
constexpr auto objectMapperService = "xyz.openbmc_project.ObjectMapper";
constexpr auto objectMapperObjectPath = "/xyz/openbmc_project/object_mapper";
constexpr auto objectMapperInfName = "xyz.openbmc_project.ObjectMapper";

static constexpr auto OUTLINE_COUNT = 191;
} // namespace constants
} // namespace vpd
228 changes: 124 additions & 104 deletions vpd-tool/src/vpd_tool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ int VpdTool::fixSystemVpd() const noexcept
int l_userSelectedOption = types::UserOption::Exit;
std::cin >> l_userSelectedOption;

std::cout << std::endl << std::string(191, '=') << std::endl;
std::cout << std::endl;

if (types::UserOption::UseBackupDataForAll == l_userSelectedOption)
{
Expand All @@ -286,6 +286,7 @@ int VpdTool::fixSystemVpd() const noexcept
else if (types::UserOption::Exit == l_userSelectedOption)
{
std::cout << "Exit successfully" << std::endl;
l_rc = constants::SUCCESS;
break;
}
else
Expand Down Expand Up @@ -827,7 +828,7 @@ void VpdTool::printSystemVpd(
std::cerr << "Invalid JSON to print system VPD" << std::endl;
}

std::string l_outline(191, '=');
std::string l_outline(constants::OUTLINE_COUNT, '=');
std::cout << "\nRestorable record-keyword pairs and their data on backup & "
"primary.\n\n"
<< l_outline << std::endl;
Expand All @@ -843,9 +844,9 @@ void VpdTool::printSystemVpd(

for (const auto& l_aRecordKwInfo : i_parsedJsonObj["backupMap"])
{
if (l_aRecordKwInfo.contains("sourceRecord") ||
l_aRecordKwInfo.contains("sourceKeyword") ||
l_aRecordKwInfo.contains("destinationkeywordValue") ||
if (l_aRecordKwInfo.contains("sourceRecord") &&
l_aRecordKwInfo.contains("sourceKeyword") &&
l_aRecordKwInfo.contains("destinationkeywordValue") &&
l_aRecordKwInfo.contains("sourcekeywordValue"))
{
std::string l_mismatchFound{
Expand All @@ -854,7 +855,7 @@ void VpdTool::printSystemVpd(
? "YES"
: "NO"};

std::string l_splitLine(191, '-');
std::string l_splitLine(constants::OUTLINE_COUNT, '-');

try
{
Expand Down Expand Up @@ -929,6 +930,7 @@ int VpdTool::updateAllKeywords(const nlohmann::json& i_parsedJsonObj,
continue;
}

l_rc = constants::FAILURE;
if (l_aRecordKwInfo["sourcekeywordValue"] !=
l_aRecordKwInfo["destinationkeywordValue"])
{
Expand Down Expand Up @@ -971,6 +973,7 @@ int VpdTool::updateAllKeywords(const nlohmann::json& i_parsedJsonObj,
}
else
{
l_rc = constants::SUCCESS;
std::cout << "No mismatch found for any of the above mentioned "
"record-keyword pair. Exit successfully."
<< std::endl;
Expand Down Expand Up @@ -1011,38 +1014,15 @@ int VpdTool::handleMoreOption(
"source path information is missing in JSON");
}

auto updateKeywordValue =
[](std::string io_vpdPath, const std::string& i_recordName,
const std::string& i_keywordName,
const types::BinaryVector& i_keywordValue) -> int {
int l_rc = constants::FAILURE;

try
{
auto l_paramsToWrite = std::make_tuple(
i_recordName, i_keywordName, i_keywordValue);
l_rc = utils::writeKeyword(io_vpdPath, l_paramsToWrite);
uint8_t l_slNum = 0;

if (l_rc > 0)
{
std::cout << std::endl
<< "Data updated successfully." << std::endl;
}
}
catch (const std::exception& l_ex)
{
// TODO: Enable log when verbose is enabled.
std::cerr << l_ex.what() << std::endl;
}
return l_rc;
};

do
for (const auto& l_aRecordKwInfo : i_parsedJsonObj["backupMap"])
{
int l_slNum = 0;
bool l_exit = false;
l_rc = constants::FAILURE;
++l_slNum;

for (const auto& l_aRecordKwInfo : i_parsedJsonObj["backupMap"])
uint8_t l_runOnce = 1;
while (l_runOnce--)
{
if (!l_aRecordKwInfo.contains("sourceRecord") ||
!l_aRecordKwInfo.contains("sourceKeyword") ||
Expand All @@ -1053,7 +1033,7 @@ int VpdTool::handleMoreOption(
std::cerr
<< "Source or destination information is missing in the JSON."
<< std::endl;
continue;
break;
}

const std::string l_mismatchFound{
Expand All @@ -1063,7 +1043,10 @@ int VpdTool::handleMoreOption(
: "NO"};

std::cout << std::endl
<< std::left << std::setw(6) << "S.No" << std::left
<< std::string(constants::OUTLINE_COUNT, '=')
<< std::endl;

std::cout << std::left << std::setw(6) << "S.No" << std::left
<< std::setw(8) << "Record" << std::left
<< std::setw(9) << "Keyword" << std::left
<< std::setw(75) << std::setfill(' ') << "Backup Data"
Expand All @@ -1072,7 +1055,7 @@ int VpdTool::handleMoreOption(
<< "Data Mismatch" << std::endl;

std::cout << std::left << std::setw(6)
<< static_cast<int>(++l_slNum) << std::left
<< static_cast<int>(l_slNum) << std::left
Copy link

Choose a reason for hiding this comment

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

Is the static_cast<>() needed? l_slNum is an int?

Copy link
Author

Choose a reason for hiding this comment

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

yes needed, along with std::setw(), passing normal number after that is not working.

Copy link

Choose a reason for hiding this comment

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

If the intention of this code block is to print data in tabular format, I suggest using class Table in vpd_utils.hpp

Copy link
Author

Choose a reason for hiding this comment

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

We can update that as another PR.

<< std::setw(8)
<< l_aRecordKwInfo.value("sourceRecord", "")
<< std::left << std::setw(9)
Expand All @@ -1086,7 +1069,8 @@ int VpdTool::handleMoreOption(
<< std::left << std::setw(14) << l_mismatchFound
<< std::endl;

std::cout << std::string(191, '=') << std::endl;
std::cout << std::string(constants::OUTLINE_COUNT, '=')
<< std::endl;

if (constants::STR_CMP_SUCCESS ==
l_mismatchFound.compare("YES"))
Expand All @@ -1101,6 +1085,7 @@ int VpdTool::handleMoreOption(
}
else
{
std::cout << "No mismatch found." << std::endl << std::endl;
printFixSystemVpdOption(types::UserOption::NewValueOnBoth);
printFixSystemVpdOption(types::UserOption::SkipCurrent);
printFixSystemVpdOption(types::UserOption::Exit);
Expand All @@ -1109,74 +1094,109 @@ int VpdTool::handleMoreOption(
int l_userSelectedOption = types::UserOption::Exit;
std::cin >> l_userSelectedOption;

if (types::UserOption::UseBackupDataForCurrent ==
l_userSelectedOption)
{
l_rc = updateKeywordValue(
l_srcVpdPath, l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
l_aRecordKwInfo["destinationkeywordValue"]);
}
else if (types::UserOption::UseSystemBackplaneDataForCurrent ==
l_userSelectedOption)
{
l_rc = updateKeywordValue(
l_srcVpdPath, l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
l_aRecordKwInfo["sourcekeywordValue"]);
}
else if (types::UserOption::NewValueOnBoth ==
l_userSelectedOption)
switch (l_userSelectedOption)
{
std::string l_newValue;
std::cout
<< std::endl
<< "Enter the new value to update on both "
"primary & backup. Value should be in ASCII or "
"in HEX(prefixed with 0x) : ";
std::cin >> l_newValue;
std::cout << std::endl
<< std::string(191, '=') << std::endl;

try
{
l_rc = updateKeywordValue(
l_srcVpdPath, l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
utils::convertToBinary(l_newValue));
}
catch (const std::exception& l_ex)
case types::UserOption::UseBackupDataForCurrent:
try
Copy link

@souvik1914581 souvik1914581 Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The following block is being used in multiple places:

try{ utils::writeKeyword(); } catch() { }

This block can be put inside a method or a lambda to make the code more readable.

Copy link
Author

@branupama branupama Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It was there in lambda function(line 1014), I removed that added in place.
Bcz, it will be useful only for UseBackupDataForCurrent & UseSystemBackplaneDataForCurrent case.
for case types::UserOption::NewValueOnBoth: need to add another try&catch block as it need to call utils::convertToBinary which can throw exception.

{
l_rc = utils::writeKeyword(
l_srcVpdPath,
std::make_tuple(
l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
l_aRecordKwInfo
["destinationkeywordValue"]));

if (l_rc > 0)
{
l_rc = constants::SUCCESS;
std::cout << std::endl
<< "Data updated successfully."
<< std::endl;
}
}
catch (const std::exception& l_ex)
{
// TODO: Enable log when verbose is enabled.
std::cerr << std::endl << l_ex.what() << std::endl;
}
break;
case types::UserOption::UseSystemBackplaneDataForCurrent:
try
{
l_rc = utils::writeKeyword(
l_srcVpdPath,
std::make_tuple(
l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
l_aRecordKwInfo["sourcekeywordValue"]));

if (l_rc > 0)
{
l_rc = constants::SUCCESS;
std::cout << std::endl
<< "Data updated successfully."
<< std::endl;
}
}
catch (const std::exception& l_ex)
{
// TODO: Enable log when verbose is enabled.
std::cerr << std::endl << l_ex.what() << std::endl;
}
break;
case types::UserOption::NewValueOnBoth:
{
// TODO: Enable logging when verbose is enabled.
std::cerr << l_ex.what() << std::endl;
std::string l_newValue;
std::cout
<< std::endl
<< "Enter the new value to update on both "
"primary & backup. Value should be in ASCII or "
"in HEX(prefixed with 0x) : ";
std::cin >> l_newValue;

try
{
l_rc = utils::writeKeyword(
l_srcVpdPath,
std::make_tuple(
l_aRecordKwInfo["sourceRecord"],
l_aRecordKwInfo["sourceKeyword"],
utils::convertToBinary(l_newValue)));

if (l_rc > 0)
{
l_rc = constants::SUCCESS;
std::cout << std::endl
<< "Data updated successfully."
<< std::endl;
}
}
catch (const std::exception& l_ex)
{
// TODO: Enable logging when verbose is enabled.
std::cerr << std::endl << l_ex.what() << std::endl;
}
break;
}
}
else if (types::UserOption::SkipCurrent == l_userSelectedOption)
{
std::cout << std::endl
<< "Skipped the above record-keyword pair. "
"Continue to the next available pair."
<< std::endl;
}
else if (types::UserOption::Exit == l_userSelectedOption)
{
std::cout << "Exit successfully" << std::endl;
l_exit = true;
break;
}
else
{
std::cout << "Provide a valid option. Retrying for the "
"current record-keyword pair"
<< std::endl;
}
}
if (l_exit)
{
l_rc = constants::SUCCESS;
break;
}
} while (true);
case types::UserOption::SkipCurrent:
l_rc = constants::SUCCESS;
std::cout << std::endl
<< "Skipped the above record-keyword pair. "
"Continue to the next available pair."
<< std::endl;
break;
case types::UserOption::Exit:
std::cout << "Exit successfully" << std::endl;
return constants::SUCCESS;
default:
l_runOnce = 1;
Copy link

Choose a reason for hiding this comment

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

how long can we allow the user to provide invalid option?
can we have a count?

Copy link
Author

Choose a reason for hiding this comment

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

there is no count for how long user can give invalid option.
user can choose 0 to exit from the application

std::cout << "Provide a valid option. Retrying for the "
"current record-keyword pair"
<< std::endl;
} // end switch
} // end while
} // end for
}
catch (const std::exception& l_ex)
{
Expand Down