From 36c89315fad651b3b8ce1f8e25310db3ea668fae Mon Sep 17 00:00:00 2001 From: Alexander Batischev Date: Sun, 25 Apr 2021 18:29:19 +0300 Subject: Expose fallibility of individual SQL results `LocalGeneration::sql::files()` runs an SQL query, iterates over the results and collects the rows into a `Vec`. This can fail at any step: the query might fail to run, or one of the rows might fail to be fetched or processed. Right now, we lump all those failures into a `Result` that wraps the whole return value. This is only possible because we process each row before returning. Once `Vec` is replaced by an iterator, we won't have that luxury anymore, so we now wrap each individual element into its own `Result` (as well as wrapping the whole vector into a `Result` of its own). --- src/cmd/list_files.rs | 1 + src/cmd/restore.rs | 2 ++ src/cmd/show_gen.rs | 17 ++++++++++------- src/generation.rs | 20 ++++++++++++++------ 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/cmd/list_files.rs b/src/cmd/list_files.rs index 22b102e..7f0cfed 100644 --- a/src/cmd/list_files.rs +++ b/src/cmd/list_files.rs @@ -23,6 +23,7 @@ impl ListFiles { let gen = client.fetch_generation(&gen_id, temp.path())?; for file in gen.files()? { + let file = file?; println!("{}", format_entry(&file.entry(), file.reason())); } diff --git a/src/cmd/restore.rs b/src/cmd/restore.rs index a321e80..f8bbaf4 100644 --- a/src/cmd/restore.rs +++ b/src/cmd/restore.rs @@ -41,6 +41,7 @@ impl Restore { info!("restoring {} files", gen.file_count()?); let progress = create_progress_bar(gen.file_count()?, true); for file in gen.files()? { + let file = file?; match file.reason() { Reason::FileError => (), _ => restore_generation( @@ -54,6 +55,7 @@ impl Restore { } } for file in gen.files()? { + let file = file?; if file.entry().is_dir() { restore_directory_metadata(file.entry(), &self.to)?; } diff --git a/src/cmd/show_gen.rs b/src/cmd/show_gen.rs index ba39809..7dc52cf 100644 --- a/src/cmd/show_gen.rs +++ b/src/cmd/show_gen.rs @@ -23,14 +23,17 @@ impl ShowGeneration { let gen = client.fetch_generation(&gen_id, temp.path())?; let files = gen.files()?; - let total_bytes = files.iter().fold(0, |acc, file| { - let e = file.entry(); - if e.kind() == FilesystemKind::Regular { - acc + file.entry().len() - } else { - acc - } + let total_bytes = files.into_iter().try_fold(0, |acc, file| { + file.map(|file| { + let e = file.entry(); + if e.kind() == FilesystemKind::Regular { + acc + e.len() + } else { + acc + } + }) }); + let total_bytes = total_bytes?; println!("generation-id: {}", gen_id); println!("file-count: {}", gen.file_count()?); diff --git a/src/generation.rs b/src/generation.rs index 1d0ee52..a780203 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -196,7 +196,7 @@ impl LocalGeneration { sql::file_count(&self.conn) } - pub fn files(&self) -> LocalGenerationResult> { + pub fn files(&self) -> LocalGenerationResult>> { sql::files(&self.conn) } @@ -289,14 +289,22 @@ mod sql { Ok(count) } - pub fn files(conn: &Connection) -> LocalGenerationResult> { + pub fn files( + conn: &Connection, + ) -> LocalGenerationResult>> { let mut stmt = conn.prepare("SELECT * FROM files")?; let iter = stmt.query_map(params![], |row| row_to_entry(row))?; - let mut files = vec![]; + let mut files: Vec> = vec![]; for x in iter { - let (fileno, json, reason) = x?; - let entry = serde_json::from_str(&json)?; - files.push(BackedUpFile::new(fileno, entry, &reason)); + match x { + Ok((fileno, json, reason)) => { + let result = serde_json::from_str(&json) + .map(|entry| BackedUpFile::new(fileno, entry, &reason)) + .map_err(|e| e.into()); + files.push(result) + } + Err(e) => files.push(Err(e.into())), + } } Ok(files) } -- cgit v1.2.1 From f21e6e0280b094efb72736f53931ecf0e7c2c0a2 Mon Sep 17 00:00:00 2001 From: Alexander Batischev Date: Sun, 25 Apr 2021 21:15:04 +0300 Subject: Use an iterator internally for LocalGeneration This adds the machinery. We have to keep the compiled SQL query while the iterator is in use, so we wrap it in an `SqlResults` struct which the iterator borrows. --- src/generation.rs | 51 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/src/generation.rs b/src/generation.rs index a780203..e5ca14c 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -197,7 +197,7 @@ impl LocalGeneration { } pub fn files(&self) -> LocalGenerationResult>> { - sql::files(&self.conn) + Ok(sql::files(&self.conn)?.iter()?.collect()) } pub fn chunkids(&self, fileno: FileId) -> LocalGenerationResult> { @@ -221,7 +221,7 @@ mod sql { use crate::backup_reason::Reason; use crate::chunkid::ChunkId; use crate::fsentry::FilesystemEntry; - use rusqlite::{params, Connection, OpenFlags, Row, Transaction}; + use rusqlite::{params, Connection, OpenFlags, Row, Statement, Transaction}; use std::os::unix::ffi::OsStrExt; use std::path::Path; @@ -289,24 +289,37 @@ mod sql { Ok(count) } - pub fn files( - conn: &Connection, - ) -> LocalGenerationResult>> { - let mut stmt = conn.prepare("SELECT * FROM files")?; - let iter = stmt.query_map(params![], |row| row_to_entry(row))?; - let mut files: Vec> = vec![]; - for x in iter { - match x { - Ok((fileno, json, reason)) => { - let result = serde_json::from_str(&json) - .map(|entry| BackedUpFile::new(fileno, entry, &reason)) - .map_err(|e| e.into()); - files.push(result) - } - Err(e) => files.push(Err(e.into())), - } + // A pointer to an iterator over values of type `LocalGenerationResult`. The iterator is + // only valid for the lifetime 'stmt. + // + // The fact that it's a pointer (`Box`) means we don't care what the actual type of + // the iterator is, and who produces it. + type SqlResultsIterator<'stmt, T> = Box> + 'stmt>; + + pub struct SqlResults<'conn> { + stmt: Statement<'conn>, + } + + impl<'conn> SqlResults<'conn> { + fn new(conn: &'conn Connection, statement: &str) -> LocalGenerationResult { + let stmt = conn.prepare(statement)?; + Ok(Self { stmt }) + } + + pub fn iter(&'_ mut self) -> LocalGenerationResult> { + let iter = self.stmt.query_map(params![], |row| row_to_entry(row))?; + let iter = iter.map(|x| match x { + Ok((fileno, json, reason)) => serde_json::from_str(&json) + .map(|entry| BackedUpFile::new(fileno, entry, &reason)) + .map_err(|e| e.into()), + Err(e) => Err(e.into()), + }); + Ok(iter) } - Ok(files) + } + + pub fn files(conn: &Connection) -> LocalGenerationResult> { + SqlResults::new(conn, "SELECT * FROM files") } pub fn chunkids(conn: &Connection, fileno: FileId) -> LocalGenerationResult> { -- cgit v1.2.1 From 2991aade4d71d28dcf30d089e54269934e4885b6 Mon Sep 17 00:00:00 2001 From: Alexander Batischev Date: Sun, 25 Apr 2021 21:31:11 +0300 Subject: Extend the iterator to users of LocalGeneration::files() --- src/cmd/list_files.rs | 2 +- src/cmd/restore.rs | 4 ++-- src/cmd/show_gen.rs | 5 +++-- src/generation.rs | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/cmd/list_files.rs b/src/cmd/list_files.rs index 7f0cfed..c5191f7 100644 --- a/src/cmd/list_files.rs +++ b/src/cmd/list_files.rs @@ -22,7 +22,7 @@ impl ListFiles { let gen_id: String = genlist.resolve(&self.gen_id)?; let gen = client.fetch_generation(&gen_id, temp.path())?; - for file in gen.files()? { + for file in gen.files()?.iter()? { let file = file?; println!("{}", format_entry(&file.entry(), file.reason())); } diff --git a/src/cmd/restore.rs b/src/cmd/restore.rs index f8bbaf4..91045f1 100644 --- a/src/cmd/restore.rs +++ b/src/cmd/restore.rs @@ -40,7 +40,7 @@ impl Restore { let gen = client.fetch_generation(&gen_id, temp.path())?; info!("restoring {} files", gen.file_count()?); let progress = create_progress_bar(gen.file_count()?, true); - for file in gen.files()? { + for file in gen.files()?.iter()? { let file = file?; match file.reason() { Reason::FileError => (), @@ -54,7 +54,7 @@ impl Restore { )?, } } - for file in gen.files()? { + for file in gen.files()?.iter()? { let file = file?; if file.entry().is_dir() { restore_directory_metadata(file.entry(), &self.to)?; diff --git a/src/cmd/show_gen.rs b/src/cmd/show_gen.rs index 7dc52cf..df8a030 100644 --- a/src/cmd/show_gen.rs +++ b/src/cmd/show_gen.rs @@ -21,9 +21,10 @@ impl ShowGeneration { let genlist = client.list_generations()?; let gen_id: String = genlist.resolve(&self.gen_id)?; let gen = client.fetch_generation(&gen_id, temp.path())?; - let files = gen.files()?; + let mut files = gen.files()?; + let mut files = files.iter()?; - let total_bytes = files.into_iter().try_fold(0, |acc, file| { + let total_bytes = files.try_fold(0, |acc, file| { file.map(|file| { let e = file.entry(); if e.kind() == FilesystemKind::Regular { diff --git a/src/generation.rs b/src/generation.rs index e5ca14c..10143ab 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -196,8 +196,8 @@ impl LocalGeneration { sql::file_count(&self.conn) } - pub fn files(&self) -> LocalGenerationResult>> { - Ok(sql::files(&self.conn)?.iter()?.collect()) + pub fn files(&self) -> LocalGenerationResult { + sql::files(&self.conn) } pub fn chunkids(&self, fileno: FileId) -> LocalGenerationResult> { -- cgit v1.2.1 From 7176a544757eacc2823f1c0f014861ad2525e6da Mon Sep 17 00:00:00 2001 From: Alexander Batischev Date: Mon, 26 Apr 2021 11:41:02 +0300 Subject: Generalize the machinery to arbitrary result types --- src/generation.rs | 73 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/src/generation.rs b/src/generation.rs index 10143ab..54c55a7 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -196,7 +196,7 @@ impl LocalGeneration { sql::file_count(&self.conn) } - pub fn files(&self) -> LocalGenerationResult { + pub fn files(&self) -> LocalGenerationResult> { sql::files(&self.conn) } @@ -289,37 +289,74 @@ mod sql { Ok(count) } - // A pointer to an iterator over values of type `LocalGenerationResult`. The iterator is - // only valid for the lifetime 'stmt. + // A pointer to a "fallible iterator" over values of type `T`, which is to say it's an iterator + // over values of type `LocalGenerationResult`. The iterator is only valid for the lifetime + // 'stmt. // // The fact that it's a pointer (`Box`) means we don't care what the actual type of // the iterator is, and who produces it. type SqlResultsIterator<'stmt, T> = Box> + 'stmt>; - pub struct SqlResults<'conn> { + // A pointer to a function which, when called on a prepared SQLite statement, would create + // a "fallible iterator" over values of type `ItemT`. (See above for an explanation of what + // a "fallible iterator" is.) + // + // The iterator is only valid for the lifetime of the associated SQLite statement; we + // call this lifetime 'stmt, and use it both both on the reference and the returned iterator. + // + // Now we're in a pickle: all named lifetimes have to be declared _somewhere_, but we can't add + // 'stmt to the signature of `CreateIterFn` because then we'll have to specify it when we + // define the function. Obviously, at that point we won't yet have a `Statement`, and thus we + // would have no idea what its lifetime is going to be. So we can't put the 'stmt lifetime into + // the signature of `CreateIterFn`. + // + // That's what `for<'stmt>` is for. This is a so-called ["higher-rank trait bound"][hrtb], and + // it enables us to say that a function is valid for *some* lifetime 'stmt that we pass into it + // at the call site. It lets Rust continue to track lifetimes even though `CreateIterFn` + // interferes by "hiding" the 'stmt lifetime from its signature. + // + // [hrtb]: https://doc.rust-lang.org/nomicon/hrtb.html + type CreateIterFn<'conn, ItemT> = Box< + dyn for<'stmt> Fn( + &'stmt mut Statement<'conn>, + ) -> LocalGenerationResult>, + >; + + pub struct SqlResults<'conn, ItemT> { stmt: Statement<'conn>, + create_iter: CreateIterFn<'conn, ItemT>, } - impl<'conn> SqlResults<'conn> { - fn new(conn: &'conn Connection, statement: &str) -> LocalGenerationResult { + impl<'conn, ItemT> SqlResults<'conn, ItemT> { + fn new( + conn: &'conn Connection, + statement: &str, + create_iter: CreateIterFn<'conn, ItemT>, + ) -> LocalGenerationResult { let stmt = conn.prepare(statement)?; - Ok(Self { stmt }) + Ok(Self { stmt, create_iter }) } - pub fn iter(&'_ mut self) -> LocalGenerationResult> { - let iter = self.stmt.query_map(params![], |row| row_to_entry(row))?; - let iter = iter.map(|x| match x { - Ok((fileno, json, reason)) => serde_json::from_str(&json) - .map(|entry| BackedUpFile::new(fileno, entry, &reason)) - .map_err(|e| e.into()), - Err(e) => Err(e.into()), - }); - Ok(iter) + pub fn iter(&'_ mut self) -> LocalGenerationResult> { + (self.create_iter)(&mut self.stmt) } } - pub fn files(conn: &Connection) -> LocalGenerationResult> { - SqlResults::new(conn, "SELECT * FROM files") + pub fn files(conn: &Connection) -> LocalGenerationResult> { + SqlResults::new( + conn, + "SELECT * FROM files", + Box::new(|stmt| { + let iter = stmt.query_map(params![], |row| row_to_entry(row))?; + let iter = iter.map(|x| match x { + Ok((fileno, json, reason)) => serde_json::from_str(&json) + .map(|entry| BackedUpFile::new(fileno, entry, &reason)) + .map_err(|e| e.into()), + Err(e) => Err(e.into()), + }); + Ok(Box::new(iter)) + }), + ) } pub fn chunkids(conn: &Connection, fileno: FileId) -> LocalGenerationResult> { -- cgit v1.2.1 From 4ae0960619537234d5591b40d05f91b131330618 Mon Sep 17 00:00:00 2001 From: Alexander Batischev Date: Mon, 26 Apr 2021 13:41:58 +0300 Subject: Port chunkids() to the iterator API --- src/backup_run.rs | 6 +++++- src/cmd/restore.rs | 3 ++- src/generation.rs | 27 +++++++++++++++++---------- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/backup_run.rs b/src/backup_run.rs index e966855..23c97f6 100644 --- a/src/backup_run.rs +++ b/src/backup_run.rs @@ -149,7 +149,11 @@ impl<'a> IncrementalBackup<'a> { Reason::Unchanged | Reason::Skipped | Reason::FileError => { let fileno = old.get_fileno(&entry.pathbuf())?; let ids = if let Some(fileno) = fileno { - old.chunkids(fileno)? + let mut ids = vec![]; + for id in old.chunkids(fileno)?.iter()? { + ids.push(id?); + } + ids } else { vec![] }; diff --git a/src/cmd/restore.rs b/src/cmd/restore.rs index 91045f1..4de0830 100644 --- a/src/cmd/restore.rs +++ b/src/cmd/restore.rs @@ -172,7 +172,8 @@ fn restore_regular( std::fs::create_dir_all(parent)?; { let mut file = std::fs::File::create(path)?; - for chunkid in gen.chunkids(fileid)? { + for chunkid in gen.chunkids(fileid)?.iter()? { + let chunkid = chunkid?; let chunk = client.fetch_chunk(&chunkid)?; file.write_all(chunk.data())?; } diff --git a/src/generation.rs b/src/generation.rs index 54c55a7..0055bfe 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -200,7 +200,7 @@ impl LocalGeneration { sql::files(&self.conn) } - pub fn chunkids(&self, fileno: FileId) -> LocalGenerationResult> { + pub fn chunkids(&self, fileno: FileId) -> LocalGenerationResult> { sql::chunkids(&self.conn, fileno) } @@ -359,15 +359,22 @@ mod sql { ) } - pub fn chunkids(conn: &Connection, fileno: FileId) -> LocalGenerationResult> { - let mut stmt = conn.prepare("SELECT chunkid FROM chunks WHERE fileno = ?1")?; - let iter = stmt.query_map(params![fileno], |row| row.get(0))?; - let mut ids: Vec = vec![]; - for x in iter { - let fileno: String = x?; - ids.push(ChunkId::from(&fileno)); - } - Ok(ids) + pub fn chunkids( + conn: &Connection, + fileno: FileId, + ) -> LocalGenerationResult> { + SqlResults::new( + conn, + "SELECT chunkid FROM chunks WHERE fileno = ?1", + Box::new(move |stmt| { + let iter = stmt.query_map(params![fileno], |row| row.get(0))?; + let iter = iter.map(|x| { + let fileno: String = x?; + Ok(ChunkId::from(&fileno)) + }); + Ok(Box::new(iter)) + }), + ) } pub fn get_file( -- cgit v1.2.1