Add a Filesystem interface for testability.

Not immediately adding any tests that take advantage of it.
The new storage schema should handle ENOSPC correctly, and this will aid in
testing it.

src/filesystem.cc

@@ -55,30 +55,109 @@
 
 namespace moonfire_nvr {
 
-bool DirForEach(const std::string &dir_path,
+namespace {
-                std::function<IterationControl(const dirent *)> fn,
+
-                std::string *error_message) {
+class RealFile : public File {
-  DIR *owned_dir = opendir(dir_path.c_str());
+ public:
-  if (owned_dir == nullptr) {
+  explicit RealFile(int fd) : fd_(fd) {}
-    int err = errno;
+  RealFile(const RealFile &) = delete;
-    *error_message =
+  void operator=(const RealFile &) = delete;
-        StrCat("Unable to examine ", dir_path, ": ", strerror(err));
+
-    return false;
+  ~RealFile() final { Close(); }
-  }
+
-  struct dirent *ent;
+  int Close() final {
-  while (errno = 0, (ent = readdir(owned_dir)) != nullptr) {
+    if (fd_ < 0) {
-    if (fn(ent) == IterationControl::kBreak) {
+      return 0;
-      closedir(owned_dir);
-      return true;
     }
+    int ret;
+    while ((ret = close(fd_)) != 0 && errno == EINTR)
+      ;
+    if (ret != 0) {
+      return errno;
+    }
+    fd_ = -1;
+    return 0;
   }
-  int err = errno;
+
-  closedir(owned_dir);
+  int Write(re2::StringPiece *data) final {
-  if (err != 0) {
+    if (fd_ < 0) {
-    *error_message = StrCat("readdir failed: ", strerror(err));
+      return EBADF;
-    return false;
+    }
+    ssize_t ret;
+    while ((ret = write(fd_, data->data(), data->size())) == -1 &&
+           errno == EINTR)
+      ;
+    if (ret < 0) {
+      return errno;
+    }
+    data->remove_prefix(ret);
+    return 0;
   }
-  return true;
+
+ private:
+  int fd_ = -1;
+};
+
+class RealFilesystem : public Filesystem {
+ public:
+  bool DirForEach(const char *dir_path,
+                  std::function<IterationControl(const dirent *)> fn,
+                  std::string *error_message) final {
+    DIR *owned_dir = opendir(dir_path);
+    if (owned_dir == nullptr) {
+      int err = errno;
+      *error_message =
+          StrCat("Unable to examine ", dir_path, ": ", strerror(err));
+      return false;
+    }
+    struct dirent *ent;
+    while (errno = 0, (ent = readdir(owned_dir)) != nullptr) {
+      if (fn(ent) == IterationControl::kBreak) {
+        closedir(owned_dir);
+        return true;
+      }
+    }
+    int err = errno;
+    closedir(owned_dir);
+    if (err != 0) {
+      *error_message = StrCat("readdir failed: ", strerror(err));
+      return false;
+    }
+    return true;
+  }
+
+  int Open(const char *path, int flags, std::unique_ptr<File> *f) final {
+    return Open(path, flags, 0, f);
+  }
+
+  int Open(const char *path, int flags, mode_t mode,
+           std::unique_ptr<File> *f) final {
+    int ret = open(path, flags, mode);
+    if (ret < 0) {
+      return errno;
+    }
+    f->reset(new RealFile(ret));
+    return 0;
+  }
+
+  int Mkdir(const char *path, mode_t mode) final {
+    return (mkdir(path, mode) < 0) ? errno : 0;
+  }
+
+  int Rmdir(const char *path) final { return (rmdir(path) < 0) ? errno : 0; }
+
+  int Stat(const char *path, struct stat *buf) final {
+    return (stat(path, buf) < 0) ? errno : 0;
+  }
+
+  int Unlink(const char *path) final { return (unlink(path) < 0) ? errno : 0; }
+};
+
+}  // namespace
+
+Filesystem *GetRealFilesystem() {
+  static Filesystem *real_filesystem = new RealFilesystem;
+  return real_filesystem;
 }
 
 }  // namespace moonfire_nvr

src/filesystem.h

@@ -38,8 +38,8 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
+#include <memory>
 #include <functional>
-#include <iostream>
 #include <string>
 
 #include <event2/buffer.h>
@@ -55,14 +55,57 @@ enum class IterationControl {
   kBreak      // indicates the caller should terminate the loop with success.
 };
 
-// Execute |fn| for each directory entry in |dir_path|, stopping early
+// Represents an open file. All methods but Close() are thread-safe.
-// (successfully) if the callback returns IterationControl::kBreak.
+class File {
-//
+ public:
-// On success, returns true.
+  // Close the file, ignoring the result.
-// On failure, returns false and updates |error_msg|.
+  virtual ~File() {}
-bool DirForEach(const std::string &dir_path,
+
-                std::function<IterationControl(const dirent *)> fn,
+  // Close the file, returning 0 on success or errno>0 on failure.
-                std::string *error_msg);
+  // Already closed is considered a success.
+  virtual int Close() = 0;
+
+  // Write to the file, returning 0 on success or errno>0 on failure.
+  // On success, data->remove_prefix will be called with the written bytes.
+  virtual int Write(re2::StringPiece *data) = 0;
+};
+
+// Interface to the local filesystem. There's typically one per program,
+// but it's an abstract class for testability. Thread-safe.
+class Filesystem {
+ public:
+  virtual ~Filesystem() {}
+
+  // Execute |fn| for each directory entry in |dir_path|, stopping early
+  // (successfully) if the callback returns IterationControl::kBreak.
+  //
+  // On success, returns true.
+  // On failure, returns false and updates |error_msg|.
+  virtual bool DirForEach(const char *dir_path,
+                          std::function<IterationControl(const dirent *)> fn,
+                          std::string *error_msg) = 0;
+
+  // open() the specified path, returning 0 on success or errno>0 on failure.
+  // On success, |f| is populated with an open file.
+  virtual int Open(const char *path, int flags, std::unique_ptr<File> *f) = 0;
+  virtual int Open(const char *path, int flags, mode_t mode,
+                   std::unique_ptr<File> *f) = 0;
+
+  // mkdir() the specified path, returning 0 on success or errno>0 on failure.
+  virtual int Mkdir(const char *path, mode_t mode) = 0;
+
+  // rmdir() the specified path, returning 0 on success or errno>0 on failure.
+  virtual int Rmdir(const char *path) = 0;
+
+  // stat() the specified path, returning 0 on success or errno>0 on failure.
+  virtual int Stat(const char *path, struct stat *buf) = 0;
+
+  // unlink() the specified file, returning 0 on success or errno>0 on failure.
+  virtual int Unlink(const char *path) = 0;
+};
+
+// Get the (singleton) real filesystem, which is never deleted.
+Filesystem *GetRealFilesystem();
 
 }  // namespace moonfire_nvr
 

src/moonfire-nvr-test.cc

@@ -73,6 +73,7 @@ class FileManagerTest : public testing::Test {
  protected:
   FileManagerTest() {
     test_dir_ = PrepareTempDirOrDie("moonfire-nvr-file-manager");
+    env_.fs = GetRealFilesystem();
   }
 
   std::vector<std::string> GetFilenames(const FileManager &mgr) {
@@ -83,12 +84,13 @@ class FileManagerTest : public testing::Test {
     return out;
   }
 
+  Environment env_;
   std::string test_dir_;
 };
 
 TEST_F(FileManagerTest, InitWithNoDirectory) {
   std::string subdir = test_dir_ + "/" + "subdir";
-  FileManager manager("foo", subdir, 0);
+  FileManager manager("foo", subdir, 0, &env_);
 
   // Should succeed.
   std::string error_message;
@@ -124,7 +126,7 @@ TEST_F(FileManagerTest, InitAndRotateWithExistingFiles) {
   WriteFileOrDie(test_dir_ + "/2.mp4", "123");
   WriteFileOrDie(test_dir_ + "/3.mp4", "12345");
   WriteFileOrDie(test_dir_ + "/other", "1234567");
-  FileManager manager("foo", test_dir_, 8);
+  FileManager manager("foo", test_dir_, 8, &env_);
 
   // Should succeed.
   std::string error_message;
@@ -145,6 +147,7 @@ class StreamTest : public testing::Test {
     test_dir_ = PrepareTempDirOrDie("moonfire-nvr-stream-copier");
     env_.clock = &clock_;
     env_.video_source = &video_source_;
+    env_.fs = GetRealFilesystem();
     clock_.Sleep({1430006400, 0});  // 2016-04-26 00:00:00 UTC
 
     config_.set_base_path(test_dir_);

src/moonfire-nvr.cc

@@ -61,15 +61,18 @@ const char kFilenameSuffix[] = ".mp4";
 
 }  // namespace
 FileManager::FileManager(const std::string &short_name, const std::string &path,
-                         uint64_t byte_limit)
+                         uint64_t byte_limit, Environment *env)
-    : short_name_(short_name), path_(path), byte_limit_(byte_limit) {}
+    : short_name_(short_name),
+      path_(path),
+      byte_limit_(byte_limit),
+      env_(env) {}
 
 bool FileManager::Init(std::string *error_message) {
   // Create the directory if it doesn't exist.
   // If the path exists, assume it is a valid directory.
-  if (mkdir(path_.c_str(), 0700) < 0 && errno != EEXIST) {
+  int ret = env_->fs->Mkdir(path_.c_str(), 0700);
-    int err = errno;
+  if (ret != 0 && ret != EEXIST) {
-    *error_message = StrCat("Unable to create ", path_, ": ", strerror(err));
+    *error_message = StrCat("Unable to create ", path_, ": ", strerror(ret));
     return false;
   }
 
@@ -94,7 +97,7 @@ bool FileManager::Init(std::string *error_message) {
     return IterationControl::kContinue;
   };
 
-  if (!DirForEach(path_, file_fn, error_message)) {
+  if (!env_->fs->DirForEach(path_.c_str(), file_fn, error_message)) {
     return false;
   }
 
@@ -115,18 +118,18 @@ bool FileManager::Rotate(std::string *error_message) {
     // won't return prematurely.
     mu_.unlock();
     string fpath = StrCat(path_, "/", filename);
-    if (unlink(fpath.c_str()) == 0) {
+    int ret = env_->fs->Unlink(fpath.c_str());
+    if (ret == 0) {
       LOG(INFO) << short_name_ << ": Deleted " << filename << " to reclaim "
                 << size << " bytes.";
-    } else if (errno == ENOENT) {
+    } else if (ret == ENOENT) {
       // This may have happened due to a racing Rotate() call.
       // In any case, the file is gone, so proceed to mark it as such.
       LOG(INFO) << short_name_ << ": File " << filename
                 << " was already deleted.";
     } else {
-      int err = errno;
       *error_message =
-          StrCat("unlink failed on ", filename, ": ", strerror(err));
+          StrCat("unlink failed on ", filename, ": ", strerror(ret));
 
       return false;
     }
@@ -154,9 +157,9 @@ bool FileManager::AddFile(const std::string &filename,
                           std::string *error_message) {
   struct stat buf;
   string fpath = StrCat(path_, "/", filename);
-  if (lstat(fpath.c_str(), &buf) != 0) {
+  int ret = env_->fs->Stat(fpath.c_str(), &buf);
-    int err = errno;
+  if (ret != 0) {
-    *error_message = StrCat("lstat on ", fpath, " failed: ", strerror(err));
+    *error_message = StrCat("stat on ", fpath, " failed: ", strerror(ret));
     return false;
   }
   VLOG(1) << short_name_ << ": adding file " << filename << " size "
@@ -466,6 +469,7 @@ void Stream::HttpCallbackForFile(evhttp_request *req, const string &filename) {
 Nvr::Nvr() {
   env_.clock = GetRealClock();
   env_.video_source = GetRealVideoSource();
+  env_.fs = GetRealFilesystem();
 }
 
 Nvr::~Nvr() {

src/moonfire-nvr.h

@@ -46,6 +46,7 @@
 #include <event2/http.h>
 
 #include "config.pb.h"
+#include "filesystem.h"
 #include "ffmpeg.h"
 #include "time.h"
 
@@ -72,6 +73,7 @@ class ShutdownSignal {
 struct Environment {
   WallClock *clock = nullptr;
   VideoSource *video_source = nullptr;
+  Filesystem *fs = nullptr;
 };
 
 // Delete old ".mp4" files within a specified directory, keeping them within a
@@ -88,7 +90,7 @@ class FileManager {
 
   // |short_name| will be prepended to log messages.
   FileManager(const std::string &short_name, const std::string &path,
-              uint64_t byte_limit);
+              uint64_t byte_limit, Environment *env);
   FileManager(const FileManager &) = delete;
   FileManager &operator=(const FileManager &) = delete;
 
@@ -120,6 +122,7 @@ class FileManager {
   const std::string short_name_;
   const std::string path_;
   const uint64_t byte_limit_;
+  Environment *const env_;
 
   mutable std::mutex mu_;
   std::map<std::string, struct stat> files_;
@@ -132,13 +135,14 @@ class FileManager {
 class Stream {
  public:
   Stream(const ShutdownSignal *signal, const moonfire_nvr::Config &config,
-         const Environment *env, const moonfire_nvr::Camera &camera)
+         Environment *const env, const moonfire_nvr::Camera &camera)
       : signal_(signal),
         env_(env),
         camera_path_(config.base_path() + "/" + camera.short_name()),
         rotate_interval_(config.rotate_sec()),
         camera_(camera),
-        manager_(camera_.short_name(), camera_path_, camera.retain_bytes()) {}
+        manager_(camera_.short_name(), camera_path_, camera.retain_bytes(),
+                 env) {}
   Stream(const Stream &) = delete;
   Stream &operator=(const Stream &) = delete;
 

src/testutil.cc

@@ -34,56 +34,57 @@
 
 #include <dirent.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <string.h>
+#include <sys/stat.h>
 #include <sys/types.h>
 
-#include <fstream>
-
 #include <glog/logging.h>
 
 #include "filesystem.h"
+#include "string.h"
 
 namespace moonfire_nvr {
 
 namespace {
 
-bool DeleteChildrenRecursively(const std::string &dirname,
+bool DeleteChildrenRecursively(const char *dirname, std::string *error_msg) {
-                               std::string *error_msg) {
   bool ok = true;
   auto fn = [&dirname, &ok, error_msg](const struct dirent *ent) {
     std::string name(ent->d_name);
-    std::string path = dirname + "/" + name;
+    std::string path = StrCat(dirname, "/", name);
     if (name == "." || name == "..") {
       return IterationControl::kContinue;
     }
     bool is_dir = (ent->d_type == DT_DIR);
     if (ent->d_type == DT_UNKNOWN) {
       struct stat buf;
-      PCHECK(stat(path.c_str(), &buf) == 0) << path;
+      int ret = GetRealFilesystem()->Stat(path.c_str(), &buf);
+      CHECK_EQ(ret, 0) << path << ": " << strerror(ret);
       is_dir = S_ISDIR(buf.st_mode);
     }
     if (is_dir) {
-      ok = ok && DeleteChildrenRecursively(path, error_msg);
+      ok = ok && DeleteChildrenRecursively(path.c_str(), error_msg);
       if (!ok) {
         return IterationControl::kBreak;
       }
-      if (rmdir(path.c_str()) != 0) {
+      int ret = GetRealFilesystem()->Rmdir(path.c_str());
-        *error_msg =
+      if (ret != 0) {
-            std::string("rmdir failed on ") + path + ": " + strerror(errno);
+        *error_msg = StrCat("rmdir failed on ", path, ": ", strerror(ret));
         ok = false;
         return IterationControl::kBreak;
       }
     } else {
-      if (unlink(path.c_str()) != 0) {
+      int ret = GetRealFilesystem()->Unlink(path.c_str());
-        *error_msg =
+      if (ret != 0) {
-            std::string("unlink failed on ") + path + ": " + strerror(errno);
+        *error_msg = StrCat("unlink failed on ", path, ": ", strerror(ret));
         ok = false;
         return IterationControl::kBreak;
       }
     }
     return IterationControl::kContinue;
   };
-  if (!DirForEach(dirname, fn, error_msg)) {
+  if (!GetRealFilesystem()->DirForEach(dirname, fn, error_msg)) {
     return false;
   }
   return ok;
@@ -92,22 +93,27 @@ bool DeleteChildrenRecursively(const std::string &dirname,
 }  // namespace
 
 std::string PrepareTempDirOrDie(const std::string &test_name) {
-  std::string dirname = std::string("/tmp/test.") + test_name;
+  std::string dirname = StrCat("/tmp/test.", test_name);
-  int res = mkdir(dirname.c_str(), 0700);
+  int ret = GetRealFilesystem()->Mkdir(dirname.c_str(), 0700);
-  if (res != 0) {
+  if (ret != 0) {
-    int err = errno;
+    CHECK_EQ(ret, EEXIST) << "mkdir failed: " << strerror(ret);
-    CHECK_EQ(err, EEXIST) << "mkdir failed: " << strerror(err);
     std::string error_msg;
-    CHECK(DeleteChildrenRecursively(dirname, &error_msg)) << error_msg;
+    CHECK(DeleteChildrenRecursively(dirname.c_str(), &error_msg)) << error_msg;
   }
   return dirname;
 }
 
-void WriteFileOrDie(const std::string &path, const std::string &contents) {
+void WriteFileOrDie(const std::string &path, re2::StringPiece contents) {
-  std::ofstream f(path);
+  std::unique_ptr<File> f;
-  f << contents;
+  int ret = GetRealFilesystem()->Open(path.c_str(),
-  f.close();
+                                      O_WRONLY | O_CREAT | O_TRUNC, 0600, &f);
-  CHECK(!f.fail()) << "failed to write: " << path;
+  CHECK_EQ(ret, 0) << "open " << path << ": " << strerror(ret);
+  while (!contents.empty()) {
+    ret = f->Write(&contents);
+    CHECK_EQ(ret, 0) << "write " << path << ": " << strerror(ret);
+  }
+  ret = f->Close();
+  CHECK_EQ(ret, 0) << "close " << path << ": " << strerror(ret);
 }
 
 }  // namespace moonfire_nvr

src/testutil.h

@@ -35,6 +35,7 @@
 
 #include <glog/logging.h>
 #include <gmock/gmock.h>
+#include <re2/stringpiece.h>
 
 namespace moonfire_nvr {
 
@@ -43,7 +44,7 @@ namespace moonfire_nvr {
 std::string PrepareTempDirOrDie(const std::string &test_name);
 
 // Write the given file contents to the given path, or die.
-void WriteFileOrDie(const std::string &path, const std::string &contents);
+void WriteFileOrDie(const std::string &path, re2::StringPiece contents);
 
 // A scoped log sink for testing that the right log messages are sent.
 // Modelled after glog's "mock-log.h", which is not exported.