Sanify sample directory references.

Before, I had a gross hardcoded path in moonfire-db.cc + a hacky
Recording::sample_file_path (which is StrCat(sample_file_dir, "/", uuid),
essentially). Now, things expect to take a File* to the sample file directory
and use openat(2). Several things had to change in the process:

* RealFileSlice now takes a File* dir.
* File has an Open that returns an fd (for RealFileSlice's benefit).
* BuildMp4 now is in WebInterface rather than MoonfireDatabase. The latter
  only manages the SQLite database, so it shouldn't know anything about the
  sample file directory.

src/filesystem.cc

@@ -79,10 +79,23 @@ class RealFile : public File {
     return 0;
   }
 
+  int Open(const char *path, int flags, int *fd) final {
+    return Open(path, flags, 0, fd);
+  }
+
   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, int *fd) final {
+    int ret = openat(fd_, path, flags, mode);
+    if (ret < 0) {
+      return errno;
+    }
+    *fd = ret;
+    return 0;
+  }
+
   int Open(const char *path, int flags, mode_t mode,
            std::unique_ptr<File> *f) final {
     int ret = openat(fd_, path, flags, mode);

src/filesystem.h

@@ -63,7 +63,9 @@ class File {
   virtual int Close() = 0;
 
   // openat(), returning 0 on success or errno>0 on failure.
+  virtual int Open(const char *path, int flags, int *fd) = 0;
   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, int *fd) = 0;
   virtual int Open(const char *path, int flags, mode_t mode,
                    std::unique_ptr<File> *f) = 0;
 
@@ -89,8 +91,8 @@ class MockFile : public File {
  public:
   MOCK_METHOD0(Close, int());
 
-  // Open is wrapped here because gmock's SetArgPointee doesn't work well with
+  // The std::unique_ptr<File> variants of Open are wrapped here because gmock's
-  // std::unique_ptr.
+  // SetArgPointee doesn't work well with std::unique_ptr.
 
   int Open(const char *path, int flags, std::unique_ptr<File> *f) final {
     File *f_tmp = nullptr;
@@ -107,6 +109,8 @@ class MockFile : public File {
     return ret;
   }
 
+  MOCK_METHOD3(Open, int(const char *, int, int *));
+  MOCK_METHOD4(Open, int(const char *, int, mode_t, int *));
   MOCK_METHOD3(OpenRaw, int(const char *, int, File **));
   MOCK_METHOD4(OpenRaw, int(const char *, int, mode_t, File **));
   MOCK_METHOD3(Read, int(void *, size_t, size_t *));

src/http.cc

@@ -55,10 +55,10 @@ namespace {
 
 class RealFile : public VirtualFile {
  public:
-  RealFile(re2::StringPiece mime_type, re2::StringPiece filename,
+  RealFile(re2::StringPiece mime_type, File *dir, re2::StringPiece filename,
            const struct stat &statbuf)
       : mime_type_(mime_type.as_string()), stat_(statbuf) {
-    slice_.Init(filename, ByteRange(0, statbuf.st_size));
+    slice_.Init(dir, filename, ByteRange(0, statbuf.st_size));
   }
 
   ~RealFile() final {}
@@ -246,17 +246,19 @@ bool EvBuffer::AddFile(int fd, ev_off_t offset, ev_off_t length,
   return true;
 }
 
-void RealFileSlice::Init(re2::StringPiece filename, ByteRange range) {
+void RealFileSlice::Init(File *dir, re2::StringPiece filename,
+                         ByteRange range) {
+  dir_ = dir;
   filename_ = filename.as_string();
   range_ = range;
 }
 
 int64_t RealFileSlice::AddRange(ByteRange range, EvBuffer *buf,
                                 std::string *error_message) const {
-  int fd = open(filename_.c_str(), O_RDONLY);
+  int fd;
-  if (fd < 0) {
+  int ret = dir_->Open(filename_.c_str(), O_RDONLY, &fd);
-    int err = errno;
+  if (ret != 0) {
-    *error_message = StrCat("open ", filename_, ": ", strerror(err));
+    *error_message = StrCat("open ", filename_, ": ", strerror(ret));
     return -1;
   }
   if (!buf->AddFile(fd, range_.begin + range.begin, range.size(),
@@ -442,10 +444,10 @@ void HttpServe(const std::shared_ptr<VirtualFile> &file, evhttp_request *req) {
   return ServeChunkCallback(con, serve);
 }
 
-void HttpServeFile(evhttp_request *req, const std::string &mime_type,
+void HttpServeFile(evhttp_request *req, const std::string &mime_type, File *dir,
                    const std::string &filename, const struct stat &statbuf) {
-  return HttpServe(
+  return HttpServe(std::shared_ptr<VirtualFile>(
-      std::shared_ptr<VirtualFile>(new RealFile(mime_type, filename, statbuf)),
+                       new RealFile(mime_type, dir, filename, statbuf)),
                    req);
 }
 

src/http.h

@@ -51,6 +51,7 @@
 #include <glog/logging.h>
 #include <re2/stringpiece.h>
 
+#include "filesystem.h"
 #include "string.h"
 
 namespace moonfire_nvr {
@@ -170,7 +171,8 @@ class VirtualFile : public FileSlice {
 
 class RealFileSlice : public FileSlice {
  public:
-  void Init(re2::StringPiece filename, ByteRange range);
+  // |dir| must outlive the RealFileSlice.
+  void Init(File *dir, re2::StringPiece filename, ByteRange range);
 
   int64_t size() const final { return range_.size(); }
 
@@ -178,6 +180,7 @@ class RealFileSlice : public FileSlice {
                    std::string *error_message) const final;
 
  private:
+  File *dir_;
   std::string filename_;
   ByteRange range_;
 };
@@ -289,7 +292,7 @@ void HttpServe(const std::shared_ptr<VirtualFile> &file, evhttp_request *req);
 
 // Serve a file over HTTP. Expects the caller to supply a sanitized |filename|
 // (rather than taking it straight from the path specified in |req|).
-void HttpServeFile(evhttp_request *req, const std::string &mime_type,
+void HttpServeFile(evhttp_request *req, const std::string &mime_type, File *dir,
                    const std::string &filename, const struct stat &statbuf);
 
 namespace internal {

src/moonfire-db.cc

@@ -414,9 +414,6 @@ bool MoonfireDatabase::ListMp4Recordings(
                  ToHex(run.ColumnBlob(4)));
       return false;
     }
-    recording.sample_file_path =
-        StrCat("/home/slamb/new-moonfire/sample/",
-               recording.sample_file_uuid.UnparseText());
     recording.sample_file_sha1 = run.ColumnBlob(5).as_string();
     recording.video_index = run.ColumnBlob(6).as_string();
     recording.video_samples = run.ColumnInt64(7);
@@ -463,85 +460,6 @@ bool MoonfireDatabase::ListReservedSampleFiles(std::vector<Uuid> *reserved,
   return true;
 }
 
-std::shared_ptr<VirtualFile> MoonfireDatabase::BuildMp4(
-    Uuid camera_uuid, int64_t start_time_90k, int64_t end_time_90k,
-    std::string *error_message) {
-  LOG(INFO) << "Building mp4 for camera: " << camera_uuid.UnparseText()
-            << ", start_time_90k: " << start_time_90k
-            << ", end_time_90k: " << end_time_90k;
-
-  Mp4FileBuilder builder;
-  int64_t next_row_start_time_90k = start_time_90k;
-  int64_t rows = 0;
-  bool ok = true;
-  auto row_cb = [&](Recording &recording,
-                    const VideoSampleEntry &sample_entry) {
-    if (rows == 0 && recording.start_time_90k != next_row_start_time_90k) {
-      *error_message = StrCat(
-          "recording starts late: ", PrettyTimestamp(recording.start_time_90k),
-          " (", recording.start_time_90k, ") rather than requested: ",
-          PrettyTimestamp(start_time_90k), " (", start_time_90k, ")");
-      ok = false;
-      return IterationControl::kBreak;
-    } else if (recording.start_time_90k != next_row_start_time_90k) {
-      *error_message = StrCat("gap/overlap in recording: ",
-                              PrettyTimestamp(next_row_start_time_90k), " (",
-                              next_row_start_time_90k, ") to: ",
-                              PrettyTimestamp(recording.start_time_90k), " (",
-                              recording.start_time_90k, ") before row ", rows);
-      ok = false;
-      return IterationControl::kBreak;
-    }
-
-    next_row_start_time_90k = recording.end_time_90k;
-
-    if (rows > 0 && recording.video_sample_entry_id != sample_entry.id) {
-      *error_message =
-          StrCat("inconsistent video sample entries: this recording has id ",
-                 recording.video_sample_entry_id, " previous had ",
-                 sample_entry.id, " (sha1 ", ToHex(sample_entry.sha1), ")");
-      ok = false;
-      return IterationControl::kBreak;
-    } else if (rows == 0) {
-      builder.SetSampleEntry(sample_entry);
-    }
-
-    // TODO: correct bounds within recording.
-    // Currently this can return too much data.
-    builder.Append(std::move(recording), 0,
-                   std::numeric_limits<int32_t>::max());
-    ++rows;
-    return IterationControl::kContinue;
-  };
-  if (!ok ||
-      !ListMp4Recordings(camera_uuid, start_time_90k, end_time_90k, row_cb,
-                         error_message)) {
-    return false;
-  }
-  if (rows == 0) {
-    *error_message = StrCat("no recordings in range");
-    return false;
-  }
-  if (next_row_start_time_90k != end_time_90k) {
-    *error_message = StrCat("recording ends early: ",
-                            PrettyTimestamp(next_row_start_time_90k), " (",
-                            next_row_start_time_90k, "), not requested: ",
-                            PrettyTimestamp(end_time_90k), " (", end_time_90k,
-                            ") after ", rows, " rows");
-    return false;
-  }
-
-  VLOG(1) << "...(3/4) building VirtualFile from " << rows << " recordings.";
-  auto file = builder.Build(error_message);
-  if (file == nullptr) {
-    return false;
-  }
-
-  VLOG(1) << "...(4/4) success, " << file->size() << " bytes, etag "
-          << file->etag();
-  return file;
-}
-
 std::vector<Uuid> MoonfireDatabase::ReserveSampleFiles(
     int n, std::string *error_message) {
   if (n == 0) {

src/moonfire-db.h

@@ -157,14 +157,6 @@ class MoonfireDatabase {
           row_cb,
       std::string *error_message);
 
-  // TODO: more nuanced error code for HTTP.
-  // TODO: this should move somewhere that has access to the
-  // currently-writing Recording as well.
-  std::shared_ptr<VirtualFile> BuildMp4(Uuid camera_uuid,
-                                        int64_t start_time_90k,
-                                        int64_t end_time_90k,
-                                        std::string *error_message);
-
   bool ListReservedSampleFiles(std::vector<Uuid> *reserved,
                                std::string *error_message);
 

src/moonfire-nvr.cc

@@ -205,7 +205,19 @@ bool Stream::Init(std::string *error_message) {
     return false;
   }
 
-  return manager_.Init(error_message);
+  if (!manager_.Init(error_message)) {
+    return false;
+  }
+
+  int ret = env_->fs->Open(camera_path_.c_str(), O_RDONLY | O_DIRECTORY,
+                           &camera_dir_);
+  if (ret != 0) {
+    *error_message =
+        StrCat("Unable to open ", camera_path_, ": ", strerror(ret));
+    return false;
+  }
+
+  return true;
 }
 
 // Call from dedicated thread. Runs until shutdown requested.
@@ -463,7 +475,7 @@ void Stream::HttpCallbackForFile(evhttp_request *req, const string &filename) {
   if (!manager_.Lookup(filename, &s)) {
     return evhttp_send_error(req, HTTP_NOTFOUND, "File not found.");
   }
-  HttpServeFile(req, "video/mp4", StrCat(camera_path_, "/", filename), s);
+  HttpServeFile(req, "video/mp4", camera_dir_.get(), filename, s);
 }
 
 Nvr::Nvr() {

src/moonfire-nvr.h

@@ -186,6 +186,7 @@ class Stream {
   const moonfire_nvr::Camera camera_;
 
   FileManager manager_;               // thread-safe.
+  std::unique_ptr<File> camera_dir_;  // thread-safe.
 
   //
   // State below is used only by the thread in Run().

src/mp4-test.cc

@@ -197,8 +197,8 @@ class IntegrationTest : public testing::Test {
  protected:
   IntegrationTest() {
     tmpdir_path_ = PrepareTempDirOrDie("mp4-integration-test");
-    int ret =
+    int ret = GetRealFilesystem()->Open(tmpdir_path_.c_str(),
-        GetRealFilesystem()->Open(tmpdir_path_.c_str(), O_RDONLY, &tmpdir_);
+                                        O_RDONLY | O_DIRECTORY, &tmpdir_);
     CHECK_EQ(0, ret) << strerror(ret);
   }
 
@@ -210,9 +210,9 @@ class IntegrationTest : public testing::Test {
     // Set start time to 2015-04-26 00:00:00 UTC.
     index.Init(&recording, UINT64_C(1430006400) * kTimeUnitsPerSecond);
     SampleFileWriter writer(tmpdir_.get());
-    recording.sample_file_path = StrCat(tmpdir_path_, "/clip.sample");
+    std::string filename = recording.sample_file_uuid.UnparseText();
-    if (!writer.Open("clip.sample", &error_message)) {
+    if (!writer.Open(filename.c_str(), &error_message)) {
-      ADD_FAILURE() << "open clip.sample: " << error_message;
+      ADD_FAILURE() << "open " << filename << ": " << error_message;
       return recording;
     }
     auto in = GetRealVideoSource()->OpenFile("../src/testdata/clip.mp4",
@@ -255,7 +255,7 @@ class IntegrationTest : public testing::Test {
 
   std::shared_ptr<VirtualFile> CreateMp4FromSingleRecording(
       const Recording &recording) {
-    Mp4FileBuilder builder;
+    Mp4FileBuilder builder(tmpdir_.get());
     builder.SetSampleEntry(video_sample_entry_);
     builder.Append(Recording(recording), 0,
                    std::numeric_limits<int32_t>::max());

src/mp4.cc

@@ -360,9 +360,11 @@ class ScopedMp4Box {
 // * mdat (media data container)
 class Mp4File : public VirtualFile {
  public:
-  Mp4File(std::vector<std::unique_ptr<Mp4FileSegment>> segments,
+  Mp4File(File *sample_file_dir,
+          std::vector<std::unique_ptr<Mp4FileSegment>> segments,
           VideoSampleEntry &&video_sample_entry)
-      : segments_(std::move(segments)),
+      : sample_file_dir_(sample_file_dir),
+        segments_(std::move(segments)),
         video_sample_entry_(std::move(video_sample_entry)),
         ftyp_(re2::StringPiece(kFtypBox, sizeof(kFtypBox))),
         moov_trak_mdia_hdlr_(re2::StringPiece(kHdlrBox, sizeof(kHdlrBox))),
@@ -390,7 +392,8 @@ class Mp4File : public VirtualFile {
     slices_.Append(mdat_.header_slice());
     initial_sample_byte_pos_ = slices_.size();
     for (const auto &segment : segments_) {
-      segment->sample_file_slice.Init(segment->recording.sample_file_path,
+      segment->sample_file_slice.Init(
+          sample_file_dir_, segment->recording.sample_file_uuid.UnparseText(),
           segment->pieces.sample_pos());
       slices_.Append(&segment->sample_file_slice, FileSlices::kLazy);
     }
@@ -541,6 +544,7 @@ class Mp4File : public VirtualFile {
   }
 
   int64_t initial_sample_byte_pos_ = 0;
+  File *sample_file_dir_ = nullptr;
   std::vector<std::unique_ptr<Mp4FileSegment>> segments_;
   VideoSampleEntry video_sample_entry_;
   FileSlices slices_;
@@ -749,8 +753,8 @@ std::shared_ptr<VirtualFile> Mp4FileBuilder::Build(std::string *error_message) {
     return std::shared_ptr<VirtualFile>();
   }
 
-  return std::shared_ptr<VirtualFile>(
+  return std::shared_ptr<VirtualFile>(new Mp4File(
-      new Mp4File(std::move(segments_), std::move(video_sample_entry_)));
+      sample_file_dir_, std::move(segments_), std::move(video_sample_entry_)));
 }
 
 }  // namespace moonfire_nvr

src/mp4.h

@@ -130,6 +130,13 @@ struct Mp4FileSegment {
 // Builder for a virtual .mp4 file.
 class Mp4FileBuilder {
  public:
+  // |sample_file_dir| must outlive the Mp4FileBuilder and the returned
+  // VirtualFile.
+  explicit Mp4FileBuilder(File *sample_file_dir)
+      : sample_file_dir_(sample_file_dir) {}
+  Mp4FileBuilder(const Mp4FileBuilder &) = delete;
+  void operator=(const Mp4FileBuilder &) = delete;
+
   // Append part or all of a recording.
   // Note that |recording.video_sample_entry_sha1| must be added via
   // AddSampleEntry.
@@ -159,6 +166,7 @@ class Mp4FileBuilder {
   std::shared_ptr<VirtualFile> Build(std::string *error_message);
 
  private:
+  File *sample_file_dir_;
   std::vector<std::unique_ptr<internal::Mp4FileSegment>> segments_;
   VideoSampleEntry video_sample_entry_;
 };

src/recording.h

@@ -64,7 +64,6 @@ constexpr int64_t kMaxRecordingDuration = 5 * 60 * kTimeUnitsPerSecond;
 struct Recording {
   int64_t id = -1;
   int64_t camera_id = -1;
-  std::string sample_file_path;
   std::string sample_file_sha1;
   Uuid sample_file_uuid;
   int64_t video_sample_entry_id = -1;

src/web.cc

@@ -210,7 +210,7 @@ void WebInterface::HandleMp4View(evhttp_request *req, void *arg) {
   }
 
   std::string error_message;
-  auto file = this_->mdb_->BuildMp4(camera_uuid, start_time_90k, end_time_90k,
+  auto file = this_->BuildMp4(camera_uuid, start_time_90k, end_time_90k,
                               &error_message);
   if (file == nullptr) {
     // TODO: more nuanced HTTP status codes.
@@ -221,4 +221,83 @@ void WebInterface::HandleMp4View(evhttp_request *req, void *arg) {
   return HttpServe(file, req);
 }
 
+std::shared_ptr<VirtualFile> WebInterface::BuildMp4(
+    Uuid camera_uuid, int64_t start_time_90k, int64_t end_time_90k,
+    std::string *error_message) {
+  LOG(INFO) << "Building mp4 for camera: " << camera_uuid.UnparseText()
+            << ", start_time_90k: " << start_time_90k
+            << ", end_time_90k: " << end_time_90k;
+
+  Mp4FileBuilder builder(sample_file_dir_);
+  int64_t next_row_start_time_90k = start_time_90k;
+  int64_t rows = 0;
+  bool ok = true;
+  auto row_cb = [&](Recording &recording,
+                    const VideoSampleEntry &sample_entry) {
+    if (rows == 0 && recording.start_time_90k != next_row_start_time_90k) {
+      *error_message = StrCat(
+          "recording starts late: ", PrettyTimestamp(recording.start_time_90k),
+          " (", recording.start_time_90k, ") rather than requested: ",
+          PrettyTimestamp(start_time_90k), " (", start_time_90k, ")");
+      ok = false;
+      return IterationControl::kBreak;
+    } else if (recording.start_time_90k != next_row_start_time_90k) {
+      *error_message = StrCat("gap/overlap in recording: ",
+                              PrettyTimestamp(next_row_start_time_90k), " (",
+                              next_row_start_time_90k, ") to: ",
+                              PrettyTimestamp(recording.start_time_90k), " (",
+                              recording.start_time_90k, ") before row ", rows);
+      ok = false;
+      return IterationControl::kBreak;
+    }
+
+    next_row_start_time_90k = recording.end_time_90k;
+
+    if (rows > 0 && recording.video_sample_entry_id != sample_entry.id) {
+      *error_message =
+          StrCat("inconsistent video sample entries: this recording has id ",
+                 recording.video_sample_entry_id, " previous had ",
+                 sample_entry.id, " (sha1 ", ToHex(sample_entry.sha1), ")");
+      ok = false;
+      return IterationControl::kBreak;
+    } else if (rows == 0) {
+      builder.SetSampleEntry(sample_entry);
+    }
+
+    // TODO: correct bounds within recording.
+    // Currently this can return too much data.
+    builder.Append(std::move(recording), 0,
+                   std::numeric_limits<int32_t>::max());
+    ++rows;
+    return IterationControl::kContinue;
+  };
+  if (!ok ||
+      !mdb_->ListMp4Recordings(camera_uuid, start_time_90k, end_time_90k,
+                               row_cb, error_message)) {
+    return false;
+  }
+  if (rows == 0) {
+    *error_message = StrCat("no recordings in range");
+    return false;
+  }
+  if (next_row_start_time_90k != end_time_90k) {
+    *error_message = StrCat("recording ends early: ",
+                            PrettyTimestamp(next_row_start_time_90k), " (",
+                            next_row_start_time_90k, "), not requested: ",
+                            PrettyTimestamp(end_time_90k), " (", end_time_90k,
+                            ") after ", rows, " rows");
+    return false;
+  }
+
+  VLOG(1) << "...(3/4) building VirtualFile from " << rows << " recordings.";
+  auto file = builder.Build(error_message);
+  if (file == nullptr) {
+    return false;
+  }
+
+  VLOG(1) << "...(4/4) success, " << file->size() << " bytes, etag "
+          << file->etag();
+  return file;
+}
+
 }  // namespace moonfire_nvr

src/web.h

@@ -59,7 +59,9 @@ namespace moonfire_nvr {
 
 class WebInterface {
  public:
-  explicit WebInterface(MoonfireDatabase *mdb) : mdb_(mdb) {}
+  // |mdb| and |sample_file_dir| must outlive the WebInterface.
+  WebInterface(MoonfireDatabase *mdb, File *sample_file_dir)
+      : mdb_(mdb), sample_file_dir_(sample_file_dir) {}
   WebInterface(const WebInterface &) = delete;
   void operator=(const WebInterface &) = delete;
 
@@ -70,7 +72,14 @@ class WebInterface {
   static void HandleCameraDetail(evhttp_request *req, void *arg);
   static void HandleMp4View(evhttp_request *req, void *arg);
 
+  // TODO: more nuanced error code for HTTP.
+  std::shared_ptr<VirtualFile> BuildMp4(Uuid camera_uuid,
+                                        int64_t start_time_90k,
+                                        int64_t end_time_90k,
+                                        std::string *error_message);
+
   MoonfireDatabase *const mdb_;
+  File *const sample_file_dir_;
 };
 
 }  // namespace moonfire_nvr