Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow opening a database using a file handle instead of a path #665

Merged
merged 4 commits into from
Sep 3, 2023
Merged

Allow opening a database using a file handle instead of a path #665

merged 4 commits into from
Sep 3, 2023

Conversation

adamreichold
Copy link
Contributor

This enables using this library with e.g. cap-std which avoids path-based access in favour of a capability-based approach.

I am not sure about the name, but open while probably most fitting is already taken for the path-based approach. Alternatives I considered were from_file, access, mount and open_or_init.

I added another commit to ensure that the build is Clippy-clean before submitting the PR. Please let me if know if you like me to drop that from the PR as it is admittedly unrelated. (Bumping fastrand resolves a few warnings about unnecessary mutability in the LMDB benchmark.)

@adamreichold
Copy link
Contributor Author

Having read the implementation of Builder::open, I wonder if the following diff

diff --git a/src/db.rs b/src/db.rs
index 586bd98..57f10cb 100644
--- a/src/db.rs
+++ b/src/db.rs
@@ -12,7 +12,6 @@ use crate::{
 use crate::{ReadTransaction, Result, WriteTransaction};
 use std::fmt::{Display, Formatter};
 use std::fs::{File, OpenOptions};
-use std::io;
 use std::io::ErrorKind;
 use std::marker::PhantomData;
 use std::ops::RangeFull;
@@ -769,20 +768,19 @@ impl Builder {
 
     /// Opens an existing redb database.
     pub fn open(&self, path: impl AsRef<Path>) -> Result<Database, DatabaseError> {
-        if !path.as_ref().exists() {
-            Err(StorageError::Io(ErrorKind::NotFound.into()).into())
-        } else if File::open(path.as_ref())?.metadata()?.len() > 0 {
-            let file = OpenOptions::new().read(true).write(true).open(path)?;
-            Database::new(
-                file,
-                self.page_size,
-                None,
-                self.read_cache_size_bytes,
-                self.write_cache_size_bytes,
-            )
-        } else {
-            Err(StorageError::Io(io::Error::from(ErrorKind::InvalidData)).into())
+        let file = OpenOptions::new().read(true).write(true).open(path)?;
+
+        if file.metadata()?.len() == 0 {
+            return Err(StorageError::Io(ErrorKind::InvalidData.into()).into());
         }
+
+        Database::new(
+            file,
+            self.page_size,
+            None,
+            self.read_cache_size_bytes,
+            self.write_cache_size_bytes,
+        )
     }
 
     /// Open an existing or create a new database in the given `file`.

could be used to avoid the extra system calls to stat and open?

@cberner
Copy link
Owner

cberner commented Sep 3, 2023

ya, that diff seems like it would work. As long as there's a test for a missing and an empty file it should be safe to change it

src/db.rs Outdated Show resolved Hide resolved
@cberner
Copy link
Owner

cberner commented Sep 3, 2023

Couple minor comments. Otherwise this seems good to me!

@adamreichold
Copy link
Contributor Author

ya, that diff seems like it would work. As long as there's a test for a missing and an empty file it should be safe to change it

I added test cases for these two error paths as a separate commit before the refactoring.

@cberner cberner merged commit 5dde756 into cberner:master Sep 3, 2023
3 checks passed
@cberner
Copy link
Owner

cberner commented Sep 3, 2023

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants