Replace `unsafe` impl of backups with `rusqlite`.

The unsafe code still exists, but I have more faith in the rusqlite authors than in myself to ensure that the code is correct.

1 files changed, 33 insertions, 101 deletions

diff --git a/src/db/backup.rs b/src/db/backup.rs
index b09bb3d..bb36aea 100644
--- a/src/db/backup.rs
+++ b/src/db/backup.rs
@@ -1,14 +1,8 @@
-use std::{
-    ffi::{c_int, CStr, CString},
-    ptr::NonNull,
-    str::from_utf8,
+use rusqlite::{
+    backup::{self},
+    Connection,
 };
-
-use libsqlite3_sys::{
-    sqlite3, sqlite3_backup, sqlite3_backup_finish, sqlite3_backup_init, sqlite3_backup_step,
-    sqlite3_errmsg, sqlite3_errstr, sqlite3_extended_errcode, SQLITE_DONE, SQLITE_OK,
-};
-use sqlx::sqlite::SqlitePool;
+use sqlx::sqlite::{LockedSqliteHandle, SqlitePool};
 
 pub struct Builder<'p> {
     from: &'p SqlitePool,
@@ -37,63 +31,39 @@ impl<'p> Backup<'p> {
 impl<'p> Backup<'p> {
     pub async fn backup(&mut self) -> Result<(), Error> {
         let mut to = self.to.acquire().await?;
-        let mut to = to.lock_handle().await?;
+        let mut to = Self::connection(&mut to.lock_handle().await?)?;
         let mut from = self.from.acquire().await?;
-        let mut from = from.lock_handle().await?;
-
-        let handle = Self::start(to.as_raw_handle(), from.as_raw_handle())?;
-        let step_result = loop {
-            match Self::step(handle, -1) {
-                Err(error) => break Err(error),
-                Ok(SQLITE_DONE) => break Ok(()),
-                Ok(SQLITE_OK) => (), // keep pumping the backup step function
-                Ok(other) => panic!("unexpected step result: {other}"),
+        let from = Self::connection(&mut from.lock_handle().await?)?;
+
+        let backup = backup::Backup::new(&from, &mut to)?;
+        loop {
+            use rusqlite::backup::StepResult::{Busy, Done, Locked, More};
+            match backup.step(-1)? {
+                Done => break Ok(()),
+                // In the system as actually written, yielding does nothing: this is the
+                // only task actually in flight at the time. However, that may change; if
+                // it does, we _definitely_ want to give other tasks an opportunity to
+                // make progress before we retry.
+                More => tokio::task::consume_budget().await,
+                // If another task is actually using the DB, then it _definitely_ needs
+                // an opportunity to make progress before we retry.
+                Busy | Locked => tokio::task::yield_now().await,
+                // As of this writing, this arm is formally unreachable: all of the enum
+                // constants that actually exist in rusqlite are matched above. However,
+                // they've reserved the prerogative to add additional arms without a
+                // breaking change by making the type as non-exhaustive; if we get one,
+                // stop immediately as there's no way to guess what the correct handling
+                // will be, a priori.
+                other => panic!("unexpected backup step result: {other:?}"),
             }
-        };
-        Self::finish(handle)?;
-
-        step_result
-    }
-
-    fn start(
-        to: NonNull<sqlite3>,
-        from: NonNull<sqlite3>,
-    ) -> Result<NonNull<sqlite3_backup>, Error> {
-        let name = CString::new("main").expect("static constant is a valid C string");
-        // Invariants:
-        //
-        // * `to` and `from` must be valid `sqlite3` pointers (guaranteed by sqlx)
-        // * `zDestName` and `zSourceName` must be valid C strings (see above)
-        //
-        // Never evaluates to null (even though `sqlite3_backup_init` can).
-        let handle = unsafe {
-            sqlite3_backup_init(to.as_ptr(), name.as_ptr(), from.as_ptr(), name.as_ptr())
-        };
-        if handle.is_null() {
-            Err(Error::from_handle(to))?;
         }
-        // Having proven that `handle` is not null, we could use new_unchecked here.
-        // Choosing not to so that any mistakes are caught, rather than causing
-        // undefined behaviour later on.
-        Ok(NonNull::new(handle).expect("backup handle is non-null"))
     }
 
-    fn step(handle: NonNull<sqlite3_backup>, pages: c_int) -> Result<c_int, Error> {
-        let step = unsafe { sqlite3_backup_step(handle.as_ptr(), pages) };
-        if [SQLITE_DONE, SQLITE_OK].contains(&step) {
-            Ok(step)
-        } else {
-            Err(Error::from_code(step))
-        }
-    }
+    fn connection(handle: &mut LockedSqliteHandle) -> Result<Connection, rusqlite::Error> {
+        let handle = handle.as_raw_handle();
+        let connection = unsafe { Connection::from_handle(handle.as_ptr()) }?;
 
-    fn finish(handle: NonNull<sqlite3_backup>) -> Result<(), Error> {
-        let finished = unsafe { sqlite3_backup_finish(handle.as_ptr()) };
-        if SQLITE_OK == finished {
-            Ok(())
-        } else {
-            Err(Error::from_code(finished))
-        }
+        Ok(connection)
     }
 }
 
@@ -101,44 +71,6 @@ impl<'p> Backup<'p> {
 pub enum Error {
     #[error(transparent)]
     Sqlx(#[from] sqlx::Error),
-    #[error("backup failed: {message} (code={code})")]
-    Backup { code: c_int, message: String },
-}
-
-impl Error {
-    pub fn from_handle(handle: NonNull<sqlite3>) -> Self {
-        Self::Backup {
-            code: Self::code_for(handle),
-            message: Self::message_for(handle),
-        }
-    }
-
-    pub fn from_code(code: c_int) -> Self {
-        Self::Backup {
-            code,
-            message: Self::message_from_code(code),
-        }
-    }
-
-    fn code_for(handle: NonNull<sqlite3>) -> c_int {
-        unsafe { sqlite3_extended_errcode(handle.as_ptr()) }
-    }
-
-    fn message_for(handle: NonNull<sqlite3>) -> String {
-        Self::message_from(|| unsafe { sqlite3_errmsg(handle.as_ptr()) })
-    }
-
-    fn message_from_code(code: c_int) -> String {
-        Self::message_from(|| unsafe { sqlite3_errstr(code) })
-    }
-
-    fn message_from(f: impl FnOnce() -> *const i8) -> String {
-        let msg = f();
-        debug_assert!(!msg.is_null());
-        from_utf8(unsafe { CStr::from_ptr(msg) }.to_bytes())
-            // This is actually promised in the Sqlite3 docs, but we check anyways to catch
-            // mistakes. See <https://www.sqlite.org/c3ref/errcode.html>.
-            .expect("error messages from sqlite are always utf-8")
-            .to_owned()
-    }
+    #[error(transparent)]
+    Rusqlite(#[from] rusqlite::Error),
 }