Add logging output

Signed-off-by: Tim Serong <[email protected]>

src/checks.cc

@@ -36,11 +36,30 @@ void Check::fix() {
 
 void Check::show() {
   for (std::shared_ptr<Fix> fix : fixes) {
-    std::cout << std::string(*fix) << std::endl;
+    // TODO: figure out how to handle embedded newlines (see comment in
+    // MetadataIntegrityFix::to_string())
+    std::cout << "  " << std::string(*fix) << std::endl;
   }
 }
 
-bool run_checks(const std::filesystem::path& path, bool should_fix) {
+bool Check::check(Check::LogLevel log_level) {
+  check_log_level = log_level;
+  if (log_level > SILENT) {
+    std::cout << "Checking " << check_name << "..." << std::endl;
+  }
+  return do_check();
+}
+
+void Check::log_verbose(const std::string& msg) const {
+  if (check_log_level == VERBOSE) {
+    std::cout << "  " << msg << std::endl;
+  }
+}
+
+bool run_checks(
+    const std::filesystem::path& path, Check::LogLevel log_level,
+    bool should_fix
+) {
   bool all_checks_passed = true;
 
   std::vector<std::shared_ptr<Check>> checks;
@@ -51,8 +70,10 @@ bool run_checks(const std::filesystem::path& path, bool should_fix) {
   checks.emplace_back(std::make_shared<ObjectIntegrityCheck>(path));
 
   for (std::shared_ptr<Check> check : checks) {
-    bool this_check_passed = check->check();
-    check->show();
+    bool this_check_passed = check->check(log_level);
+    if (log_level > Check::LogLevel::SILENT) {
+      check->show();
+    }
     if (!this_check_passed) {
       all_checks_passed = false;
       if (check->is_fatal()) {
@@ -68,8 +89,8 @@ bool run_checks(const std::filesystem::path& path, bool should_fix) {
     }
   }
 
-  if (all_checks_passed) {
-    std::cout << "Everything's cool." << std::endl;
+  if (all_checks_passed && log_level > Check::LogLevel::SILENT) {
+    std::cout << "All checks passed." << std::endl;
   }
   return all_checks_passed;
 }

src/checks.h

@@ -44,19 +44,30 @@ class Fix {
 class Check {
  protected:
   std::vector<std::shared_ptr<Fix>> fixes;
+  const std::string check_name;
   enum Fatality { FATAL, NONFATAL } fatality;
   const std::filesystem::path& root_path;
+  virtual bool do_check() = 0;
+  void log_verbose(const std::string& msg) const;
 
  public:
-  Check(Fatality f, const std::filesystem::path& path)
-      : fatality(f), root_path(path) {}
+  Check(const std::string& name, Fatality f, const std::filesystem::path& path)
+      : check_name(name), fatality(f), root_path(path) {}
   virtual ~Check(){};
-  virtual bool check() = 0;
+  enum LogLevel { SILENT, NORMAL, VERBOSE };
+  bool check(LogLevel log_level = NORMAL);
   bool is_fatal() { return fatality == FATAL; }
   void fix();
   void show();
+
+ private:
+  // Transient, only valid inside check() so that log_verbose() works
+  LogLevel check_log_level;
 };
 
-bool run_checks(const std::filesystem::path& path, bool should_fix);
+bool run_checks(
+    const std::filesystem::path& path, Check::LogLevel log_level,
+    bool should_fix
+);
 
 #endif  // FSCK_SFS_SRC_CHECKS_H__

src/checks/metadata_integrity.cc

@@ -32,20 +32,23 @@ void MetadataIntegrityFix::fix() {}
 std::string MetadataIntegrityFix::to_string() const {
   std::string msg("Database integrity check failed:");
   for (auto& s : errors) {
-    msg += "\n- " + s;
+    // note: this indent needs to match the indent in Check::show()
+    // TODO: figure out if we can drop the indent here and make Check::show()
+    // handle it automatically when it sees newlines.
+    msg += "\n  - " + s;
   }
   return msg;
 }
 
 MetadataIntegrityCheck::MetadataIntegrityCheck(const std::filesystem::path& path
 )
-    : Check(FATAL, path) {
+    : Check("metadata integrity", FATAL, path) {
   metadata = std::make_unique<Database>(root_path / "s3gw.db");
 }
 
 MetadataIntegrityCheck::~MetadataIntegrityCheck() {}
 
-bool MetadataIntegrityCheck::check() {
+bool MetadataIntegrityCheck::do_check() {
   std::vector<std::string> errors;
   auto callback = [](void* arg, int num_columns, char** column_data, char**) {
     assert(num_columns == 1);

src/checks/metadata_integrity.h

@@ -40,10 +40,12 @@ class MetadataIntegrityCheck : public Check {
  private:
   std::unique_ptr<Database> metadata;
 
+ protected:
+  virtual bool do_check() override;
+
  public:
   MetadataIntegrityCheck(const std::filesystem::path& path);
   virtual ~MetadataIntegrityCheck() override;
-  virtual bool check() override;
 };
 
 #endif  // FSCK_SFS_SRC_CHECKS_METADATA_INTEGRITY_H__

src/checks/metadata_schema_version.cc

@@ -36,13 +36,13 @@ std::string MetadataSchemaVersionFix::to_string() const {
 MetadataSchemaVersionCheck::MetadataSchemaVersionCheck(
     const std::filesystem::path& path
 )
-    : Check(FATAL, path) {
+    : Check("metadata schema version", FATAL, path) {
   metadata = std::make_unique<Database>(root_path / "s3gw.db");
 }
 
 MetadataSchemaVersionCheck::~MetadataSchemaVersionCheck() {}
 
-bool MetadataSchemaVersionCheck::check() {
+bool MetadataSchemaVersionCheck::do_check() {
   std::string query = "PRAGMA user_version;";
   int version = 0;
   int rc = 0;
@@ -55,9 +55,12 @@ bool MetadataSchemaVersionCheck::check() {
   if (rc == SQLITE_ROW && sqlite3_column_count(stm) > 0) {
     version = sqlite3_column_int(stm, 0);
   } else {
+    // FIXME: this should probably raise an exception (at the
+    // very least we shouldn't be printing anything directly here)
     std::cout << "Error" << sqlite3_errmsg(metadata->handle) << std::endl;
   }
   sqlite3_finalize(stm);
+  log_verbose("Got schema version " + std::to_string(version));
   if (version != EXPECTED_METADATA_SCHEMA_VERSION) {
     fixes.emplace_back(
         std::make_shared<MetadataSchemaVersionFix>(root_path, version)

src/checks/metadata_schema_version.h

@@ -44,10 +44,12 @@ class MetadataSchemaVersionCheck : public Check {
  private:
   std::unique_ptr<Database> metadata;
 
+ protected:
+  virtual bool do_check() override;
+
  public:
   MetadataSchemaVersionCheck(const std::filesystem::path& path);
   virtual ~MetadataSchemaVersionCheck() override;
-  virtual bool check() override;
 };
 
 const int EXPECTED_METADATA_SCHEMA_VERSION = 4;

src/checks/object_integrity.cc

@@ -19,6 +19,7 @@
 #include <sqlite3.h>
 
 #include <filesystem>
+#include <iostream>
 
 ObjectIntegrityFix::ObjectIntegrityFix(
     const std::filesystem::path& root, const std::filesystem::path& object,
@@ -34,13 +35,13 @@ std::string ObjectIntegrityFix::to_string() const {
 }
 
 ObjectIntegrityCheck::ObjectIntegrityCheck(const std::filesystem::path& path)
-    : Check(NONFATAL, path) {
+    : Check("object integrity", NONFATAL, path) {
   metadata = std::make_unique<Database>(root_path / "s3gw.db");
 }
 
 ObjectIntegrityCheck::~ObjectIntegrityCheck() {}
 
-bool ObjectIntegrityCheck::check() {
+bool ObjectIntegrityCheck::do_check() {
   int fail_count = 0;
   // TODO: is there any sense in implementing this as a separate check
   // class as I have it here?  Why not just slide it in as part of the
@@ -63,6 +64,7 @@ bool ObjectIntegrityCheck::check() {
     std::string uuid{
         reinterpret_cast<const char*>(sqlite3_column_text(stm, 0))};
     std::string id{reinterpret_cast<const char*>(sqlite3_column_text(stm, 1))};
+    log_verbose("Checking object " + id + " (uuid: " + uuid + ")");
     id.append(".v");
     std::string checksum{
         reinterpret_cast<const char*>(sqlite3_column_text(stm, 2))};

src/checks/object_integrity.h

@@ -45,10 +45,12 @@ class ObjectIntegrityCheck : public Check {
  private:
   std::unique_ptr<Database> metadata;
 
+ protected:
+  virtual bool do_check() override;
+
  public:
   ObjectIntegrityCheck(const std::filesystem::path& path);
   virtual ~ObjectIntegrityCheck() override;
-  virtual bool check() override;
 };
 
 #endif  // FSCK_SFS_SRC_CHECKS_OBJECT_INTEGRITY_H__

src/checks/orphaned_metadata.cc

@@ -37,13 +37,13 @@ std::string OrphanedMetadataFix::to_string() const {
 }
 
 OrphanedMetadataCheck::OrphanedMetadataCheck(const std::filesystem::path& path)
-    : Check(NONFATAL, path) {
+    : Check("orphaned metadata", NONFATAL, path) {
   metadata = std::make_unique<Database>(root_path / "s3gw.db");
 }
 
 OrphanedMetadataCheck::~OrphanedMetadataCheck() {}
 
-bool OrphanedMetadataCheck::check() {
+bool OrphanedMetadataCheck::do_check() {
   int orphan_count = 0;
   // TODO: Should we do a join here with the objects table in order
   // to get bucket id and object name for display purposes if something
@@ -64,6 +64,7 @@ bool OrphanedMetadataCheck::check() {
     std::string uuid{
         reinterpret_cast<const char*>(sqlite3_column_text(stm, 0))};
     std::string id{reinterpret_cast<const char*>(sqlite3_column_text(stm, 1))};
+    log_verbose("Checking object " + id + " (uuid: " + uuid + ")");
     id.append(".v");
     // first/second/fname logic lifted from sfs's UUIDPath class
     std::filesystem::path first = uuid.substr(0, 2);

src/checks/orphaned_metadata.h

@@ -49,10 +49,12 @@ class OrphanedMetadataCheck : public Check {
  private:
   std::unique_ptr<Database> metadata;
 
+ protected:
+  virtual bool do_check() override;
+
  public:
   OrphanedMetadataCheck(const std::filesystem::path& path);
   virtual ~OrphanedMetadataCheck() override;
-  virtual bool check() override;
 };
 
 #endif  // FSCK_SFS_SRC_CHECKS_ORPHANED_METADATA_H__

src/checks/orphaned_objects.cc

@@ -77,13 +77,13 @@ std::string UnexpectedFileFix::to_string() const {
 }
 
 OrphanedObjectsCheck::OrphanedObjectsCheck(const std::filesystem::path& path)
-    : Check(NONFATAL, path) {
+    : Check("orphaned objects", NONFATAL, path) {
   metadata = std::make_unique<Database>(root_path / "s3gw.db");
 }
 
 OrphanedObjectsCheck::~OrphanedObjectsCheck() {}
 
-bool OrphanedObjectsCheck::check() {
+bool OrphanedObjectsCheck::do_check() {
   int orphan_count = 0;
   std::stack<std::filesystem::path> stack;
 
@@ -109,6 +109,8 @@ bool OrphanedObjectsCheck::check() {
         std::filesystem::path rel =
             std::filesystem::relative(cwd / entry.path(), root_path);
 
+        log_verbose("Checking file " + rel.string());
+
         std::filesystem::path uuid_path =
             std::filesystem::relative(cwd, root_path);
         std::string uuid = uuid_path.string();
@@ -158,11 +160,13 @@ bool OrphanedObjectsCheck::check() {
             } else {
               std::cout << "This can't happen" << std::endl;
               // TODO: You sure about that bro?
+              // FIXME: Throw an exception here rather than printing to stdout
             }
             sqlite3_finalize(stm);
           } else {
             std::cout << "This shouldn't happen" << std::endl;
             // TODO: What?  Seriously?  Do better with the error handling.
+            // FIXME: Throw an exception here rather than printing to stdout
           }
         } else {
           // This is something else

src/checks/orphaned_objects.h

@@ -65,10 +65,12 @@ class OrphanedObjectsCheck : public Check {
  private:
   std::unique_ptr<Database> metadata;
 
+ protected:
+  virtual bool do_check() override;
+
  public:
   OrphanedObjectsCheck(const std::filesystem::path& path);
   virtual ~OrphanedObjectsCheck() override;
-  virtual bool check() override;
 };
 
 #endif  // FSCK_SFS_SRC_CHECKS_ORPHANED_OBJECTS_H__

src/main.cc

@@ -47,6 +47,8 @@ int main(int argc, char* argv[]) {
       "don't return an error if the volume is uninitialized")(
         "path,p", boost::program_options::value<std::string>(), "path to check"
     )("quiet,q", "run silently")("verbose,v", "more verbose output");
+    // TODO: it's currently possible to specify both quiet and
+    // verbose at the same time.  This is a bit ridiculous.
 
     boost::program_options::positional_options_description p;
     p.add("path", -1);
@@ -95,5 +97,14 @@ int main(int argc, char* argv[]) {
     );
   }
 
-  return run_checks(path_root, options_map.count("fix") > 0) ? 0 : 1;
+  Check::LogLevel log_level = Check::LogLevel::NORMAL;
+  if (options_map.count("quiet") > 0) {
+    // TODO: fix discrepancy between terms "quiet" and "silent"?
+    log_level = Check::LogLevel::SILENT;
+  }
+  if (options_map.count("verbose") > 0) {
+    log_level = Check::LogLevel::VERBOSE;
+  }
+
+  return run_checks(path_root, log_level, options_map.count("fix") > 0) ? 0 : 1;
 }