Write the clang-tidy file OpenBMC needs
Now that CI can handle clang-tidy, and a lot of the individual fixes
have landed for the various static analysis checks, lets see how close
we are.
This includes bringing a bunch of the code up to par with the checks
that require. Most of them fall into the category of extraneous else
statements, const correctness problems, or extra copies.
Tested:
CI only. Unit tests pass.
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I9fbd346560a75fdd3901fa40c57932486275e912
diff --git a/include/ibm/locks.hpp b/include/ibm/locks.hpp
index f6719c7..b897fa7 100644
--- a/include/ibm/locks.hpp
+++ b/include/ibm/locks.hpp
@@ -317,20 +317,16 @@
BMCWEB_LOG_DEBUG << "Not a Valid request id";
return std::make_pair(false, status);
}
- else
+ // Validation passed, check if all the locks are owned by the
+ // requesting HMC
+ auto status2 = isItMyLock(p, ids);
+ if (status2.first)
{
- // Validation passed, check if all the locks are owned by the
- // requesting HMC
- auto status = isItMyLock(p, ids);
- if (status.first)
- {
- // The current hmc owns all the locks, so we can release
- // them
- releaseLock(p);
- }
- return std::make_pair(true, status);
+ // The current hmc owns all the locks, so we can release
+ // them
+ releaseLock(p);
}
- return std::make_pair(false, status);
+ return std::make_pair(true, status);
}
inline RcAcquireLock Lock::acquireLock(const LockRequests lockRequestStructure)
@@ -358,19 +354,14 @@
BMCWEB_LOG_DEBUG << "There is a conflict within itself";
return std::make_pair(true, std::make_pair(status, 1));
}
- else
- {
- BMCWEB_LOG_DEBUG << "The request is not conflicting within itself";
+ BMCWEB_LOG_DEBUG << "The request is not conflicting within itself";
- // Need to check for conflict with the locktable entries.
+ // Need to check for conflict with the locktable entries.
- auto conflict = isConflictWithTable(multiRequest);
+ auto conflict = isConflictWithTable(multiRequest);
- BMCWEB_LOG_DEBUG << "Done with checking conflict with the locktable";
- return std::make_pair(false, conflict);
- }
-
- return std::make_pair(true, std::make_pair(true, 1));
+ BMCWEB_LOG_DEBUG << "Done with checking conflict with the locktable";
+ return std::make_pair(false, conflict);
}
inline void Lock::releaseLock(const ListOfTransactionIds& refRids)
@@ -562,43 +553,38 @@
saveLocks();
return std::make_pair(false, transactionId);
}
+ BMCWEB_LOG_DEBUG
+ << "Lock table is not empty, check for conflict with lock table";
+ // Lock table is not empty, compare the lockrequest entries with
+ // the entries in the lock table
- else
+ for (const auto& lockRecord1 : refLockRequestStructure)
{
- BMCWEB_LOG_DEBUG
- << "Lock table is not empty, check for conflict with lock table";
- // Lock table is not empty, compare the lockrequest entries with
- // the entries in the lock table
-
- for (const auto& lockRecord1 : refLockRequestStructure)
+ for (const auto& map : lockTable)
{
- for (const auto& map : lockTable)
+ for (const auto& lockRecord2 : map.second)
{
- for (const auto& lockRecord2 : map.second)
+ bool status = isConflictRecord(lockRecord1, lockRecord2);
+ if (status)
{
- bool status = isConflictRecord(lockRecord1, lockRecord2);
- if (status)
- {
- return std::make_pair(
- true, std::make_pair(map.first, lockRecord2));
- }
+ return std::make_pair(
+ true, std::make_pair(map.first, lockRecord2));
}
}
}
-
- // Reached here, so no conflict with the locktable, so we are safe to
- // add the request records into the lock table
-
- // Lock table is empty, so we are safe to add the lockrecords
- // as there will be no conflict
- BMCWEB_LOG_DEBUG << " Adding elements into lock table";
- transactionId = generateTransactionId();
- lockTable.emplace(
- std::make_pair(transactionId, refLockRequestStructure));
-
- // save the lock in the persistent file
- saveLocks();
}
+
+ // Reached here, so no conflict with the locktable, so we are safe to
+ // add the request records into the lock table
+
+ // Lock table is empty, so we are safe to add the lockrecords
+ // as there will be no conflict
+ BMCWEB_LOG_DEBUG << " Adding elements into lock table";
+ transactionId = generateTransactionId();
+ lockTable.emplace(std::make_pair(transactionId, refLockRequestStructure));
+
+ // save the lock in the persistent file
+ saveLocks();
return std::make_pair(false, transactionId);
}
@@ -615,25 +601,22 @@
return false;
}
- else
+ BMCWEB_LOG_DEBUG
+ << "There are multiple lock requests coming in a single request";
+
+ // There are multiple requests a part of one request
+
+ for (uint32_t i = 0; i < refLockRequestStructure.size(); i++)
{
- BMCWEB_LOG_DEBUG
- << "There are multiple lock requests coming in a single request";
-
- // There are multiple requests a part of one request
-
- for (uint32_t i = 0; i < refLockRequestStructure.size(); i++)
+ for (uint32_t j = i + 1; j < refLockRequestStructure.size(); j++)
{
- for (uint32_t j = i + 1; j < refLockRequestStructure.size(); j++)
- {
- const LockRequest& p = refLockRequestStructure[i];
- const LockRequest& q = refLockRequestStructure[j];
- bool status = isConflictRecord(p, q);
+ const LockRequest& p = refLockRequestStructure[i];
+ const LockRequest& q = refLockRequestStructure[j];
+ bool status = isConflictRecord(p, q);
- if (status)
- {
- return true;
- }
+ if (status)
+ {
+ return true;
}
}
}
@@ -661,11 +644,6 @@
return false;
}
- else
- {
- return true;
- }
-
return true;
}
@@ -681,69 +659,64 @@
return false;
}
- else
+ uint32_t i = 0;
+ for (const auto& p : std::get<4>(refLockRecord1))
{
- uint32_t i = 0;
- for (const auto& p : std::get<4>(refLockRecord1))
+
+ // return conflict when any of them is try to lock all resources
+ // under the current resource level.
+ if (boost::equals(p.first, "LockAll") ||
+ boost::equals(std::get<4>(refLockRecord2)[i].first, "LockAll"))
{
+ BMCWEB_LOG_DEBUG
+ << "Either of the Comparing locks are trying to lock all "
+ "resources under the current resource level";
+ return true;
+ }
- // return conflict when any of them is try to lock all resources
- // under the current resource level.
- if (boost::equals(p.first, "LockAll") ||
- boost::equals(std::get<4>(refLockRecord2)[i].first, "LockAll"))
+ // determine if there is a lock-all-with-same-segment-size.
+ // If the current segment sizes are the same,then we should fail.
+
+ if ((boost::equals(p.first, "LockSame") ||
+ boost::equals(std::get<4>(refLockRecord2)[i].first, "LockSame")) &&
+ (p.second == std::get<4>(refLockRecord2)[i].second))
+ {
+ return true;
+ }
+
+ // if segment lengths are not the same, it means two different locks
+ // So no conflict
+ if (p.second != std::get<4>(refLockRecord2)[i].second)
+ {
+ BMCWEB_LOG_DEBUG << "Segment lengths are not same";
+ BMCWEB_LOG_DEBUG << "Segment 1 length : " << p.second;
+ BMCWEB_LOG_DEBUG << "Segment 2 length : "
+ << std::get<4>(refLockRecord2)[i].second;
+ return false;
+ }
+
+ // compare segment data
+
+ for (uint32_t i = 0; i < p.second; i++)
+ {
+ // if the segment data is different , then the locks is on a
+ // different resource So no conflict between the lock
+ // records.
+ // BMC is little endian , but the resourceID is formed by
+ // the Management Console in such a way that, the first byte
+ // from the MSB Position corresponds to the First Segment
+ // data. Therefore we need to convert the in-comming
+ // resourceID into Big Endian before processing further.
+ if (!(checkByte(
+ boost::endian::endian_reverse(std::get<3>(refLockRecord1)),
+ boost::endian::endian_reverse(std::get<3>(refLockRecord2)),
+ i)))
{
- BMCWEB_LOG_DEBUG
- << "Either of the Comparing locks are trying to lock all "
- "resources under the current resource level";
- return true;
- }
-
- // determine if there is a lock-all-with-same-segment-size.
- // If the current segment sizes are the same,then we should fail.
-
- if ((boost::equals(p.first, "LockSame") ||
- boost::equals(std::get<4>(refLockRecord2)[i].first,
- "LockSame")) &&
- (p.second == std::get<4>(refLockRecord2)[i].second))
- {
- return true;
- }
-
- // if segment lengths are not the same, it means two different locks
- // So no conflict
- if (p.second != std::get<4>(refLockRecord2)[i].second)
- {
- BMCWEB_LOG_DEBUG << "Segment lengths are not same";
- BMCWEB_LOG_DEBUG << "Segment 1 length : " << p.second;
- BMCWEB_LOG_DEBUG << "Segment 2 length : "
- << std::get<4>(refLockRecord2)[i].second;
return false;
}
-
- // compare segment data
-
- for (uint32_t i = 0; i < p.second; i++)
- {
- // if the segment data is different , then the locks is on a
- // different resource So no conflict between the lock
- // records.
- // BMC is little endian , but the resourceID is formed by
- // the Management Console in such a way that, the first byte
- // from the MSB Position corresponds to the First Segment
- // data. Therefore we need to convert the in-comming
- // resourceID into Big Endian before processing further.
- if (!(checkByte(boost::endian::endian_reverse(
- std::get<3>(refLockRecord1)),
- boost::endian::endian_reverse(
- std::get<3>(refLockRecord2)),
- i)))
- {
- return false;
- }
- }
-
- ++i;
}
+
+ ++i;
}
return false;
diff --git a/include/ibm/management_console_rest.hpp b/include/ibm/management_console_rest.hpp
index 2c45693..b0e57ed 100644
--- a/include/ibm/management_console_rest.hpp
+++ b/include/ibm/management_console_rest.hpp
@@ -91,10 +91,7 @@
res.jsonValue["Description"] = contentNotAcceptableMsg;
return;
}
- else
- {
- BMCWEB_LOG_DEBUG << "Not a multipart/form-data. Continue..";
- }
+ BMCWEB_LOG_DEBUG << "Not a multipart/form-data. Continue..";
BMCWEB_LOG_DEBUG
<< "handleIbmPut: Request to create/update the save-area file";
@@ -109,7 +106,7 @@
std::filesystem::path loc("/var/lib/obmc/bmc-console-mgmt/save-area");
loc /= fileID;
- std::string data = std::move(req.body);
+ const std::string& data = req.body;
BMCWEB_LOG_DEBUG << "data capaticty : " << data.capacity();
if (data.capacity() > maxSaveareaFileSize)
{
@@ -133,27 +130,24 @@
res.jsonValue["Description"] = "Error while creating the file";
return;
}
+ file << data;
+ std::string origin = "/ibm/v1/Host/ConfigFiles/" + fileID;
+ // Push an event
+ if (fileExists)
+ {
+ BMCWEB_LOG_DEBUG << "config file is updated";
+ res.jsonValue["Description"] = "File Updated";
+
+ redfish::EventServiceManager::getInstance().sendEvent(
+ redfish::messages::resourceChanged(), origin, "IBMConfigFile");
+ }
else
{
- file << data;
- std::string origin = "/ibm/v1/Host/ConfigFiles/" + fileID;
- // Push an event
- if (fileExists)
- {
- BMCWEB_LOG_DEBUG << "config file is updated";
- res.jsonValue["Description"] = "File Updated";
+ BMCWEB_LOG_DEBUG << "config file is created";
+ res.jsonValue["Description"] = "File Created";
- redfish::EventServiceManager::getInstance().sendEvent(
- redfish::messages::resourceChanged(), origin, "IBMConfigFile");
- }
- else
- {
- BMCWEB_LOG_DEBUG << "config file is created";
- res.jsonValue["Description"] = "File Created";
-
- redfish::EventServiceManager::getInstance().sendEvent(
- redfish::messages::resourceCreated(), origin, "IBMConfigFile");
- }
+ redfish::EventServiceManager::getInstance().sendEvent(
+ redfish::messages::resourceCreated(), origin, "IBMConfigFile");
}
}
@@ -165,7 +159,7 @@
{
for (const auto& file : std::filesystem::directory_iterator(loc))
{
- std::filesystem::path pathObj(file.path());
+ const std::filesystem::path& pathObj = file.path();
pathObjList.push_back("/ibm/v1/Host/ConfigFiles/" +
pathObj.filename().string());
}
@@ -257,6 +251,7 @@
std::ifstream fileOpen(filePath.c_str());
if (static_cast<bool>(fileOpen))
+ {
if (remove(filePath.c_str()) == 0)
{
BMCWEB_LOG_DEBUG << "File removed!\n";
@@ -268,6 +263,7 @@
res.result(boost::beast::http::status::internal_server_error);
res.jsonValue["Description"] = internalServerError;
}
+ }
else
{
BMCWEB_LOG_ERROR << "File not found!\n";
@@ -429,33 +425,30 @@
res.end();
return;
}
- else
+ BMCWEB_LOG_DEBUG << "There is a conflict with the lock table";
+ res.result(boost::beast::http::status::conflict);
+ auto var =
+ std::get<std::pair<uint32_t, LockRequest>>(conflictStatus.second);
+ nlohmann::json returnJson, segments;
+ nlohmann::json myarray = nlohmann::json::array();
+ returnJson["TransactionID"] = var.first;
+ returnJson["SessionID"] = std::get<0>(var.second);
+ returnJson["HMCID"] = std::get<1>(var.second);
+ returnJson["LockType"] = std::get<2>(var.second);
+ returnJson["ResourceID"] = std::get<3>(var.second);
+
+ for (auto& i : std::get<4>(var.second))
{
- BMCWEB_LOG_DEBUG << "There is a conflict with the lock table";
- res.result(boost::beast::http::status::conflict);
- auto var = std::get<std::pair<uint32_t, LockRequest>>(
- conflictStatus.second);
- nlohmann::json returnJson, segments;
- nlohmann::json myarray = nlohmann::json::array();
- returnJson["TransactionID"] = var.first;
- returnJson["SessionID"] = std::get<0>(var.second);
- returnJson["HMCID"] = std::get<1>(var.second);
- returnJson["LockType"] = std::get<2>(var.second);
- returnJson["ResourceID"] = std::get<3>(var.second);
-
- for (auto& i : std::get<4>(var.second))
- {
- segments["LockFlag"] = i.first;
- segments["SegmentLength"] = i.second;
- myarray.push_back(segments);
- }
-
- returnJson["SegmentFlags"] = myarray;
-
- res.jsonValue["Record"] = returnJson;
- res.end();
- return;
+ segments["LockFlag"] = i.first;
+ segments["SegmentLength"] = i.second;
+ myarray.push_back(segments);
}
+
+ returnJson["SegmentFlags"] = myarray;
+
+ res.jsonValue["Record"] = returnJson;
+ res.end();
+ return;
}
}
inline void handleRelaseAllAPI(const crow::Request& req, crow::Response& res)
@@ -490,47 +483,41 @@
res.end();
return;
}
- else
+ auto statusRelease =
+ std::get<crow::ibm_mc_lock::RcRelaseLock>(varReleaselock.second);
+ if (statusRelease.first)
{
- auto statusRelease =
- std::get<crow::ibm_mc_lock::RcRelaseLock>(varReleaselock.second);
- if (statusRelease.first)
- {
- // The current hmc owns all the locks, so we already released
- // them
- res.result(boost::beast::http::status::ok);
- res.end();
- return;
- }
-
- else
- {
- // valid rid, but the current hmc does not own all the locks
- BMCWEB_LOG_DEBUG << "Current HMC does not own all the locks";
- res.result(boost::beast::http::status::unauthorized);
-
- auto var = statusRelease.second;
- nlohmann::json returnJson, segments;
- nlohmann::json myArray = nlohmann::json::array();
- returnJson["TransactionID"] = var.first;
- returnJson["SessionID"] = std::get<0>(var.second);
- returnJson["HMCID"] = std::get<1>(var.second);
- returnJson["LockType"] = std::get<2>(var.second);
- returnJson["ResourceID"] = std::get<3>(var.second);
-
- for (auto& i : std::get<4>(var.second))
- {
- segments["LockFlag"] = i.first;
- segments["SegmentLength"] = i.second;
- myArray.push_back(segments);
- }
-
- returnJson["SegmentFlags"] = myArray;
- res.jsonValue["Record"] = returnJson;
- res.end();
- return;
- }
+ // The current hmc owns all the locks, so we already released
+ // them
+ res.result(boost::beast::http::status::ok);
+ res.end();
+ return;
}
+
+ // valid rid, but the current hmc does not own all the locks
+ BMCWEB_LOG_DEBUG << "Current HMC does not own all the locks";
+ res.result(boost::beast::http::status::unauthorized);
+
+ auto var = statusRelease.second;
+ nlohmann::json returnJson, segments;
+ nlohmann::json myArray = nlohmann::json::array();
+ returnJson["TransactionID"] = var.first;
+ returnJson["SessionID"] = std::get<0>(var.second);
+ returnJson["HMCID"] = std::get<1>(var.second);
+ returnJson["LockType"] = std::get<2>(var.second);
+ returnJson["ResourceID"] = std::get<3>(var.second);
+
+ for (auto& i : std::get<4>(var.second))
+ {
+ segments["LockFlag"] = i.first;
+ segments["SegmentLength"] = i.second;
+ myArray.push_back(segments);
+ }
+
+ returnJson["SegmentFlags"] = myArray;
+ res.jsonValue["Record"] = returnJson;
+ res.end();
+ return;
}
inline void handleGetLockListAPI(crow::Response& res,