Fix compiler warnings now that we are using C++17

Upgrading the target language version also results in more clang-tidy
warnings. Fix up many of these, and disable the rest.

I also created #117 to discuss a
more robust approach to putting clang-tidy in the build.

.clang-tidy

@@ -1,18 +1,24 @@
 ---
 Checks:          >-
   boost-*,
-  bugprone-*,
-  cert-*,-cert-err58-cpp,-cert-env33-c,-cert-err60-cpp,
+  bugprone-*,-bugprone-easily-swappable-parameters,
+  cert-*,-cert-err58-cpp,-cert-env33-c,-cert-err60-cpp,-cert-err33-c,
   clang-analyzer-*,
   cppcoreguidelines-*,-cppcoreguidelines-avoid-magic-numbers,-cppcoreguidelines-init-variables,-cppcoreguidelines-owning-memory,-cppcoreguidelines-non-private-member-variables-in-classes,-cppcoreguidelines-macro-usage,
   cppcoreguidelines-pro-*,-cppcoreguidelines-pro-type-reinterpret-cast,-cppcoreguidelines-pro-type-vararg,-cppcoreguidelines-pro-type-const-cast,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,
+  -cppcoreguidelines-prefer-member-initialize,-cppcoreguidelines-avoid-const-or-ref-data-members,
+  -cppcoreguidelines-avoid-do-while,
   google-*,-google-runtime-references,-google-readability-todo,
+  -google-readability-casting,
   hicpp-*,-hicpp-no-array-decay,-hicpp-no-assembler,-hicpp-signed-bitwise,-hicpp-vararg,
   llvm-namespace-comment,
-  misc-*,-misc-non-private-member-variables-in-classes,
+  misc-*,-misc-non-private-member-variables-in-classes,-misc-const-correctness,
+  -misc-use-anonymous-namespace,
   modernize-*,-modernize-avoid-bind,-modernize-loop-convert,-modernize-use-trailing-return-type,
+  -modernize-use-nodiscard,-modernize-return-braced-init-list,-cppcoreguidelines-prefer-member-initializer,
   performance-*,
   readability-*,-readability-else-after-return,-readability-identifier-naming,-readability-magic-numbers,-readability-function-cognitive-complexity,
+  -readability-identifier-length,-readability-container-data-pointer
 
 WarningsAsErrors: '*'
 AnalyzeTemporaryDtors: false

CMakeLists.txt

