Merge pull request #202 from mozilla/ignore-io-error

Small migration helper followups

Cargo.toml

@@ -23,6 +23,7 @@ backtrace = ["failure/backtrace", "failure/std"]
 db-dup-sort = []
 db-int-key = []
 default = ["db-dup-sort", "db-int-key"]
+no-canonicalize-path = []
 with-asan = ["lmdb-rkv/with-asan"]
 with-fuzzer = ["lmdb-rkv/with-fuzzer"]
 with-fuzzer-no-link = ["lmdb-rkv/with-fuzzer-no-link"]

src/manager.rs

@@ -49,6 +49,15 @@ lazy_static! {
 
 /// A process is only permitted to have one open handle to each Rkv environment. This
 /// manager exists to enforce that constraint: don't open environments directly.
+///
+/// By default, path canonicalization is enabled for identifying RKV instances. This
+/// is true by default, because it helps enforce the constraints guaranteed by
+/// this manager. However, path canonicalization might crash in some fringe
+/// circumstances, so the `no-canonicalize-path` feature offers the possibility of
+/// disabling it. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1531887
+///
+/// When path canonicalization is disabled, you *must* ensure an RKV environment is
+/// always created or retrieved with the same path.
 pub struct Manager<E> {
     environments: BTreeMap<PathBuf, SharedRkv<E>>,
 }
@@ -68,7 +77,11 @@ where
     where
         P: Into<&'p Path>,
     {
-        let canonical = canonicalize_path(path)?;
+        let canonical = if cfg!(feature = "no-canonicalize-path") {
+            path.into().to_path_buf()
+        } else {
+            canonicalize_path(path)?
+        };
         Ok(self.environments.get(&canonical).cloned())
     }
 
@@ -78,7 +91,11 @@ where
         F: FnOnce(&Path) -> Result<Rkv<E>>,
         P: Into<&'p Path>,
     {
-        let canonical = canonicalize_path(path)?;
+        let canonical = if cfg!(feature = "no-canonicalize-path") {
+            path.into().to_path_buf()
+        } else {
+            canonicalize_path(path)?
+        };
         Ok(match self.environments.entry(canonical) {
             Entry::Occupied(e) => e.get().clone(),
             Entry::Vacant(e) => {
@@ -94,7 +111,11 @@ where
         F: FnOnce(&Path, c_uint) -> Result<Rkv<E>>,
         P: Into<&'p Path>,
     {
-        let canonical = canonicalize_path(path)?;
+        let canonical = if cfg!(feature = "no-canonicalize-path") {
+            path.into().to_path_buf()
+        } else {
+            canonicalize_path(path)?
+        };
         Ok(match self.environments.entry(canonical) {
             Entry::Occupied(e) => e.get().clone(),
             Entry::Vacant(e) => {
@@ -111,7 +132,11 @@ where
         P: Into<&'p Path>,
         B: BackendEnvironmentBuilder<'e, Environment = E>,
     {
-        let canonical = canonicalize_path(path)?;
+        let canonical = if cfg!(feature = "no-canonicalize-path") {
+            path.into().to_path_buf()
+        } else {
+            canonicalize_path(path)?
+        };
         Ok(match self.environments.entry(canonical) {
             Entry::Occupied(e) => e.get().clone(),
             Entry::Vacant(e) => {
@@ -128,7 +153,11 @@ where
     where
         P: Into<&'p Path>,
     {
-        let canonical = canonicalize_path(path)?;
+        let canonical = if cfg!(feature = "no-canonicalize-path") {
+            path.into().to_path_buf()
+        } else {
+            canonicalize_path(path)?
+        };
         match self.environments.entry(canonical) {
             Entry::Vacant(_) => {}, // noop
             Entry::Occupied(e) => {

src/migrator.rs

@@ -101,7 +101,12 @@ macro_rules! fn_migrator {
             let mut builder = Rkv::<$src_env>::environment_builder::<$builder>();
             builder.set_max_dbs(crate::env::DEFAULT_MAX_DBS);
             builder = build(builder);
-            let src_env = manager.get_or_create_from_builder(path, builder, Rkv::from_builder::<$builder>)?;
+
+            let src_env = match manager.get_or_create_from_builder(path, builder, Rkv::from_builder::<$builder>) {
+                Err(crate::StoreError::IoError(ref e)) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()),
+                Err(crate::StoreError::UnsuitableEnvironmentPath(_)) => return Ok(()),
+                result => result,
+            }?;
 
             match Migrator::$migrate(src_env.read()?, dst_env) {
                 Err(crate::MigrateError::SourceEmpty) => return Ok(()),

tests/env-migration.rs

@@ -8,7 +8,10 @@
 // CONDITIONS OF ANY KIND, either express or implied. See the License for the
 // specific language governing permissions and limitations under the License.
 
-use std::fs;
+use std::{
+    fs,
+    path::Path,
+};
 
 use tempfile::Builder;
 
@@ -180,6 +183,44 @@ fn test_migrator_round_trip() {
     }
 }
 
+#[test]
+fn test_migrator_no_dir_1() {
+    let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    let dst_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
+    Migrator::auto_migrate_lmdb_to_safe_mode(Path::new("bogus"), |builder| builder, &dst_env).expect("migrated");
+
+    let mut datamdb = root.path().to_path_buf();
+    let mut lockmdb = root.path().to_path_buf();
+    let mut safebin = root.path().to_path_buf();
+    datamdb.push("data.mdb");
+    lockmdb.push("lock.mdb");
+    safebin.push("data.safe.bin");
+    assert!(!datamdb.exists());
+    assert!(!lockmdb.exists());
+    assert!(!safebin.exists()); // safe mode doesn't write an empty db to disk
+}
+
+#[test]
+fn test_migrator_no_dir_2() {
+    let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    let dst_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
+    Migrator::auto_migrate_safe_mode_to_lmdb(Path::new("bogus"), |builder| builder, &dst_env).expect("migrated");
+
+    let mut datamdb = root.path().to_path_buf();
+    let mut lockmdb = root.path().to_path_buf();
+    let mut safebin = root.path().to_path_buf();
+    datamdb.push("data.mdb");
+    lockmdb.push("lock.mdb");
+    safebin.push("data.safe.bin");
+    assert!(datamdb.exists()); // lmdb writes an empty db to disk
+    assert!(lockmdb.exists());
+    assert!(!safebin.exists());
+}
+
 #[test]
 #[should_panic(expected = "migrated: SourceEmpty")]
 fn test_migrator_lmdb_to_safe_1() {