Merge pull request #3145 from smartdevicelink/fix/SDL_crash_during_SetAppIconremote_atf_testing

Fix/sdl crash during set app icon

4 files changed, 262 insertions, 104 deletions

diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/set_app_icon_request.h b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/set_app_icon_request.h
index 86ca2fb126..520f27d8b9 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/set_app_icon_request.h
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/set_app_icon_request.h
@@ -83,9 +83,11 @@ class SetAppIconRequest : public app_mngr::commands::CommandRequestImpl {
  private:
   /**
    * @brief Copies file to icon storage
+   * @param policy_app_id application policy app id
    * @param path_to_file Path to icon
    */
-  void CopyToIconStorage(const std::string& path_to_file) const;
+  void CopyToIconStorage(const std::string& policy_app_id,
+                         const std::string& path_to_file) const;
 
   /**
    * @brief Remove oldest icons
@@ -108,6 +110,11 @@ class SetAppIconRequest : public app_mngr::commands::CommandRequestImpl {
    * @brief Checks, if icons saving to configured folder is enabled
    */
   bool is_icons_saving_enabled_;
+
+  /**
+   * @brief Contains full file path to icon
+   */
+  std::string full_file_path_for_hmi_;
 };
 
 }  // namespace commands
diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/set_app_icon_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/set_app_icon_request.cc
index b2363e870f..acf312d532 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/set_app_icon_request.cc
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/set_app_icon_request.cc
@@ -106,25 +106,22 @@ void SetAppIconRequest::Run() {
   msg_params[strings::sync_file_name] =
       smart_objects::SmartObject(smart_objects::SmartType_Map);
 
-  // Panasonic requres unchanged path value without encoded special characters
-  const std::string full_file_path_for_hmi =
-      file_system::ConvertPathForURL(full_file_path);
+  // For further use in on_event function
+  full_file_path_for_hmi_ = file_system::ConvertPathForURL(full_file_path);
 
-  msg_params[strings::sync_file_name][strings::value] = full_file_path_for_hmi;
+  msg_params[strings::sync_file_name][strings::value] = full_file_path_for_hmi_;
 
   // TODO(VS): research why is image_type hardcoded
   msg_params[strings::sync_file_name][strings::image_type] =
       static_cast<int32_t>(SetAppIconRequest::ImageType::DYNAMIC);
 
-  // for further use in on_event function
-  (*message_)[strings::msg_params][strings::sync_file_name] =
-      msg_params[strings::sync_file_name];
   StartAwaitForInterface(HmiInterfaces::HMI_INTERFACE_UI);
   SendHMIRequest(hmi_apis::FunctionID::UI_SetAppIcon, &msg_params, true);
 }
 
 void SetAppIconRequest::CopyToIconStorage(
-    const std::string& path_to_file) const {
+    const std::string& policy_app_id, const std::string& path_to_file) const {
+  LOG4CXX_AUTO_TRACE(logger_);
   if (!(application_manager_.protocol_handler()
             .get_settings()
             .max_supported_protocol_version() >=
@@ -146,6 +143,11 @@ void SetAppIconRequest::CopyToIconStorage(
       application_manager_.get_settings().app_icons_folder_max_size());
   const uint64_t file_size = file_system::FileSize(path_to_file);
 
+  if (0 == file_size) {
+    LOG4CXX_ERROR(logger_, "Can't get the icon file size: " << path_to_file);
+    return;
+  }
+
   if (storage_max_size < file_size) {
     LOG4CXX_ERROR(logger_,
                   "Icon size (" << file_size
@@ -159,6 +161,12 @@ void SetAppIconRequest::CopyToIconStorage(
 
   const uint64_t storage_size =
       static_cast<uint64_t>(file_system::DirectorySize(icon_storage));
+
+  if (0 == storage_size) {
+    LOG4CXX_ERROR(logger_, "Can't get the folder size: " << icon_storage);
+    return;
+  }
+
   if (storage_max_size < (file_size + storage_size)) {
     const uint32_t icons_amount =
         application_manager_.get_settings().app_icons_amount_to_remove();
@@ -174,23 +182,15 @@ void SetAppIconRequest::CopyToIconStorage(
       RemoveOldestIcons(icon_storage, icons_amount);
     }
   }
-  ApplicationConstSharedPtr app =
-      application_manager_.application(connection_key());
 
-  if (!app) {
-    LOG4CXX_ERROR(
-        logger_,
-        "Can't get application for connection key: " << connection_key());
-    return;
-  }
+  const std::string icon_path = icon_storage + "/" + policy_app_id;
 
-  const std::string icon_path = icon_storage + "/" + app->policy_app_id();
   if (!file_system::CreateFile(icon_path)) {
     LOG4CXX_ERROR(logger_, "Can't create icon: " << icon_path);
     return;
   }
 
-  if (!file_system::Write(icon_path, file_content)) {
+  if (!file_system::WriteBinaryFile(icon_path, file_content)) {
     LOG4CXX_ERROR(logger_, "Can't write icon: " << icon_path);
     return;
   }
@@ -266,21 +266,19 @@ void SetAppIconRequest::on_event(const event_engine::Event& event) {
         ApplicationSharedPtr app =
             application_manager_.application(connection_key());
 
-        if ((message_.use_count() == 0) || (app.use_count() == 0)) {
-          LOG4CXX_ERROR(logger_, "NULL pointer.");
+        if (!app) {
+          LOG4CXX_ERROR(
+              logger_,
+              "Can't get application for connection key: " << connection_key());
           return;
         }
 
-        const std::string& path =
-            (*message_)[strings::msg_params][strings::sync_file_name]
-                       [strings::value]
-                           .asString();
-
-        if (is_icons_saving_enabled_) {
-          CopyToIconStorage(path);
+        if (is_icons_saving_enabled_ && !full_file_path_for_hmi_.empty()) {
+          const auto policy_app_id = app->policy_app_id();
+          CopyToIconStorage(policy_app_id, full_file_path_for_hmi_);
         }
 
-        app->set_app_icon_path(path);
+        app->set_app_icon_path(full_file_path_for_hmi_);
 
         LOG4CXX_INFO(logger_,
                      "Icon path was set to '" << app->app_icon_path() << "'");
diff --git a/src/components/utils/include/utils/file_system.h b/src/components/utils/include/utils/file_system.h
index 9e0dc233af..f575da129f 100644
--- a/src/components/utils/include/utils/file_system.h
+++ b/src/components/utils/include/utils/file_system.h
@@ -70,10 +70,10 @@ uint64_t FileSize(const std::string& path);
 
 /**
  * @brief Creates directory with owner_all permissions
- * @param path directory to create
- * @return true if directory was created or already exist.
+ * @param name directory to create
+ * @return true if directory was created or already existed.
  */
-bool CreateDirectory(const std::string& path);
+bool CreateDirectory(const std::string& name);
 
 /**
  * @brief Creates directory recursively
@@ -188,7 +188,7 @@ void remove_directory_content(const std::string& directory_name);
  * @return returns true if the directory is successfully deleted.
  */
 bool RemoveDirectory(const std::string& directory_name,
-                     bool is_recursively = true);
+                     const bool is_recursively = true);
 
 /**
  * @brief Check access rights
@@ -197,7 +197,7 @@ bool RemoveDirectory(const std::string& directory_name,
  * @param how Read/write attribute.
  * @return returns true if file has the given mode.
  */
-bool IsAccessible(const std::string& name, int32_t how);
+bool IsAccessible(const std::string& name, const int32_t how);
 
 /**
  * @brief Check access rights for writing
diff --git a/src/components/utils/src/file_system.cc b/src/components/utils/src/file_system.cc
index c2fb8a4eb4..015dc25a22 100644
--- a/src/components/utils/src/file_system.cc
+++ b/src/components/utils/src/file_system.cc
@@ -47,80 +47,116 @@
 #include <cstdio>
 #include <fstream>
 
-CREATE_LOGGERPTR_GLOBAL(logger_, "Utils")
+CREATE_LOGGERPTR_GLOBAL(logger_, "Utils::FileSystem")
 
 // Easier reference
 namespace fs = boost::filesystem;
 using boost::system::error_code;
 
 uint64_t file_system::GetAvailableDiskSpace(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  fs::space_info si = fs::space(path, ec);
+  fs::space_info si = {0, 0, 0};
+  si = fs::space(path, ec);
 
   if (ec) {
     // If something went wrong, assume no free space
-    return 0;
-  } else {
-    return si.free;
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to get available disk space: '"
+                                 << path << "', reason: " << ec.message());
   }
+  return si.free;
 }
 
 uint64_t file_system::FileSize(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
   // Boost returns sizes as unsigned
-  uint64_t fsize = (uint64_t)fs::file_size(path, ec);
+  const uint64_t fsize = static_cast<uint64_t>(fs::file_size(path, ec));
 
   if (ec) {
-    LOG4CXX_WARN_WITH_ERRNO(logger_, "Could not get file size: " << path);
+    LOG4CXX_ERROR_WITH_ERRNO(
+        logger_,
+        "Unable to get file size: '" << path << "', reason: " << ec.message());
     return 0;
   }
   return fsize;
 }
 
 size_t file_system::DirectorySize(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
   size_t dir_size = 0;
   error_code ec;
   // Recursively iterate through directory to accumulate file sizes
   fs::recursive_directory_iterator iter(path, ec);
-  // Directory does not exist
   if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to get directory size: '"
+                                 << path << "', reason: " << ec.message());
     return 0;
   }
+
   // default constructor gives end iterator
   fs::recursive_directory_iterator end;
-  while (iter != end) {
-    size_t fsize = fs::file_size(iter->path(), ec);
-    // No error means we can add the file
-    if (!ec) {
-      dir_size += fsize;
+  while (end != iter) {
+    const bool is_directory = fs::is_directory(iter->path(), ec);
+    if (ec) {
+      LOG4CXX_WARN_WITH_ERRNO(
+          logger_,
+          "Failed check if '" << iter->path()
+                              << "' is directory, reason: " << ec.message());
+    }
+
+    if (!is_directory && !ec) {
+      const size_t fsize = fs::file_size(iter->path(), ec);
+      if (ec) {
+        LOG4CXX_WARN_WITH_ERRNO(logger_,
+                                "Failed to get file_size: '"
+                                    << path << "', reason: " << ec.message());
+      } else {
+        // No error means we can add the file
+        dir_size += fsize;
+        LOG4CXX_DEBUG(logger_, "Adding: " << fsize << ", total: " << dir_size);
+      }
+    }
+
+    // Increment the iterator to point to next entry in recursive iteration
+    iter.increment(ec);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                               "Failed to increment iterator for path '"
+                                   << path << "', reason: " << ec.message());
+      return dir_size;
     }
-    iter++;  // next entry
   }
+
   return dir_size;
 }
 
 // NOTE that boost makes 0777 permissions by default
-bool file_system::CreateDirectory(const std::string& path) {
+bool file_system::CreateDirectory(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  const bool success = fs::create_directory(path, ec);
+  const bool success = fs::create_directory(name, ec);
   if (!success || ec) {
-    LOG4CXX_WARN_WITH_ERRNO(logger_, "Unable to create directory: " << path);
+    LOG4CXX_WARN_WITH_ERRNO(logger_, "Unable to create directory: " << name);
   } else {
     // Set 0700 permissions to maintain previous API
-    fs::permissions(path, fs::perms::owner_all, ec);
+    fs::permissions(name, fs::perms::owner_all, ec);
   }
   return success;
 }
 
 bool file_system::CreateDirectoryRecursively(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
   // Create directory and all parents
   fs::create_directories(path, ec);
 
   if (ec) {
-    LOG4CXX_WARN_WITH_ERRNO(logger_,
-                            "Unable to create directory recursively: "
-                                << path << " reason: " << ec.message());
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to create directory recursively: '"
+                                 << path << "', reason: " << ec.message());
 
     return false;
   }
@@ -129,23 +165,44 @@ bool file_system::CreateDirectoryRecursively(const std::string& path) {
 }
 
 bool file_system::IsDirectory(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  return fs::is_directory(name, ec);
+  const bool is_directory = fs::is_directory(name, ec);
+  if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to check if it is directory: "
+                                 << name << " reason: " << ec.message());
+  }
+  return is_directory;
 }
+
 // NOTE this may be a duplicate of IsDirectory since it already checks
 // existence
 bool file_system::DirectoryExists(const std::string& name) {
-  return FileExists(name) && IsDirectory(name);
+  LOG4CXX_AUTO_TRACE(logger_);
+  const bool exists = FileExists(name) && IsDirectory(name);
+  LOG4CXX_DEBUG(
+      logger_,
+      "Directory '" << name << "' " << (exists ? "exists" : "NOT exists"));
+  return exists;
 }
 
 bool file_system::FileExists(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  return fs::exists(name, ec);
+  const bool exists = fs::exists(name, ec);
+  if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to check that file exists: "
+                                 << name << " reason: " << ec.message());
+  }
+  return exists;
 }
 
 bool file_system::Write(const std::string& file_name,
                         const std::vector<uint8_t>& data,
                         std::ios_base::openmode mode) {
+  LOG4CXX_AUTO_TRACE(logger_);
   std::ofstream file(file_name.c_str(), std::ios_base::binary | mode);
   if (file.is_open()) {
     for (uint32_t i = 0; i < data.size(); ++i) {
@@ -159,6 +216,7 @@ bool file_system::Write(const std::string& file_name,
 
 std::ofstream* file_system::Open(const std::string& file_name,
                                  std::ios_base::openmode mode) {
+  LOG4CXX_AUTO_TRACE(logger_);
   std::ofstream* file = new std::ofstream();
   file->open(file_name.c_str(), std::ios_base::binary | mode);
   if (file->is_open()) {
@@ -166,12 +224,13 @@ std::ofstream* file_system::Open(const std::string& file_name,
   }
 
   delete file;
-  return NULL;
+  return nullptr;
 }
 
 bool file_system::Write(std::ofstream* const file_stream,
                         const uint8_t* data,
                         uint32_t data_size) {
+  LOG4CXX_AUTO_TRACE(logger_);
   bool result = false;
   if (file_stream) {
     for (size_t i = 0; i < data_size; ++i) {
@@ -183,69 +242,105 @@ bool file_system::Write(std::ofstream* const file_stream,
 }
 
 void file_system::Close(std::ofstream* file_stream) {
+  LOG4CXX_AUTO_TRACE(logger_);
   if (file_stream) {
     file_stream->close();
   }
 }
 
 std::string file_system::CurrentWorkingDirectory() {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  fs::path currpath = fs::current_path(ec);
+  const fs::path currpath = fs::current_path(ec);
   if (ec) {
-    LOG4CXX_WARN(logger_, "Could not get CWD");
+    LOG4CXX_WARN_WITH_ERRNO(logger_,
+                            "Unable to get current working directory: '"
+                                << currpath << "' reason: " << ec.message());
   }
   return currpath.string();
 }
 
 std::string file_system::GetAbsolutePath(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
   error_code ec;
-  fs::path absolute = fs::canonical(path, ec);
+  const fs::path absolute = fs::canonical(path, ec);
   if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to get absolute path: '"
+                                 << path << "', reason: " << ec.message());
     return std::string();  // invalid path
   }
   return absolute.string();
 }
 
 bool file_system::IsFileNameValid(const std::string& file_name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   return file_name.end() == std::find(file_name.begin(), file_name.end(), '/');
 }
 
 // Does not remove if file is write-protected
 bool file_system::DeleteFile(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   if (FileExists(name) && IsAccessible(name, W_OK)) {
     error_code ec;
-    bool success = fs::remove(name.c_str(), ec);
+    const bool success = fs::remove(name.c_str(), ec);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                               "Unable to delete file: '"
+                                   << name << "', reason: " << ec.message()
+                                   << "success: " << success);
+    }
+
     return success && !ec;
   }
+  LOG4CXX_WARN(
+      logger_,
+      "Unable to delete file either doesn't exist or is not accessible");
   return false;
 }
 
 void file_system::remove_directory_content(const std::string& directory_name) {
+  LOG4CXX_AUTO_TRACE(logger_);
+
   error_code ec;
   fs::directory_iterator dir_iter(directory_name, ec);
-
   if (ec) {
-    LOG4CXX_WARN_WITH_ERRNO(logger_,
-                            "Unable to empty directory: " << directory_name);
+    LOG4CXX_ERROR_WITH_ERRNO(
+        logger_,
+        "Unable to remove directory contents: " << directory_name
+                                                << " reason: " << ec.message());
   }
 
   // According to Boost's documentation, removing shouldn't invalidate the
   // iterator, although it may cause the removed entry to appear again,
   // duplicating the warning message. See here:
   // https://www.boost.org/doc/libs/1_67_0/libs/filesystem/doc/reference.html#Class-directory_iterator
-  for (auto& dirent : dir_iter) {
-    fs::remove_all(dirent, ec);
+  const fs::directory_iterator end;
+  while (dir_iter != end) {
+    fs::remove_all(dir_iter->path(), ec);
     if (ec) {
-      LOG4CXX_WARN_WITH_ERRNO(
-          logger_, "Unable to remove file: " << dirent.path().string());
+      LOG4CXX_ERROR_WITH_ERRNO(
+          logger_,
+          "Unable to remove file: " << dir_iter->path().string() << " reason "
+                                    << ec.message());
+    }
+    dir_iter.increment(ec);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(
+          logger_, "Unable to increment dir_iter: reason " << ec.message());
+      break;
     }
   }
 }
 
 bool file_system::RemoveDirectory(const std::string& directory_name,
-                                  bool is_recursively) {
+                                  const bool is_recursively) {
+  LOG4CXX_AUTO_TRACE(logger_);
   // Make sure the directory exists
   if (!DirectoryExists(directory_name) && IsAccessible(directory_name, W_OK)) {
+    LOG4CXX_WARN(
+        logger_,
+        "Unable to remove directory either doesn't exist or is not accessible");
     return false;
   }
   error_code ec;
@@ -253,42 +348,67 @@ bool file_system::RemoveDirectory(const std::string& directory_name,
   // If recursive, just force full remove
   if (is_recursively) {
     success = (fs::remove_all(directory_name, ec) != 0);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                               "Unable to remove all: '" << directory_name
+                                                         << "', reason "
+                                                         << ec.message());
+    }
   } else {
     // Otherwise try to remove
     success = fs::remove(directory_name, ec);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                               "Unable to remove: '" << directory_name
+                                                     << "', reason "
+                                                     << ec.message());
+    }
   }
   return success && !ec;
 }
 
-bool file_system::IsAccessible(const std::string& name, int32_t how) {
+bool file_system::IsAccessible(const std::string& name, const int32_t how) {
+  LOG4CXX_AUTO_TRACE(logger_);
   return !access(name.c_str(), how);
 }
 
 bool file_system::IsWritingAllowed(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   return IsAccessible(name, W_OK);
 }
 
 bool file_system::IsReadingAllowed(const std::string& name) {
+  LOG4CXX_AUTO_TRACE(logger_);
   return IsAccessible(name, R_OK);
 }
 
 std::vector<std::string> file_system::ListFiles(
     const std::string& directory_name) {
-  error_code ec;
-
-  fs::directory_iterator dir_iter(directory_name, ec);
-  std::vector<std::string> listFiles;
+  LOG4CXX_AUTO_TRACE(logger_);
 
-  // In case the directory doesn't exist / can't be read, second check may be
-  // redundant
-  if (ec || !DirectoryExists(directory_name)) {
-    return listFiles;
+  error_code ec;
+  fs::directory_iterator iter(directory_name, ec), end;
+  if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(
+        logger_,
+        "Unable to get directory_iterator: " << directory_name << " reason "
+                                             << ec.message());
+    return std::vector<std::string>();
   }
 
-  for (auto& dirent : dir_iter) {
-    listFiles.push_back(dirent.path().filename().string());
+  std::vector<std::string> list_files;
+  while (end != iter) {
+    list_files.push_back(iter->path().filename().string());
+    iter.increment(ec);
+    if (ec) {
+      LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                               "Failed to increment iterator for path '"
+                                   << directory_name
+                                   << "', reason: " << ec.message());
+      return list_files;
+    }
   }
-  return listFiles;
+  return list_files;
 }
 
 bool file_system::WriteBinaryFile(const std::string& name,
@@ -302,17 +422,22 @@ bool file_system::WriteBinaryFile(const std::string& name,
 
 bool file_system::ReadBinaryFile(const std::string& name,
                                  std::vector<uint8_t>& result) {
+  LOG4CXX_AUTO_TRACE(logger_);
+  LOG4CXX_DEBUG(logger_, "Filename: " << name);
   if (!FileExists(name) || !IsAccessible(name, R_OK)) {
+    LOG4CXX_ERROR(logger_, "Not able to read binary file: " << name);
     return false;
   }
 
-  std::ifstream file(name.c_str(), std::ios_base::binary);
-  std::ostringstream ss;
-  ss << file.rdbuf();
-  const std::string s = ss.str();
+  std::ifstream file(name.c_str(), std::ios::in | std::ios_base::binary);
+  if (!file.is_open()) {
+    return false;
+  }
+
+  std::vector<uint8_t> content((std::istreambuf_iterator<char>(file)),
+                               std::istreambuf_iterator<char>());
+  result.swap(content);
 
-  result.resize(s.length());
-  std::copy(s.begin(), s.end(), result.begin());
   return true;
 }
 
@@ -321,12 +446,16 @@ bool file_system::ReadBinaryFile(const std::string& name,
                                  uint32_t offset,
                                  uint32_t length) {
   if (!FileExists(name) || !IsAccessible(name, R_OK)) {
+    LOG4CXX_ERROR(logger_, "Not able to read binary file: " << name);
     return false;
   }
 
   std::ifstream file(name.c_str(), std::ios_base::binary);
+  if (!file.is_open()) {
+    return false;
+  }
+
   file.ignore(offset);
-  std::ostringstream ss;
   std::string s;
   s.resize(length);
   file.read(&s[0], length);
@@ -337,11 +466,16 @@ bool file_system::ReadBinaryFile(const std::string& name,
 }
 
 bool file_system::ReadFile(const std::string& name, std::string& result) {
+  LOG4CXX_AUTO_TRACE(logger_);
   if (!FileExists(name) || !IsAccessible(name, R_OK)) {
+    LOG4CXX_ERROR(logger_, "Not able to read file: " << name);
     return false;
   }
-
   std::ifstream file(name.c_str());
+  if (!file) {
+    LOG4CXX_ERROR(logger_, "Not able to open binary file: " << name);
+    return false;
+  }
   std::ostringstream ss;
   ss << file.rdbuf();
   result = ss.str();
@@ -349,53 +483,66 @@ bool file_system::ReadFile(const std::string& name, std::string& result) {
 }
 
 const std::string file_system::ConvertPathForURL(const std::string& path) {
-  std::string::const_iterator it_path = path.begin();
-  std::string::const_iterator it_path_end = path.end();
-
+  LOG4CXX_AUTO_TRACE(logger_);
   const std::string reserved_symbols = "!#$&'()*+,:;=?@[] ";
   size_t pos = std::string::npos;
   std::string converted_path;
 
-  for (; it_path != it_path_end; ++it_path) {
-    pos = reserved_symbols.find_first_of(*it_path);
+  for (const auto symbol : path) {
+    pos = reserved_symbols.find_first_of(symbol);
     if (pos != std::string::npos) {
       const size_t size = 100;
       char percent_value[size];
-      snprintf(percent_value, size, "%%%x", *it_path);
+      snprintf(percent_value, size, "%%%x", symbol);
       converted_path += percent_value;
     } else {
-      converted_path += *it_path;
+      converted_path += symbol;
     }
   }
   return converted_path;
 }
 
 bool file_system::CreateFile(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
+
   std::ofstream file(path);
   if (!(file.is_open())) {
+    LOG4CXX_WARN(logger_, "failed to create file: " << path);
     return false;
-  } else {
-    file.close();
-    return true;
   }
+  file.close();
+  return true;
 }
 
 time_t file_system::GetFileModificationTime(const std::string& path) {
+  LOG4CXX_AUTO_TRACE(logger_);
+
   error_code ec;
   std::time_t time = fs::last_write_time(path, ec);
   if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(logger_,
+                             "Unable to get file modification time: "
+                                 << path << " reason " << ec.message());
+
     return 0;
   }
   return time;
 }
 
 bool file_system::CopyFile(const std::string& src, const std::string& dst) {
+  LOG4CXX_AUTO_TRACE(logger_);
   if (!FileExists(src) || FileExists(dst) || !CreateFile(dst)) {
+    LOG4CXX_WARN(
+        logger_,
+        "Failed to copy file from: '" << src << "', to: '" << dst << "'");
     return false;
   }
   error_code ec;
   fs::copy_file(src, dst, ec);
   if (ec) {
+    LOG4CXX_ERROR_WITH_ERRNO(
+        logger_,
+        "Unable to copy file: '" << src << "', reason: " << ec.message());
     // something failed
     return false;
   }
@@ -403,7 +550,7 @@ bool file_system::CopyFile(const std::string& src, const std::string& dst) {
 }
 
 bool file_system::MoveFile(const std::string& src, const std::string& dst) {
-  error_code ec;
+  LOG4CXX_AUTO_TRACE(logger_);
 
   if (std::rename(src.c_str(), dst.c_str()) == 0) {
     return true;
@@ -413,9 +560,15 @@ bool file_system::MoveFile(const std::string& src, const std::string& dst) {
   // Instead, copy the file over and delete the old one
   bool success = CopyFile(src, dst);
   if (!success) {
+    LOG4CXX_ERROR(
+        logger_,
+        "Failed to copy file from: '" << src << "', to: '" << dst << "'");
+    return false;
+  }
+  success = DeleteFile(src);
+  if (!success) {
+    LOG4CXX_ERROR(logger_, "Failed to delete file '" << src << "'");
     return false;
   }
-  DeleteFile(src);
-
   return true;
 }