Improve cdc_fuse_fs and path (#2)

Improve cdc_fuse_fs and path

Improves the error handling in path so that std:error_codes are not
assumed to be of system category, and also that their messages are
displayed. Also improves debug messages in GameletComponent.

cdc_fuse_fs/main.cc

@@ -36,19 +36,36 @@ namespace {
 constexpr char kFuseFilename[] = "cdc_fuse_fs";
 constexpr char kLibFuseFilename[] = "libfuse.so";
 
-absl::StatusOr<bool> IsUpToDate(const std::string& components_arg) {
+bool IsUpToDate(const std::string& components_arg) {
   // Components are expected to reside in the same dir as the executable.
   std::string component_dir;
-  RETURN_IF_ERROR(path::GetExeDir(&component_dir));
+  absl::Status status = path::GetExeDir(&component_dir);
+  if (!status.ok()) {
+    // Should(TM) be super rare, so just log an error.
+    LOG_DEBUG("Failed to exe dir: %s", status.ToString());
+    return false;
+  }
 
   std::vector<GameletComponent> components =
       GameletComponent::FromCommandLineArgs(components_arg);
+  if (components.size() == 0) {
+    LOG_DEBUG("Invalid components arg '%s'", components_arg);
+    return false;
+  }
+
   std::vector<GameletComponent> our_components;
-  absl::Status status =
+  status = GameletComponent::Get({path::Join(component_dir, kFuseFilename),
-      GameletComponent::Get({path::Join(component_dir, kFuseFilename),
                                   path::Join(component_dir, kLibFuseFilename)},
                                  &our_components);
-  if (!status.ok() || components != our_components) {
+  if (!status.ok()) {
+    LOG_DEBUG("Failed to get component data: %s", status.ToString())
+    return false;
+  }
+
+  if (components != our_components) {
+    LOG_DEBUG("Component mismatch, args don't match ours '%s' != '%s'",
+              GameletComponent::ToCommandLineArgs(components),
+              GameletComponent::ToCommandLineArgs(our_components));
     return false;
   }
 
@@ -112,13 +129,7 @@ int main(int argc, char* argv[]) {
       cdc_ft::Log::VerbosityToLogLevel(verbosity)));
 
   // Perform up-to-date check.
-  absl::StatusOr<bool> is_up_to_date = cdc_ft::IsUpToDate(components);
+  if (!cdc_ft::IsUpToDate(components)) {
-  if (!is_up_to_date.ok()) {
-    LOG_ERROR("Failed to check file system freshness: %s",
-              is_up_to_date.status().ToString());
-    return static_cast<int>(is_up_to_date.status().code());
-  }
-  if (!*is_up_to_date) {
     printf("%s\n", cdc_ft::kFuseNotUpToDate);
     return 0;
   }

cdc_indexer/indexer.cc

@@ -334,8 +334,7 @@ void Indexer::Worker::Run() {
 absl::Status Indexer::Worker::IndexFile(const std::string& filepath) {
   std::FILE* fin = std::fopen(filepath.c_str(), "rb");
   if (!fin) {
-    return ErrnoToCanonicalStatus(
+    return ErrnoToCanonicalStatus(errno, "failed to open file '%s'", filepath);
-        errno, absl::StrFormat("failed to open file '%s'", filepath));
   }
   path::FileCloser closer(fin);
   std::fseek(fin, 0, SEEK_SET);
@@ -349,8 +348,8 @@ absl::Status Indexer::Worker::IndexFile(const std::string& filepath) {
     size_t cnt = std::fread(buf.data(), sizeof(uint8_t), buf.size(), fin);
     err = std::ferror(fin);
     if (err) {
-      return ErrnoToCanonicalStatus(
+      return ErrnoToCanonicalStatus(err, "failed to read from file '%s'",
-          err, absl::StrFormat("failed to read from file '%s'", filepath));
+                                    filepath);
     }
     if (cnt) {
       chunker.Process(buf.data(), cnt);

cdc_indexer/main.cc

@@ -273,8 +273,8 @@ absl::Status WriteResultsFile(const std::string& filepath,
   bool exists = path::FileExists(filepath);
   std::FILE* fout = std::fopen(filepath.c_str(), "a");
   if (!fout) {
-    return ErrnoToCanonicalStatus(
+    return ErrnoToCanonicalStatus(errno, "Couldn't write to file '%s'",
-        errno, absl::StrFormat("Couldn't write to file '%s'", filepath));
+                                  filepath);
   }
 
   path::FileCloser closer(fout);

common/BUILD

@@ -66,6 +66,7 @@ cc_library(
     deps = [
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/strings:str_format",
     ],
 )
 

common/dir_iter.cc

@@ -221,8 +221,8 @@ bool DirectoryIterator::Open(const std::string& path,
     impl_->PushDir(dir, path::BaseName(path));
     return true;
   }
-  impl_->SetStatus(ErrnoToCanonicalStatus(
+  impl_->SetStatus(
-      errno, absl::StrFormat("Failed to open directory '%s'", path)));
+      ErrnoToCanonicalStatus(errno, "Failed to open directory '%s'", path));
   return false;
 }
 
@@ -259,7 +259,7 @@ bool DirectoryIterator::NextEntry(DirectoryEntry* entry) {
           // Ignore access errors and proceed.
         } else {
           impl_->SetStatus(ErrnoToCanonicalStatus(
-              errno, absl::StrFormat("Failed to open directory '%s'", subdir)));
+              errno, "Failed to open directory '%s'", subdir));
           return false;
         }
       }
@@ -278,8 +278,7 @@ bool DirectoryIterator::NextEntry(DirectoryEntry* entry) {
 
   if (errno) {
     impl_->SetStatus(ErrnoToCanonicalStatus(
-        errno, absl::StrFormat("Failed to iterate over directory '%s'",
+        errno, "Failed to iterate over directory '%s'", impl_->DirsPath()));
-                               impl_->DirsPath())));
   }
   return false;
 }

common/errno_mapping.cc

@@ -16,8 +16,6 @@
 
 #include <errno.h>
 
-#include <string>
-
 namespace cdc_ft {
 
 absl::StatusCode ErrnoToCanonicalCode(int error_number) {
@@ -160,10 +158,4 @@ absl::StatusCode ErrnoToCanonicalCode(int error_number) {
   }
 }
 
-absl::Status ErrnoToCanonicalStatus(int error_number,
-                                    absl::string_view message) {
-  return absl::Status(ErrnoToCanonicalCode(error_number),
-                      absl::StrCat(message, ": ", strerror(error_number)));
-}
-
 }  // namespace cdc_ft

common/errno_mapping.h

@@ -17,8 +17,10 @@
 #ifndef COMMON_ERRNO_MAPPING_H_
 #define COMMON_ERRNO_MAPPING_H_
 
+#include <string>
+
 #include "absl/status/status.h"
-#include "absl/strings/string_view.h"
+#include "absl/strings/str_format.h"
 
 namespace cdc_ft {
 
@@ -26,8 +28,27 @@ namespace cdc_ft {
 absl::StatusCode ErrnoToCanonicalCode(int error_number);
 
 // Creates a status by converting the errno |error_number| to an absl code.
+template <typename... Args>
 absl::Status ErrnoToCanonicalStatus(int error_number,
-                                    absl::string_view message);
+                                    const absl::FormatSpec<Args...>& format,
+                                    Args... args) {
+  if (error_number == 0) return absl::OkStatus();
+  std::string msg = absl::StrFormat(format, args...);
+  return absl::Status(ErrnoToCanonicalCode(error_number),
+                      absl::StrCat(msg, ": ", strerror(error_number)));
+}
+
+template <typename... Args>
+absl::Status ErrorCodeToCanonicalStatus(const std::error_code& code,
+                                        const absl::FormatSpec<Args...>& format,
+                                        Args... args) {
+  if (!code) return absl::OkStatus();
+  std::string msg = absl::StrFormat(format, args...);
+  absl::StatusCode absl_code = code.category() == std::system_category()
+                                   ? ErrnoToCanonicalCode(code.value())
+                                   : absl::StatusCode::kUnknown;
+  return absl::Status(absl_code, absl::StrCat(msg, ": ", code.message()));
+}
 
 }  // namespace cdc_ft
 

common/path.cc

@@ -470,8 +470,7 @@ absl::StatusOr<FILE*> OpenFile(const std::string& path, const char* mode) {
 #endif
   if (!file) {
     int err = errno;
-    return ErrnoToCanonicalStatus(
+    return ErrnoToCanonicalStatus(err, "Failed to open file '%s'", path);
-        err, absl::StrFormat("Failed to open file '%s'", path));
   }
   return file;
 }
@@ -914,8 +913,7 @@ absl::Status GetStats(const std::string& path, Stats* stats) {
   if (!result) {
     int err = errno;
     *stats = Stats();
-    return ErrnoToCanonicalStatus(err,
+    return ErrnoToCanonicalStatus(err, "Failed to stat '%s'", path);
-                                  absl::StrFormat("Failed to stat '%s'", path));
   }
 
   stats->mode = os_stats.st_mode;
@@ -973,14 +971,9 @@ absl::Status CreateSymlink(const std::string& target,
     std::filesystem::create_directory_symlink(target_u8, link_path_u8,
                                               error_code);
   }
-  if (error_code) {
+  return ErrorCodeToCanonicalStatus(
-    assert(error_code.category() == std::system_category());
+      error_code, "Failed to create symlink '%s' with target '%s'", link_path,
-    return ErrnoToCanonicalStatus(
+      target);
-        error_code.value(),
-        absl::StrFormat("Failed to create symlink '%s' with target '%s'",
-                        link_path, target));
-  }
-  return absl::OkStatus();
 }
 
 absl::StatusOr<std::string> GetSymlinkTarget(const std::string& link_path) {
@@ -989,9 +982,8 @@ absl::StatusOr<std::string> GetSymlinkTarget(const std::string& link_path) {
   std::filesystem::path symlink_target =
       std::filesystem::read_symlink(link_path_u8, error_code);
   if (error_code) {
-    return ErrnoToCanonicalStatus(
+    return ErrorCodeToCanonicalStatus(error_code, "Failed to read symlink '%s'",
-        error_code.value(),
+                                      link_path);
-        absl::StrFormat("Failed to read symlink '%s'", link_path));
   }
   return symlink_target.u8string();
 }
@@ -1014,26 +1006,16 @@ bool Exists(const std::string& path) {
 absl::Status CreateDir(const std::string& path) {
   std::error_code error_code;
   std::filesystem::create_directory(std::filesystem::u8path(path), error_code);
-  if (error_code) {
+  return ErrorCodeToCanonicalStatus(error_code,
-    assert(error_code.category() == std::system_category());
+                                    "Failed to create directory '%s'", path);
-    return ErrnoToCanonicalStatus(
-        error_code.value(),
-        absl::StrFormat("Failed to create directory '%s'", path));
-  }
-  return absl::OkStatus();
 }
 
 absl::Status CreateDirRec(const std::string& path) {
   std::error_code error_code;
   std::filesystem::create_directories(std::filesystem::u8path(path),
                                       error_code);
-  if (error_code) {
+  return ErrorCodeToCanonicalStatus(error_code,
-    assert(error_code.category() == std::system_category());
+                                    "Failed to create directory '%s'", path);
-    return ErrnoToCanonicalStatus(
-        error_code.value(),
-        absl::StrFormat("Failed to create directory '%s'", path));
-  }
-  return absl::OkStatus();
 }
 
 absl::Status RenameFile(const std::string& from_path,

manifest/content_id.cc

@@ -78,7 +78,7 @@ bool ContentId::FromHexString(const std::string& str,
   std::string* hash = content_id->mutable_blake3_sum_160();
   hash->clear();
   hash->reserve(kHashSize);
-  for (int n = 0; n < str.size(); n += 2) {
+  for (size_t n = 0; n < str.size(); n += 2) {
     int high = HexToInt(str[n]);
     int low = HexToInt(str[n + 1]);
     if (high == -1 || low == -1) {