Rework file_system

2 files changed, 228 insertions, 75 deletions

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;
 }