improve test of upgrade from v0 on up; fix bugs

Now the test actually has a recording and garbage with matching files.
This caught a few problems in the upgrade procedure:

* it didn't work with foreign keys enabled because the new recording
  table was set up after the new camera table, and the old recording
  table was destroyed after the old camera table. And now I enable
  foreign keys all the time. Reorder the procedure to fix.

* the pathname manipulation in the v2 to v3 procedure was incorrect
  since my introduction of nix because I gave it a &[u8] with the
  trailing nul, where I should have used CStr::from_bytes_with_nul.

* it wasn't removing garbage files. It'd be most natural to do this
  in the v2 to v3 upgrade (with the rename) but I historically removed
  the table when upgrading to v2. I can't redefine the schema now, so
  do it unnaturally.

  I'm considering also renaming all uuid-like files on upgrade to v4/v5
  to clean up this mess automatically for installations that have
  already done this upgrade.

db/testutil.rs

@@ -48,6 +48,14 @@ static INIT: sync::Once = sync::ONCE_INIT;
 pub const TEST_CAMERA_ID: i32 = 1;
 pub const TEST_STREAM_ID: i32 = 1;
 
+pub const TEST_VIDEO_SAMPLE_ENTRY_DATA: &[u8] =
+    b"\x00\x00\x00\x7D\x61\x76\x63\x31\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\
+    \x00\x00\x00\x00\x00\x00\x00\x00\x00\x07\x80\x04\x38\x00\x48\x00\x00\x00\x48\x00\x00\x00\x00\
+    \x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\
+    \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x18\xFF\xFF\x00\x00\x00\x27\x61\x76\
+    \x63\x43\x01\x4D\x00\x2A\xFF\xE1\x00\x10\x67\x4D\x00\x2A\x95\xA8\x1E\x00\x89\xF9\x66\xE0\x20\
+    \x20\x20\x40\x01\x00\x04\x68\xEE\x3C\x80";
+
 /// Performs global initialization for tests.
 ///    * set up logging. (Note the output can be confusing unless `RUST_TEST_THREADS=1` is set in
 ///      the program's environment prior to running.)

db/upgrade/mod.rs

@@ -35,7 +35,11 @@
 use crate::db;
 use failure::{Error, bail};
 use log::info;
+use std::ffi::CStr;
+use std::io::Write;
+use nix::NixPath;
 use rusqlite::params;
+use uuid::Uuid;
 
 mod v0_to_v1;
 mod v1_to_v2;
@@ -82,7 +86,7 @@ fn upgrade(args: &Args, target_ver: i32, conn: &mut rusqlite::Connection) -> Res
             bail!("Database is at negative version {}!", old_ver);
         }
         info!("Upgrading database from version {} to version {}...", old_ver, target_ver);
