OTA-4814: clean the code and make clang-tidy happy

Signed-off-by: Mike Sul <[email protected]>

src/aktualizr_primary/secondary.cc

@@ -29,7 +29,7 @@ static SecondaryFactoryRegistry sec_factory_registry = {
     {VirtualSecondaryConfig::Type,
      [](const SecondaryConfig& config, Aktualizr& aktualizr) {
        auto virtual_sec_cgf = dynamic_cast<const VirtualSecondaryConfig&>(config);
-       ImageReader image_reader = std::bind(&Aktualizr::OpenStoredTarget, &aktualizr, std::placeholders::_1);
+       ImageReaderProvider image_reader = std::bind(&Aktualizr::OpenStoredTarget, &aktualizr, std::placeholders::_1);
        return Secondaries({std::make_shared<VirtualSecondary>(virtual_sec_cgf, image_reader)});
      }},
     //  {
@@ -67,14 +67,14 @@ void initSecondaries(Aktualizr& aktualizr, const boost::filesystem::path& config
 
 class SecondaryWaiter {
  public:
-  SecondaryWaiter(uint16_t wait_port, int timeout_s, Secondaries& secondaries, ImageReader image_reader,
-                  TlsCredsProvider treehub_cred_provider)
+  SecondaryWaiter(uint16_t wait_port, int timeout_s, Secondaries& secondaries,
+                  ImageReaderProvider image_reader_provider, TlsCredsProvider treehub_cred_provider)
       : endpoint_{boost::asio::ip::tcp::v4(), wait_port},
         timeout_{static_cast<boost::posix_time::seconds>(timeout_s)},
         timer_{io_context_},
         connected_secondaries_{secondaries},
-        image_reader_{image_reader},
-        treehub_cred_provider_{treehub_cred_provider} {}
+        image_reader__provider_{std::move(image_reader_provider)},
+        treehub_cred_provider_{std::move(treehub_cred_provider)} {}
 
   void addSecondary(const std::string& ip, uint16_t port) { secondaries_to_wait_for_.insert(key(ip, port)); }
 
@@ -112,8 +112,8 @@ class SecondaryWaiter {
 
       LOG_INFO << "Accepted connection from a Secondary: (" << sec_ip << ":" << sec_port << ")";
       try {
-        auto secondary = Uptane::IpUptaneSecondary::create(sec_ip, sec_port, con_socket_.native_handle(), image_reader_,
-                                                           treehub_cred_provider_);
+        auto secondary = Uptane::IpUptaneSecondary::create(sec_ip, sec_port, con_socket_.native_handle(),
+                                                           image_reader__provider_, treehub_cred_provider_);
         if (secondary) {
           connected_secondaries_.push_back(secondary);
         }
@@ -146,23 +146,23 @@ class SecondaryWaiter {
 
   Secondaries& connected_secondaries_;
   std::unordered_set<std::string> secondaries_to_wait_for_;
-  ImageReader image_reader_;
+  ImageReaderProvider image_reader__provider_;
   TlsCredsProvider treehub_cred_provider_;
 };
 
 static Secondaries createIPSecondaries(const IPSecondariesConfig& config, Aktualizr& aktualizr) {
   Secondaries result;
   const bool provision = !aktualizr.IsRegistered();
-  auto image_reader = std::bind(&Aktualizr::OpenStoredTarget, &aktualizr, std::placeholders::_1);
+  auto image_reader_provider = std::bind(&Aktualizr::OpenStoredTarget, &aktualizr, std::placeholders::_1);
   auto treehub_creds_provider = std::bind(&Aktualizr::GetTreehubTlsCreds, &aktualizr);
 
   if (provision) {
-    SecondaryWaiter sec_waiter{config.secondaries_wait_port, config.secondaries_timeout_s, result, image_reader,
-                               treehub_creds_provider};
+    SecondaryWaiter sec_waiter{config.secondaries_wait_port, config.secondaries_timeout_s, result,
+                               image_reader_provider, treehub_creds_provider};
 
     for (const auto& ip_sec_cfg : config.secondaries_cfg) {
-      auto secondary = Uptane::IpUptaneSecondary::connectAndCreate(ip_sec_cfg.ip, ip_sec_cfg.port, image_reader,
-                                                                   treehub_creds_provider);
+      auto secondary = Uptane::IpUptaneSecondary::connectAndCreate(ip_sec_cfg.ip, ip_sec_cfg.port,
+                                                                   image_reader_provider, treehub_creds_provider);
       if (secondary) {
         result.push_back(secondary);
       } else {
@@ -204,7 +204,8 @@ static Secondaries createIPSecondaries(const IPSecondariesConfig& config, Aktual
         LOG_INFO << "Migrated single IP Secondary to new storage format";
       } else if (f == secondaries_info.cend()) {
         // Match the other way if we can
-        secondary = Uptane::IpUptaneSecondary::connectAndCreate(cfg.ip, cfg.port, image_reader, treehub_creds_provider);
+        secondary = Uptane::IpUptaneSecondary::connectAndCreate(cfg.ip, cfg.port, image_reader_provider,
+                                                                treehub_creds_provider);
         if (secondary == nullptr) {
           LOG_ERROR << "Could not instantiate Secondary " << cfg.ip << ":" << cfg.port;
           continue;
@@ -222,8 +223,8 @@ static Secondaries createIPSecondaries(const IPSecondariesConfig& config, Aktual
       }
 
       if (secondary == nullptr) {
-        secondary = Uptane::IpUptaneSecondary::connectAndCheck(cfg.ip, cfg.port, info->serial, info->hw_id,
-                                                               info->pub_key, image_reader, treehub_creds_provider);
+        secondary = Uptane::IpUptaneSecondary::connectAndCheck(
+            cfg.ip, cfg.port, info->serial, info->hw_id, info->pub_key, image_reader_provider, treehub_creds_provider);
       }
 
       if (secondary != nullptr) {

src/aktualizr_secondary/CMakeLists.txt

@@ -48,7 +48,6 @@ set(ALL_AKTUALIZR_SECONDARY_HEADERS
     aktualizr_secondary_config.h
     aktualizr_secondary_metadata.h
     secondary_tcp_server.h
-    aktualizr_secondary_interface.h
     msg_handler.h
     )
 

src/aktualizr_secondary/aktualizr_secondary.cc

@@ -224,7 +224,7 @@ AktualizrSecondary::ReturnCode AktualizrSecondary::getManifestHdlr(Asn1Message&
   out_msg.present(AKIpUptaneMes_PR_manifestResp);
   auto manifest_resp = out_msg.manifestResp();
   manifest_resp->manifest.present = manifest_PR_json;
-  SetString(&manifest_resp->manifest.choice.json, Utils::jsonToStr(getManifest()));
+  SetString(&manifest_resp->manifest.choice.json, Utils::jsonToStr(getManifest()));  // NOLINT
 
   LOG_TRACE << "Manifest : \n" << getManifest();
   return ReturnCode::kOk;
@@ -263,32 +263,6 @@ AktualizrSecondary::ReturnCode AktualizrSecondary::putMetaHdlr(Asn1Message& in_m
   return ReturnCode::kOk;
 }
 
-// MsgDispatcher::HandleStatusCode AktualizrSecondary::MsgDispatcher::sendFirmwareHdlr(Asn1Message& in_msg,
-//                                                                                  Asn1Message& out_msg) {
-//  auto fw = in_msg.sendFirmwareReq();
-//  auto send_firmware_result = secondary_.sendFirmware(ToString(in_msg.sendFirmwareReq()->firmware));
-
-//  out_msg.present(AKIpUptaneMes_PR_sendFirmwareResp).sendFirmwareResp()->result =
-//      send_firmware_result ? AKInstallationResult_success : AKInstallationResult_failure;
-//  ;
-
-//  return HandleStatusCode::kOk;
-//}
-
-// MsgDispatcher::HandleStatusCode AktualizrSecondary::MsgDispatcher::sendFirmwareDataHdlr(Asn1Message& in_msg,
-//                                                                                      Asn1Message& out_msg) {
-//  auto fw = in_msg.sendFirmwareDataReq();
-
-//  auto send_firmware_result =
-//      secondary_.sendFirmware(in_msg.sendFirmwareDataReq()->data.buf, in_msg.sendFirmwareDataReq()->data.size);
-
-//  out_msg.present(AKIpUptaneMes_PR_sendFirmwareResp).sendFirmwareResp()->result =
-//      (send_firmware_result == data::ResultCode::Numeric::kOk) ? AKInstallationResult_success
-//                                                               : AKInstallationResult_failure;
-
-//  return HandleStatusCode::kOk;
-//}
-
 AktualizrSecondary::ReturnCode AktualizrSecondary::installHdlr(Asn1Message& in_msg, Asn1Message& out_msg) {
   (void)in_msg;
   auto install_result = install();

src/aktualizr_secondary/aktualizr_secondary.h

@@ -17,9 +17,6 @@ class AktualizrSecondary : public MsgDispatcher {
  public:
   using Ptr = std::shared_ptr<AktualizrSecondary>;
 
- public:
-  virtual ~AktualizrSecondary() = default;
-
   const Uptane::EcuSerial& serial() const { return ecu_serial_; }
   const Uptane::HardwareIdentifier& hwID() const { return hardware_id_; }
   PublicKey publicKey() const;
@@ -40,7 +37,6 @@ class AktualizrSecondary : public MsgDispatcher {
   virtual data::ResultCode::Numeric installPendingTarget(const Uptane::Target& target) = 0;
   virtual data::InstallationResult applyPendingInstall(const Uptane::Target& target) = 0;
 
- protected:
   // protected interface to be used by child classes
   Uptane::Target& pendingTarget() { return pending_target_; }
   INvStorage& storage() { return *storage_; }
@@ -55,14 +51,12 @@ class AktualizrSecondary : public MsgDispatcher {
   void uptaneInitialize();
   void registerHandlers();
 
- private:
   // Message handlers
   ReturnCode getInfoHdlr(Asn1Message& in_msg, Asn1Message& out_msg);
   ReturnCode getManifestHdlr(Asn1Message& in_msg, Asn1Message& out_msg);
   ReturnCode putMetaHdlr(Asn1Message& in_msg, Asn1Message& out_msg);
   ReturnCode installHdlr(Asn1Message& in_msg, Asn1Message& out_msg);
 
- private:
   Uptane::HardwareIdentifier hardware_id_{Uptane::HardwareIdentifier::Unknown()};
   Uptane::EcuSerial ecu_serial_{Uptane::EcuSerial::Unknown()};
 

src/aktualizr_secondary/aktualizr_secondary_file.cc

@@ -8,7 +8,7 @@ AktualizrSecondaryFile::AktualizrSecondaryFile(AktualizrSecondaryConfig config)
 
 AktualizrSecondaryFile::AktualizrSecondaryFile(AktualizrSecondaryConfig config, std::shared_ptr<INvStorage> storage,
                                                std::shared_ptr<FileUpdateAgent> update_agent)
-    : AktualizrSecondary(config, storage), update_agent_{update_agent} {
+    : AktualizrSecondary(config, std::move(storage)), update_agent_{std::move(update_agent)} {
   registerHandler(AKIpUptaneMes_PR_uploadDataReq, std::bind(&AktualizrSecondaryFile::uploadDataHdlr, this,
                                                             std::placeholders::_1, std::placeholders::_2));
   if (!update_agent_) {
@@ -35,6 +35,7 @@ data::ResultCode::Numeric AktualizrSecondaryFile::receiveData(const uint8_t* dat
     return data::ResultCode::Numeric::kGeneralError;
   }
 
+  LOG_INFO << "Receiving target image data from Primary: " << size;
   return update_agent_->receiveData(pendingTarget(), data, size);
 }
 
@@ -57,9 +58,15 @@ data::ResultCode::Numeric AktualizrSecondaryFile::installPendingTarget(const Upt
 void AktualizrSecondaryFile::completeInstall() { return update_agent_->completeInstall(); }
 
 MsgHandler::ReturnCode AktualizrSecondaryFile::uploadDataHdlr(Asn1Message& in_msg, Asn1Message& out_msg) {
-  auto fw = in_msg.uploadDataReq();
+  LOG_INFO << "Handling upload data request from Primary";
 
-  auto send_firmware_result = receiveData(in_msg.uploadDataReq()->data.buf, in_msg.uploadDataReq()->data.size);
+  auto rec_buf_size = in_msg.uploadDataReq()->data.size;
+  if (rec_buf_size < 0) {
+    LOG_ERROR << "The received data buffer size has a negative size: " << rec_buf_size;
+    return ReturnCode::kOk;
+  }
+
+  auto send_firmware_result = receiveData(in_msg.uploadDataReq()->data.buf, static_cast<size_t>(rec_buf_size));
 
   out_msg.present(AKIpUptaneMes_PR_uploadDataResp).uploadDataResp()->result =
       (send_firmware_result == data::ResultCode::Numeric::kOk) ? AKInstallationResult_success

src/aktualizr_secondary/aktualizr_secondary_ostree.cc

@@ -5,7 +5,8 @@
 AktualizrSecondaryOstree::AktualizrSecondaryOstree(AktualizrSecondaryConfig config)
     : AktualizrSecondaryOstree(config, INvStorage::newStorage(config.storage)) {}
 
-AktualizrSecondaryOstree::AktualizrSecondaryOstree(AktualizrSecondaryConfig config, std::shared_ptr<INvStorage> storage)
+AktualizrSecondaryOstree::AktualizrSecondaryOstree(AktualizrSecondaryConfig config,
+                                                   const std::shared_ptr<INvStorage>& storage)
     : AktualizrSecondary(config, storage) {
   registerHandler(AKIpUptaneMes_PR_downloadOstreeRevReq, std::bind(&AktualizrSecondaryOstree::downloadOstreeRev, this,
                                                                    std::placeholders::_1, std::placeholders::_2));
@@ -53,6 +54,7 @@ AktualizrSecondaryOstree::AktualizrSecondaryOstree(AktualizrSecondaryConfig conf
 }
 
 MsgHandler::ReturnCode AktualizrSecondaryOstree::downloadOstreeRev(Asn1Message& in_msg, Asn1Message& out_msg) {
+  LOG_INFO << "Received a request to download a new ostree revision from Treehub";
   auto download_ostree_rev_result = downloadOstreeUpdate(ToString(in_msg.downloadOstreeRevReq()->tlsCred));
 
   out_msg.present(AKIpUptaneMes_PR_downloadOstreeRevResp).downloadOstreeRevResp()->result =
@@ -61,19 +63,16 @@ MsgHandler::ReturnCode AktualizrSecondaryOstree::downloadOstreeRev(Asn1Message&
   return ReturnCode::kOk;
 }
 
-data::ResultCode::Numeric AktualizrSecondaryOstree::downloadOstreeUpdate(std::string packed_tls_creds) {
+data::ResultCode::Numeric AktualizrSecondaryOstree::downloadOstreeUpdate(const std::string& packed_tls_creds) {
   if (!pendingTarget().IsValid()) {
     LOG_ERROR << "Aborting image download/receiving; no valid target found.";
     return data::ResultCode::Numeric::kGeneralError;
   }
 
   if (!update_agent_->downloadTargetRev(pendingTarget(), packed_tls_creds)) {
-    LOG_ERROR << "Failed to pull/store an update data";
     pendingTarget() = Uptane::Target::Unknown();
     return data::ResultCode::Numeric::kGeneralError;
   }
-
-  LOG_INFO << "Download firmware " << pendingTarget().filename() << " successful.";
   return data::ResultCode::Numeric::kOk;
 }
 

src/aktualizr_secondary/aktualizr_secondary_ostree.h

@@ -7,11 +7,10 @@ class OstreeUpdateAgent;
 
 class AktualizrSecondaryOstree : public AktualizrSecondary {
  public:
-  AktualizrSecondaryOstree(AktualizrSecondaryConfig config, std::shared_ptr<INvStorage> storage);
+  AktualizrSecondaryOstree(AktualizrSecondaryConfig config, const std::shared_ptr<INvStorage>& storage);
   AktualizrSecondaryOstree(AktualizrSecondaryConfig config);
 
- public:
-  data::ResultCode::Numeric downloadOstreeUpdate(std::string packed_tls_creds);
+  data::ResultCode::Numeric downloadOstreeUpdate(const std::string& packed_tls_creds);
 
  protected:
   bool getInstalledImageInfo(Uptane::InstalledImageInfo& installed_image_info) const override;

src/aktualizr_secondary/aktualizr_secondary_test.cc

@@ -105,7 +105,7 @@ class UptaneRepoWrapper {
       std::ofstream broken_image{broken_image_file_path,
                                  std::ios_base::in | std::ios_base::out | std::ios_base::ate | std::ios_base::binary};
       unsigned char data_to_inject[]{0xFF};
-      broken_image.seekp(-sizeof(data_to_inject), std::ios_base::end);
+      broken_image.seekp(static_cast<long>(-sizeof(data_to_inject)), std::ios_base::end);
       broken_image.write(reinterpret_cast<const char*>(data_to_inject), sizeof(data_to_inject));
       broken_image.close();
     }
@@ -143,8 +143,8 @@ class UptaneRepoWrapper {
     unsigned char cur_symbol;
 
     for (unsigned int ii = 0; ii < size; ++ii) {
-      cur_symbol = symbols[rand() % sizeof(symbols)];
-      file.put(cur_symbol);
+      cur_symbol = symbols[static_cast<unsigned int>(rand()) % sizeof(symbols)];
+      file.put(static_cast<char>(cur_symbol));
     }
 
     file.close();
@@ -196,12 +196,12 @@ class SecondaryTest : public ::testing::Test {
         return data::ResultCode::Numeric::kGeneralError;
       }
 
-      auto result = secondary_->receiveData(buf, read_bytes);
+      auto result = secondary_->receiveData(buf, static_cast<size_t>(read_bytes));
       if (result != data::ResultCode::Numeric::kOk) {
         file.close();
         return result;
       }
-      read_and_send_data_size += read_bytes;
+      read_and_send_data_size += static_cast<size_t>(read_bytes);
     }
 
     file.close();

src/aktualizr_secondary/main.cc

@@ -97,8 +97,16 @@ int main(int argc, char *argv[]) {
     else {
       secondary = std::make_shared<AktualizrSecondaryOstree>(config);
     }
+#else
+    else {
+      LOG_ERROR << "Unsupported type of Secondary: " << config.pacman.type;
+    }
 #endif  // BUILD_OSTREE
 
+    if (!secondary) {
+      throw std::runtime_error("Failed to create IP Secondary of the specified type: " + config.pacman.type);
+    }
+
     SecondaryTcpServer tcp_server(*secondary, config.network.primary_ip, config.network.primary_port,
                                   config.network.port, config.uptane.force_install_completion);
 
@@ -108,7 +116,7 @@ int main(int argc, char *argv[]) {
       secondary->completeInstall();
     }
 
-  } catch (std::runtime_error &exc) {
+  } catch (std::exception &exc) {
     LOG_ERROR << "Error: " << exc.what();
     ret = EXIT_FAILURE;
   }

src/aktualizr_secondary/msg_handler.cc

@@ -7,15 +7,15 @@ void MsgDispatcher::registerHandler(AKIpUptaneMes_PR msg_id, Handler handler) {
 }
 
 MsgHandler::ReturnCode MsgDispatcher::handleMsg(const Asn1Message::Ptr& in_msg, Asn1Message::Ptr& out_msg) {
-  LOG_INFO << "Got a request message from Primary: " << in_msg->toStr();
+  LOG_DEBUG << "Got a request message from Primary: " << in_msg->toStr();
 
   auto find_res_it = handler_map_.find(in_msg->present());
   if (find_res_it == handler_map_.end()) {
     return MsgHandler::kUnkownMsg;
   }
-  LOG_INFO << "Found a handler for the request message, processing it...";
+  LOG_DEBUG << "Found a handler for the request message, processing it...";
   auto handle_status_code = find_res_it->second(*in_msg, *out_msg);
-  LOG_INFO << "Got a response message from a handler: " << out_msg->toStr();
+  LOG_DEBUG << "Got a response message from a handler: " << out_msg->toStr();
 
   return handle_status_code;
 }