@@ -167,8 +167,9 @@ SET(CMAKE_EXE_LINKER_FLAGS_TSAN "-O1 -g -fsanitize=thread -fno-omit-frame-pointe
 
 if (CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
     add_definitions(-fstack-protector-all)
-    # Enable maximum set of warnings.
-    add_definitions(-Wall -Wextra -Wformat-security -Wfloat-equal -Wcast-qual -Wswitch-default -Wconversion)
+    # Enable maximum set of warnings
+    # TODO Add [[nodiscard]] everywhere and remove this
+    add_definitions(-Wall -Wextra -Wformat-security -Wfloat-equal -Wcast-qual -Wswitch-default -Wconversion -Wno-modernize-use-nodiscard)
     # only for CXX, gcc doesn't like that when building C...
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wnon-virtual-dtor")
     if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")
@@ -392,7 +393,7 @@ execute_process(COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/scripts/amalgamate-jsoncpp.s
 include_directories(${PROJECT_BINARY_DIR}/jsoncpp)
 # jsoncpp triggers a number of warnings that are turned on by default in our build
 set_source_files_properties(${PROJECT_BINARY_DIR}/jsoncpp/jsoncpp.cc PROPERTIES
-    COMPILE_FLAGS "-Wno-error -Wno-float-equal -Wno-switch-default -Wno-deprecated-declarations")
+    COMPILE_FLAGS "-Wno-error -Wno-float-equal -Wno-switch-default -Wno-deprecated-declarations -Wno-unneeded-internal-declaration")
 add_library(jsoncpp OBJECT ${PROJECT_BINARY_DIR}/jsoncpp/jsoncpp.cc)
 
 add_subdirectory("config")

include/libaktualizr/campaign.h

@@ -11,7 +11,7 @@ class HttpInterface;
 
 namespace campaign {
 
-constexpr int64_t kMaxCampaignsMetaSize = 1024 * 1024;
+constexpr int64_t kMaxCampaignsMetaSize = 1024L * 1024;
 
 class CampaignParseError : std::exception {
  public:

include/libaktualizr/secondary_provider.h

@@ -26,9 +26,10 @@ class SecondaryProvider {
                     std::shared_ptr<const PackageManagerInterface> package_manager_in)
       : config_(config_in), storage_(std::move(storage_in)), package_manager_(std::move(package_manager_in)) {}
 
+  // NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members)
   Config& config_;
-  const std::shared_ptr<const INvStorage> storage_;
-  const std::shared_ptr<const PackageManagerInterface> package_manager_;
+  std::shared_ptr<const INvStorage> storage_;
+  std::shared_ptr<const PackageManagerInterface> package_manager_;
 };
 
 #endif  // UPTANE_SECONDARY_PROVIDER_H

scripts/clang-tidy-wrapper.sh

@@ -13,4 +13,7 @@ if [[ ! -e "${CMAKE_BINARY_DIR}/compile_commands.json" ]]; then
 fi
 
 ${CLANG_TIDY} -quiet -header-filter="\(\(${CMAKE_SOURCE_DIR}|\\.\\.\)/src/|include/libaktualizr/\).*" \
-  --extra-arg-before=-Wno-unknown-warning-option -format-style=file -p "${CMAKE_BINARY_DIR}" "${FILE}"
+  --extra-arg-before=-Wno-unknown-warning-option \
+  --extra-arg-before=-Wno-warnings-as-errors \
+  --extra-arg=-Wno-deprecated-declarations \
+  --format-style=file -p "${CMAKE_BINARY_DIR}" "${FILE}"

src/aktualizr_primary/secondary_config.cc

@@ -11,8 +11,6 @@
 
 namespace Primary {
 
-constexpr const char* const IPSecondariesConfig::Type;
-
 SecondaryConfigParser::Configs SecondaryConfigParser::parse_config_file(const boost::filesystem::path& config_file) {
   if (!boost::filesystem::exists(config_file)) {
     throw std::invalid_argument("Specified config file doesn't exist: " + config_file.string());

src/libaktualizr-posix/asn1/asn1-cerstream.h

@@ -12,7 +12,7 @@ namespace asn1 {
 class Token {
  public:
   enum TokType { seq_tok, endseq_tok, restseq_tok, expl_tok, peekexpl_tok, endexpl_tok, opt_tok, endopt_tok };
-  explicit Token(TokType t) { type = t; }
+  explicit Token(TokType t) : type{t} {}
   virtual ~Token() = default;
   Token(const Token&) = default;
   Token(Token&&) = default;

src/libaktualizr/bootloader/bootloader.cc

@@ -13,7 +13,7 @@
 
 Bootloader::Bootloader(BootloaderConfig config, INvStorage& storage) : config_(std::move(config)), storage_(storage) {
   reboot_sentinel_ = config_.reboot_sentinel_dir / config_.reboot_sentinel_name;
-  reboot_command_ = config_.reboot_command;
+  reboot_command_ = config_.reboot_command;  // NOLINT
 
   if (!Utils::createSecureDirectory(config_.reboot_sentinel_dir)) {
     LOG_WARNING << "Could not create " << config_.reboot_sentinel_dir << " securely, reboot detection support disabled";

src/libaktualizr/bootloader/bootloader.h

@@ -30,9 +30,13 @@ class Bootloader {
   void reboot(bool fake = false);
 
  protected:
+  // TODO Fix this
+  // NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members)
   const BootloaderConfig config_;
 
  private:
+  // TODO Fix this
+  // NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members)
   INvStorage& storage_;
   boost::filesystem::path reboot_sentinel_;
   std::string reboot_command_;

src/libaktualizr/campaign/campaign.cc

@@ -59,7 +59,7 @@ void Campaign::getJson(Json::Value &out) const {
 
   out["id"] = id;
   out["name"] = name;
-  out["size"] = Json::UInt(size);
+  out["size"] = static_cast<Json::UInt>(size);
   out["autoAccept"] = autoAccept;
 
   out["metadata"][0]["type"] = "DESCRIPTION";
@@ -89,7 +89,7 @@ std::vector<Campaign> Campaign::campaignsFromJson(const Json::Value &json) {
 
   for (const auto &c : campaigns_array) {
     try {
-      campaigns.emplace_back(Campaign(c));
+      campaigns.emplace_back(c);
     } catch (const CampaignParseError &exc) {
       LOG_ERROR << "Error parsing " << c << ": " << exc.what();
     }

src/libaktualizr/crypto/CMakeLists.txt

@@ -7,6 +7,12 @@ set(HEADERS crypto.h
 
 set_source_files_properties(p11engine.cc PROPERTIES COMPILE_FLAGS -Wno-deprecated-declarations)
 
+# OpenSSL 3 deprecated several APIs that we use. Unfortunately there
+# doesn't appear to be a solution to PKCS11 support that is available
+# in OpenSSL 1.1 and not deprecated. For more discussion, see:
+# https://github.com/uptane/aktualizr/issues/83
+set_source_files_properties(crypto.cc PROPERTIES COMPILE_FLAGS -Wno-deprecated-declarations)
+
 add_library(crypto OBJECT ${SOURCES})
 aktualizr_source_file_checks(${SOURCES} ${HEADERS})
 

src/libaktualizr/crypto/crypto.cc

@@ -18,9 +18,12 @@
 #include "openssl_compat.h"
 #include "utilities/utils.h"
 
-PublicKey::PublicKey(const boost::filesystem::path &path) : value_(Utils::readFile(path)) {
-  type_ = Crypto::IdentifyRSAKeyType(value_);
-}
+#if !AKTUALIZR_OPENSSL_PRE_3
+#include <openssl/provider.h>
+#endif
+
+PublicKey::PublicKey(const boost::filesystem::path &path)
+    : value_(Utils::readFile(path)), type_(Crypto::IdentifyRSAKeyType(value_)) {}
 
 PublicKey::PublicKey(const Json::Value &uptane_json) {
   std::string keytype;
@@ -345,7 +348,7 @@ std::string Crypto::extractSubjectCN(const std::string &cert) {
   if (len < 0) {
     throw std::runtime_error("Could not get CN from certificate");
   }
-  boost::scoped_array<char> buf(new char[len + 1]);
+  boost::scoped_array<char> buf(new char[len + 1]);  // NOLINT
   X509_NAME_get_text_by_NID(X509_get_subject_name(x.get()), NID_commonName, buf.get(), len + 1);
   return std::string(buf.get());
 }
@@ -759,3 +762,22 @@ std::ostream &operator<<(std::ostream &os, const Hash &h) {
   os << "Hash: " << h.hash_;
   return os;
 }
+
+class CryptoOpenSSlInit {
+ public:
+  // NOLINTNEXTLINE(hicpp-use-equals-default,modernize-use-equals-default)
+  CryptoOpenSSlInit() {
+#if !AKTUALIZR_OPENSSL_PRE_3
+    OSSL_PROVIDER *legacy = OSSL_PROVIDER_try_load(nullptr, "legacy", 1);
+    if (legacy == nullptr) {
+      std::cout << "Warning: could not load 'legacy' OpenSSL provider";
+    }
+    OSSL_PROVIDER *default_p = OSSL_PROVIDER_try_load(nullptr, "default", 1);
+    if (default_p == nullptr) {
+      std::cout << "Warning: could not load 'default' OpenSSL provider";
+    }
+#endif
+  }
+};
+
+CryptoOpenSSlInit const CryptoIniter{};

src/libaktualizr/crypto/openssl_compat.h

@@ -11,4 +11,5 @@
 
 #define AKTUALIZR_OPENSSL_AFTER_11 (!AKTUALIZR_OPENSSL_PRE_11)
 
+#define AKTUALIZR_OPENSSL_PRE_3 (OPENSSL_VERSION_NUMBER < 0x30000000)
 #endif  // AKTUALIZR_OPENSSL_COMPAT_H

src/libaktualizr/crypto/p11engine.cc

@@ -78,7 +78,7 @@ P11Engine::P11Engine(boost::filesystem::path module_path, std::string pass)
   LOG_DEBUG << "Slot token model.......: " << slot->token->model;
   LOG_DEBUG << "Slot token serialnr....: " << slot->token->serialnr;
 
-  uri_prefix_ = std::string("pkcs11:serial=") + slot->token->serialnr + ";pin-value=" + pass + ";id=%";
+  uri_prefix_ = std::string("pkcs11:serial=") + slot->token->serialnr + ";pin-value=" + pass_ + ";id=%";
 
   ENGINE_load_builtin_engines();
   ENGINE* engine = ENGINE_by_id("dynamic");

src/libaktualizr/http/httpinterface.h

@@ -55,8 +55,8 @@ class HttpInterface {
   virtual void setCerts(const std::string &ca, CryptoSource ca_source, const std::string &cert,
                         CryptoSource cert_source, const std::string &pkey, CryptoSource pkey_source) = 0;
   static constexpr int64_t kNoLimit = 0;  // no limit the size of downloaded data
-  static constexpr int64_t kPostRespLimit = 64 * 1024;
-  static constexpr int64_t kPutRespLimit = 64 * 1024;
+  static constexpr int64_t kPostRespLimit = 64L * 1024;
+  static constexpr int64_t kPutRespLimit = 64L * 1024;
 
  protected:
   HttpInterface(const HttpInterface &) = default;

src/libaktualizr/package_manager/ostreemanager.h

@@ -29,12 +29,11 @@ struct PullMetaStruct {
   PullMetaStruct(Uptane::Target target_in, const api::FlowControlToken *token_in, GCancellable *cancellable_in,
                  OstreeProgressCb progress_cb_in)
       : target{std::move(target_in)},
-        percent_complete{0},
         token{token_in},
         cancellable{cancellable_in},
         progress_cb{std::move(progress_cb_in)} {}
   Uptane::Target target;
-  unsigned int percent_complete;
+  unsigned int percent_complete{0};
   const api::FlowControlToken *token;
   GObjectUniquePtr<GCancellable> cancellable;
   OstreeProgressCb progress_cb;

src/libaktualizr/primary/aktualizr.cc

@@ -9,7 +9,6 @@
 #include "utilities/apiqueue.h"
 #include "utilities/timer.h"
 
-using std::make_shared;
 using std::shared_ptr;
 
 Aktualizr::Aktualizr(const Config &config)

src/libaktualizr/storage/sql_utils.h

@@ -120,7 +120,7 @@ class SQLiteStatement {
 
   sqlite3* db_;
   std::unique_ptr<sqlite3_stmt, int (*)(sqlite3_stmt*)> stmt_;
-  int bind_cnt_;
+  int bind_cnt_;  // NOLINT
   // copies of data that need to persist for the object duration
   // (avoid vector because of resizing issues)
   std::list<std::string> owned_data_;

src/libaktualizr/storage/sqlstorage.cc

@@ -277,7 +277,7 @@ bool SQLStorage::loadSecondariesInfo(std::vector<SecondaryInfo>* secondaries) co
         key = PublicKey(statement.get_result_col_str(4).value_or(""), key_type);
       }
       std::string extra = statement.get_result_col_str(5).value_or("");
-      new_secs.emplace_back(SecondaryInfo{serial, hw_id, sec_type, key, extra});
+      new_secs.emplace_back(serial, hw_id, sec_type, key, extra);
       empty = false;
     } catch (const boost::bad_optional_access&) {
       continue;
@@ -1373,7 +1373,7 @@ void SQLStorage::getPendingEcus(std::vector<std::pair<Uptane::EcuSerial, Hash>>*
   for (; statement_result != SQLITE_DONE; statement_result = statement.step()) {
     std::string ecu_serial = statement.get_result_col_str(0).value();
     std::string hash = statement.get_result_col_str(1).value();
-    ecu_res.emplace_back(std::make_pair(Uptane::EcuSerial(ecu_serial), Hash(Hash::Type::kSha256, hash)));
+    ecu_res.emplace_back(Uptane::EcuSerial(ecu_serial), Hash(Hash::Type::kSha256, hash));
   }
 
   if (pendingEcus != nullptr) {

src/libaktualizr/uptane/fetcher.h

@@ -8,11 +8,11 @@
 
 namespace Uptane {
 
-constexpr int64_t kMaxRootSize = 64 * 1024;
-constexpr int64_t kMaxDirectorTargetsSize = 64 * 1024;
-constexpr int64_t kMaxTimestampSize = 64 * 1024;
-constexpr int64_t kMaxSnapshotSize = 64 * 1024;
-constexpr int64_t kMaxImageTargetsSize = 8 * 1024 * 1024;
+constexpr int64_t kMaxRootSize = 64L * 1024;
+constexpr int64_t kMaxDirectorTargetsSize = 64L * 1024;
+constexpr int64_t kMaxTimestampSize = 64L * 1024;
+constexpr int64_t kMaxSnapshotSize = 64L * 1024;
+constexpr int64_t kMaxImageTargetsSize = 8L * 1024 * 1024;
 
 class IMetadataFetcher {
  public:

src/libaktualizr/uptane/metawithkeys.cc

@@ -76,6 +76,7 @@ void Uptane::MetaWithKeys::UnpackSignedObject(const RepositoryType repo, const R
   }
 
   const std::string canonical = Utils::jsonToCanonicalStr(signed_object["signed"]);
+  // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
   const Json::Value signatures = signed_object["signatures"];
   int valid_signatures = 0;
 

src/libaktualizr/uptane/tuf.cc

@@ -13,7 +13,6 @@
 #include "uptane/exceptions.h"
 
 using Uptane::Target;
-using Uptane::Version;
 
 std::ostream &Uptane::operator<<(std::ostream &os, const RepositoryType &repo_type) {
   os << repo_type.ToString();
@@ -83,7 +82,7 @@ Target::Target(std::string filename, const Json::Value &content) : filename_(std
 
   length_ = content["length"].asUInt64();
 
-  const Json::Value hashes = content["hashes"];
+  const auto &hashes = content["hashes"];
   for (auto i = hashes.begin(); i != hashes.end(); ++i) {
     Hash h(i.key().asString(), (*i).asString());
     if (h.HaveAlgorithm()) {
@@ -101,7 +100,7 @@ void Target::updateCustom(const Json::Value &custom) {
   if (custom_.isMember("hardwareIds")) {
     Json::Value hwids = custom_["hardwareIds"];
     for (auto i = hwids.begin(); i != hwids.end(); ++i) {
-      hwids_.emplace_back(HardwareIdentifier((*i).asString()));
+      hwids_.emplace_back((*i).asString());
     }
   }
 

src/libaktualizr/utilities/apiqueue.h

@@ -76,7 +76,7 @@ class CommandBase<void> : public ICommand {
 template <class T>
 class Command : public CommandBase<T> {
  public:
-  explicit Command(std::function<T()>&& func) : f_{move(func)} {}
+  explicit Command(std::function<T()>&& func) : f_{std::move(func)} {}
   T TaskImplementation(Context* ctx) override {
     (void)ctx;
     return f_();
@@ -89,7 +89,7 @@ class Command : public CommandBase<T> {
 template <class T>
 class CommandFlowControl : public CommandBase<T> {
  public:
-  explicit CommandFlowControl(std::function<T(const api::FlowControlToken*)>&& func) : f_{move(func)} {}
+  explicit CommandFlowControl(std::function<T(const api::FlowControlToken*)>&& func) : f_{std::move(func)} {}
   T TaskImplementation(Context* ctx) override { return f_(ctx->flow_control); }
 
  private:

src/libaktualizr/utilities/utils.cc

@@ -288,7 +288,7 @@ std::string Utils::readFile(const boost::filesystem::path &filename, const bool
   return content;
 }
 
-static constexpr size_t BSIZE = 20 * 512;
+static constexpr size_t BSIZE = 20L * 512;
 
 struct archive_state {
  public:

src/libaktualizr/utilities/utils.h

@@ -176,6 +176,6 @@ static void curlEasySetoptWrapper(CURL *curl_handle, CURLoption option, T &&...a
 // std_ used to contain an implementation of make_unique, and there are various
 // references throughout the codebase. This provides backwards compatbility.
 namespace std_ {
-using std::make_unique;
+using std::make_unique;  // NOLINT
 }
 #endif  // UTILS_H_

src/sota_tools/ostree_repo.cc

@@ -9,7 +9,7 @@ OSTreeObject::ptr OSTreeRepo::GetObject(const uint8_t sha256[32], const OstreeOb
 
 OSTreeObject::ptr OSTreeRepo::GetObject(const OSTreeHash hash, const OstreeObjectType type) const {
   // If we've already seen this object, return another pointer to it
-  otable::const_iterator obj_it = ObjectTable.find(hash);
+  auto obj_it = ObjectTable.find(hash);
   if (obj_it != ObjectTable.cend()) {
     return obj_it->second;
   }
@@ -85,4 +85,4 @@ boost::filesystem::path OSTreeRepo::GetPathForHash(OSTreeHash hash, OstreeObject
       throw OSTreeUnsupportedObjectType(type);
   }
   return boost::filesystem::path(objpath);
-}
+}

src/uptane_generator/repo.cc

@@ -317,7 +317,7 @@ void Repo::refresh(const Uptane::Role &role) {
   if (current_expire_time.IsExpiredAt(TimeStamp::Now())) {
     time_t new_expiration_time;
     std::time(&new_expiration_time);
-    new_expiration_time += 60 * 60;  // make it valid for the next hour
+    new_expiration_time += 60L * 60;  // make it valid for the next hour
     struct tm new_expiration_time_str {};
     gmtime_r(&new_expiration_time, &new_expiration_time_str);
 
@@ -351,7 +351,7 @@ void Repo::rotate(const Uptane::Role &role, KeyType key_type) {
   if (current_expire_time.IsExpiredAt(TimeStamp::Now())) {
     time_t new_expiration_time;
     std::time(&new_expiration_time);
-    new_expiration_time += 60 * 60;  // make it valid for the next hour
+    new_expiration_time += 60L * 60;  // make it valid for the next hour
     struct tm new_expiration_time_str {};
     gmtime_r(&new_expiration_time, &new_expiration_time_str);