-        set_journal_mode(&conn, args.flag_preset_journal).unwrap();
+        set_journal_mode(&conn, args.flag_preset_journal)?;
         for ver in old_ver .. target_ver {
             info!("...from version {} to version {}", ver, ver + 1);
             let tx = conn.transaction()?;
@@ -113,22 +117,46 @@ pub fn run(args: &Args, conn: &mut rusqlite::Connection) -> Result<(), Error> {
 
     // WAL is the preferred journal mode for normal operation; it reduces the number of syncs
     // without compromising safety.
-    set_journal_mode(&conn, "wal").unwrap();
+    set_journal_mode(&conn, "wal")?;
     if !args.flag_no_vacuum {
         info!("...vacuuming database after upgrade.");
         conn.execute_batch(r#"
             pragma page_size = 16384;
             vacuum;
-        "#).unwrap();
+        "#)?;
     }
     info!("...done.");
 
     Ok(())
 }
 
+/// A uuid-based path, as used in version 0 and version 1 schemas.
+struct UuidPath([u8; 37]);
+
+impl UuidPath {
+    pub(crate) fn from(uuid: Uuid) -> Self {
+        let mut buf = [0u8; 37];
+        write!(&mut buf[..36], "{}", uuid.to_hyphenated_ref())
+            .expect("can't format uuid to pathname buf");
+        UuidPath(buf)
+    }
+}
+
+impl NixPath for UuidPath {
+    fn len(&self) -> usize { 36 }
+
+    fn with_nix_path<T, F>(&self, f: F) -> Result<T, nix::Error>
+    where F: FnOnce(&CStr) -> T {
+        let p = CStr::from_bytes_with_nul(&self.0[..]).expect("no interior nuls");
+        Ok(f(p))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::compare;
+    use crate::testutil;
+    use failure::{ResultExt, format_err};
     use super::*;
 
     fn new_conn() -> Result<rusqlite::Connection, Error> {
@@ -152,25 +180,59 @@ mod tests {
     /// Doesn't (yet) compare any actual data.
     #[test]
     fn upgrade_and_compare() -> Result<(), Error> {
-        let tmpdir = tempdir::TempDir::new("moonfire-nvr-test").unwrap();
-        let path = tmpdir.path().to_str().unwrap().to_owned();
+        testutil::init();
+        let tmpdir = tempdir::TempDir::new("moonfire-nvr-test")?;
+        let path = tmpdir.path().to_str().ok_or_else(|| format_err!("invalid UTF-8"))?.to_owned();
         let mut upgraded = new_conn()?;
         upgraded.execute_batch(include_str!("v0.sql"))?;
+        upgraded.execute_batch(r#"
+            insert into camera (id, uuid, short_name, description, host, username, password,
+                                main_rtsp_path, sub_rtsp_path, retain_bytes)
+                        values (1, zeroblob(16), 'test camera', 'desc', 'host', 'user', 'pass',
+                                'main', 'sub', 42);
+        "#)?;
+        upgraded.execute(r#"
+            insert into video_sample_entry (id, sha1, width, height, data)
+                                    values (1, X'3BA3EDE1BD93B7BCB7AB5BD099C047701451B822',
+                                            1920, 1080, ?);
+        "#, params![testutil::TEST_VIDEO_SAMPLE_ENTRY_DATA])?;
+        upgraded.execute_batch(r#"
+            insert into recording (id, camera_id, sample_file_bytes, start_time_90k, duration_90k,
+                                   local_time_delta_90k, video_samples, video_sync_samples,
+                                   video_sample_entry_id, sample_file_uuid, sample_file_sha1,
+                                   video_index)
+                           values (1, 1, 42, 140063580000000, 90000, 0, 1, 1, 1,
+                                   X'E69D45E8CBA64DC1BA2ECB1585983A10', zeroblob(20), X'00');
+            insert into reserved_sample_files values (X'51EF700C933E4197AAE4EE8161E94221', 0),
+                                                     (X'E69D45E8CBA64DC1BA2ECB1585983A10', 1);
+        "#)?;
+        let rec1 = tmpdir.path().join("e69d45e8-cba6-4dc1-ba2e-cb1585983a10");
+        let garbage = tmpdir.path().join("51ef700c-933e-4197-aae4-ee8161e94221");
+        std::fs::File::create(&rec1)?;
+        std::fs::File::create(&garbage)?;
 
         for (ver, fresh_sql) in &[(1, Some(include_str!("v1.sql"))),
                                   (2, None),  // transitional; don't compare schemas.
                                   (3, Some(include_str!("v3.sql"))),
                                   (4, None),  // transitional; don't compare schemas.
-                                  (4, Some(include_str!("../schema.sql")))] {
+                                  (5, Some(include_str!("../schema.sql")))] {
             upgrade(&Args {
                 flag_sample_file_dir: Some(&path),
                 flag_preset_journal: "delete",
                 flag_no_vacuum: false,
-            }, *ver, &mut upgraded)?;
+            }, *ver, &mut upgraded).context(format!("upgrading to version {}", ver))?;
             if let Some(f) = fresh_sql {
                 compare(&upgraded, *ver, f)?;
             }
         }
+
+        // Check that recording files get renamed.
+        assert!(!rec1.exists());
+        assert!(tmpdir.path().join("0000000100000001").exists());
+
+        // Check that garbage files get cleaned up.
+        assert!(!garbage.exists());
+
         Ok(())
     }
 }

db/upgrade/v0_to_v1.rs

@@ -34,7 +34,7 @@ use crate::db;
 use crate::recording;
 use failure::Error;
 use log::warn;
-use rusqlite::types::ToSql;
+use rusqlite::params;
 use std::collections::HashMap;
 
 pub fn run(_args: &super::Args, tx: &rusqlite::Transaction) -> Result<(), Error> {
@@ -88,12 +88,27 @@ pub fn run(_args: &super::Args, tx: &rusqlite::Transaction) -> Result<(), Error>
           sample_file_sha1 blob not null check (length(sample_file_sha1) = 20),
           video_index blob not null check (length(video_index) > 0)
         );
+        insert into camera
+        select
+          id,
+          uuid,
+          short_name,
+          description,
+          host,
+          username,
+          password,
+          main_rtsp_path,
+          sub_rtsp_path,
+          retain_bytes,
+          1 as next_recording_id
+        from
+          old_camera;
     "#)?;
-    let camera_state = fill_recording(tx).unwrap();
-    fill_camera(tx, camera_state).unwrap();
+    let camera_state = fill_recording(tx)?;
+    update_camera(tx, camera_state)?;
     tx.execute_batch(r#"
-      drop table old_camera;
       drop table old_recording;
+      drop table old_camera;
     "#)?;
     Ok(())
 }
@@ -142,7 +157,7 @@ fn fill_recording(tx: &rusqlite::Transaction) -> Result<HashMap<i32, CameraState
       insert into recording_playback values (:composite_id, :sample_file_uuid, :sample_file_sha1,
                                              :video_index)
     "#)?;
-    let mut rows = select.query(&[] as &[&dyn ToSql])?;
+    let mut rows = select.query(params![])?;
     let mut camera_state: HashMap<i32, CameraState> = HashMap::new();
     while let Some(row) = rows.next()? {
         let camera_id: i32 = row.get(0)?;
@@ -203,44 +218,15 @@ fn fill_recording(tx: &rusqlite::Transaction) -> Result<HashMap<i32, CameraState
     Ok(camera_state)
 }
 
-fn fill_camera(tx: &rusqlite::Transaction, camera_state: HashMap<i32, CameraState>)
+fn update_camera(tx: &rusqlite::Transaction, camera_state: HashMap<i32, CameraState>)
                 -> Result<(), Error> {
-    let mut select = tx.prepare(r#"
-      select
-        id, uuid, short_name, description, host, username, password, main_rtsp_path,
-        sub_rtsp_path, retain_bytes
-      from
-        old_camera
+    let mut stmt = tx.prepare(r#"
+      update camera set next_recording_id = :next_recording_id where id = :id
     "#)?;
-    let mut insert = tx.prepare(r#"
-      insert into camera values (:id, :uuid, :short_name, :description, :host, :username, :password,
-      :main_rtsp_path, :sub_rtsp_path, :retain_bytes, :next_recording_id)
-    "#)?;
-    let mut rows = select.query(&[] as &[&dyn ToSql])?;
-    while let Some(row) = rows.next()? {
-        let id: i32 = row.get(0)?;
-        let uuid: Vec<u8> = row.get(1)?;
-        let short_name: String = row.get(2)?;
-        let description: String = row.get(3)?;
-        let host: String = row.get(4)?;
-        let username: String = row.get(5)?;
-        let password: String = row.get(6)?;
-        let main_rtsp_path: String = row.get(7)?;
-        let sub_rtsp_path: String = row.get(8)?;
-        let retain_bytes: i64 = row.get(9)?;
-        insert.execute_named(&[
+    for (ref id, ref state) in &camera_state {
+        stmt.execute_named(&[
             (":id", &id),
-            (":uuid", &uuid),
-            (":short_name", &short_name),
-            (":description", &description),
-            (":host", &host),
-            (":username", &username),
-            (":password", &password),
-            (":main_rtsp_path", &main_rtsp_path),
-            (":sub_rtsp_path", &sub_rtsp_path),
-            (":retain_bytes", &retain_bytes),
-            (":next_recording_id",
-             &camera_state.get(&id).map(|s| s.next_recording_id).unwrap_or(1)),
+            (":next_recording_id", &state.next_recording_id),
         ])?;
     }
     Ok(())

db/upgrade/v1_to_v2.rs

@@ -346,7 +346,16 @@ fn verify_dir_contents(sample_file_path: &str, dir: &mut nix::dir::Dir,
     let mut rows = stmt.query(&[] as &[&dyn ToSql])?;
     while let Some(row) = rows.next()? {
         let uuid: crate::db::FromSqlUuid = row.get(0)?;
-        files.remove(&uuid.0);
+        if files.remove(&uuid.0) {
+            // Also remove the garbage file. For historical reasons (version 2 was originally
+            // defined as not having a garbage table so still is), do this here rather than with
+            // the other path manipulations in v2_to_v3.rs. There's no harm anyway in deleting
+            // a garbage file so if the upgrade transation fails this is still a valid and complete
+            // version 1 database.
+            let p = super::UuidPath::from(uuid.0);
+            nix::unistd::unlinkat(Some(dir.as_raw_fd()), &p,
+                                  nix::unistd::UnlinkatFlags::NoRemoveDir)?;
+        }
     }
 
     if !files.is_empty() {

db/upgrade/v2_to_v3.rs

@@ -38,10 +38,8 @@ use failure::Error;
 use crate::schema;
 use protobuf::prelude::MessageField;
 use rusqlite::types::ToSql;
-use std::io::Write;
 use std::os::unix::io::AsRawFd;
 use std::sync::Arc;
-use uuid::Uuid;
 
 /// Opens the sample file dir.
 ///
@@ -88,9 +86,9 @@ pub fn run(_args: &super::Args, tx: &rusqlite::Transaction) -> Result<(), Error>
     while let Some(row) = rows.next()? {
         let id = db::CompositeId(row.get(0)?);
         let sample_file_uuid: FromSqlUuid = row.get(1)?;
-        let from_path = get_uuid_pathname(sample_file_uuid.0);
+        let from_path = super::UuidPath::from(sample_file_uuid.0);
         let to_path = crate::dir::CompositeIdPath::from(id);
-        if let Err(e) = nix::fcntl::renameat(Some(d.fd.as_raw_fd()), &from_path[..],
+        if let Err(e) = nix::fcntl::renameat(Some(d.fd.as_raw_fd()), &from_path,
                                              Some(d.fd.as_raw_fd()), &to_path) {
             if e == nix::Error::Sys(nix::errno::Errno::ENOENT) {
                 continue;  // assume it was already moved.
@@ -119,11 +117,3 @@ pub fn run(_args: &super::Args, tx: &rusqlite::Transaction) -> Result<(), Error>
     "#)?;
     Ok(())
 }
-
-/// Gets a pathname for a sample file suitable for passing to open or unlink.
-fn get_uuid_pathname(uuid: Uuid) -> [u8; 37] {
-    let mut buf = [0u8; 37];
-    write!(&mut buf[..36], "{}", uuid.to_hyphenated_ref())
-        .expect("can't format uuid to pathname buf");
-    buf
-}