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/authorization.hpp b/include/authorization.hpp
index 24dbb7b..634620b 100644
--- a/include/authorization.hpp
+++ b/include/authorization.hpp
@@ -34,7 +34,7 @@
}
}
-static const std::shared_ptr<persistent_data::UserSession>
+static std::shared_ptr<persistent_data::UserSession>
performBasicAuth(std::string_view auth_header)
{
BMCWEB_LOG_DEBUG << "[AuthMiddleware] Basic authentication";
@@ -79,7 +79,7 @@
isConfigureSelfOnly);
}
-static const std::shared_ptr<persistent_data::UserSession>
+static std::shared_ptr<persistent_data::UserSession>
performTokenAuth(std::string_view auth_header)
{
BMCWEB_LOG_DEBUG << "[AuthMiddleware] Token authentication";
@@ -90,7 +90,7 @@
return session;
}
-static const std::shared_ptr<persistent_data::UserSession>
+static std::shared_ptr<persistent_data::UserSession>
performXtokenAuth(const crow::Request& req)
{
BMCWEB_LOG_DEBUG << "[AuthMiddleware] X-Auth-Token authentication";
@@ -105,7 +105,7 @@
return session;
}
-static const std::shared_ptr<persistent_data::UserSession>
+static std::shared_ptr<persistent_data::UserSession>
performCookieAuth(const crow::Request& req)
{
BMCWEB_LOG_DEBUG << "[AuthMiddleware] Cookie authentication";
@@ -130,7 +130,7 @@
std::string_view authKey =
cookieValue.substr(startIndex, endIndex - startIndex);
- const std::shared_ptr<persistent_data::UserSession> session =
+ std::shared_ptr<persistent_data::UserSession> session =
persistent_data::SessionStore::getInstance().loginSessionByToken(
authKey);
if (session == nullptr)
@@ -163,7 +163,7 @@
}
#ifdef BMCWEB_ENABLE_MUTUAL_TLS_AUTHENTICATION
-static const std::shared_ptr<persistent_data::UserSession>
+static std::shared_ptr<persistent_data::UserSession>
performTLSAuth(const crow::Request& req, Response& res,
std::weak_ptr<persistent_data::UserSession> session)
{
@@ -176,24 +176,19 @@
<< " will be used for this request.";
return sp;
}
- else
+ std::string_view cookieValue = req.getHeaderValue("Cookie");
+ if (cookieValue.empty() ||
+ cookieValue.find("SESSION=") == std::string::npos)
{
- std::string_view cookieValue = req.getHeaderValue("Cookie");
- if (cookieValue.empty() ||
- cookieValue.find("SESSION=") == std::string::npos)
- {
- // TODO: change this to not switch to cookie auth
- res.addHeader(
- "Set-Cookie",
- "XSRF-TOKEN=" + sp->csrfToken +
- "; Secure\r\nSet-Cookie: SESSION=" + sp->sessionToken +
- "; Secure; HttpOnly\r\nSet-Cookie: "
- "IsAuthenticated=true; Secure");
- BMCWEB_LOG_DEBUG
- << " TLS session: " << sp->uniqueId
- << " with cookie will be used for this request.";
- return sp;
- }
+ // TODO: change this to not switch to cookie auth
+ res.addHeader("Set-Cookie", "XSRF-TOKEN=" + sp->csrfToken +
+ "; Secure\r\nSet-Cookie: SESSION=" +
+ sp->sessionToken +
+ "; Secure; HttpOnly\r\nSet-Cookie: "
+ "IsAuthenticated=true; Secure");
+ BMCWEB_LOG_DEBUG << " TLS session: " << sp->uniqueId
+ << " with cookie will be used for this request.";
+ return sp;
}
}
return nullptr;
diff --git a/include/http_utility.hpp b/include/http_utility.hpp
index e0166e3..d282a28 100644
--- a/include/http_utility.hpp
+++ b/include/http_utility.hpp
@@ -19,7 +19,7 @@
{
return true;
}
- else if (encoding == "application/json")
+ if (encoding == "application/json")
{
return false;
}
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,
diff --git a/include/openbmc_dbus_rest.hpp b/include/openbmc_dbus_rest.hpp
index 6bb09d0..2c801c7 100644
--- a/include/openbmc_dbus_rest.hpp
+++ b/include/openbmc_dbus_rest.hpp
@@ -823,7 +823,7 @@
}
const std::string& keyType = codes[0];
const std::string& valueType = codes[1];
- for (auto it : j->items())
+ for (const auto& it : j->items())
{
r = convertJsonToDbus(m, keyType, it.key());
if (r < 0)
@@ -1453,10 +1453,7 @@
}
return;
}
- else
- {
- transaction->methodPassed = true;
- }
+ transaction->methodPassed = true;
handleMethodResponse(transaction, m2,
returnType);
@@ -2146,7 +2143,7 @@
.methods(boost::beast::http::verb::get)([](const crow::Request&,
crow::Response& res,
const std::string& dumpId) {
- std::regex validFilename("^[\\w\\- ]+(\\.?[\\w\\- ]*)$");
+ std::regex validFilename(R"(^[\w\- ]+(\.?[\w\- ]*)$)");
if (!std::regex_match(dumpId, validFilename))
{
res.result(boost::beast::http::status::bad_request);
@@ -2250,7 +2247,7 @@
// causes the normal trailing backslash redirector to
// fail.
}
- else if (!it->empty())
+ if (!it->empty())
{
objectPath += "/" + *it;
}
diff --git a/include/ssl_key_handler.hpp b/include/ssl_key_handler.hpp
index deb3a76..19d3ec7 100644
--- a/include/ssl_key_handler.hpp
+++ b/include/ssl_key_handler.hpp
@@ -32,10 +32,7 @@
{
return true;
}
- else
- {
- return false;
- }
+ return false;
}
inline bool validateCertificate(X509* const cert)
@@ -85,12 +82,9 @@
<< X509_verify_cert_error_string(errCode);
return true;
}
- else
- {
- BMCWEB_LOG_ERROR << "Certificate verification failed. Reason: "
- << X509_verify_cert_error_string(errCode);
- return false;
- }
+ BMCWEB_LOG_ERROR << "Certificate verification failed. Reason: "
+ << X509_verify_cert_error_string(errCode);
+ return false;
}
BMCWEB_LOG_ERROR
diff --git a/include/webassets.hpp b/include/webassets.hpp
index 9944dfd..c2a6851 100644
--- a/include/webassets.hpp
+++ b/include/webassets.hpp
@@ -65,7 +65,7 @@
for (const std::filesystem::directory_entry& dir : paths)
{
- std::filesystem::path absolutePath = dir.path();
+ const std::filesystem::path& absolutePath = dir.path();
std::filesystem::path relativePath{
absolutePath.string().substr(rootpath.string().size() - 1)};
if (std::filesystem::is_directory(dir))