Abstract out disk-writing utilities from FilesystemPersister

valentinewallace · valentinewallace · commit 4e4d01828160 · 2021-02-11T16:23:58.000-05:00
diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs
@@ -1,16 +1,18 @@
+mod util;
+
 extern crate lightning;
 extern crate bitcoin;
 extern crate libc;
 
 use bitcoin::hashes::hex::ToHex;
+use crate::util::DiskWriteable;
 use lightning::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateErr};
 use lightning::chain::channelmonitor;
 use lightning::chain::keysinterface::ChannelKeys;
 use lightning::chain::transaction::OutPoint;
 use lightning::util::ser::Writeable;
 use std::fs;
 use std::io::Error;
-use std::path::{Path, PathBuf};
 
 #[cfg(test)]
 use {
@@ -22,9 +24,6 @@ use {
 	std::io::Cursor
 };
 
-#[cfg(not(target_os = "windows"))]
-use std::os::unix::io::AsRawFd;
-
 /// FilesystemPersister persists channel data on disk, where each channel's
 /// data is stored in a file named after its funding outpoint.
 ///
@@ -41,13 +40,9 @@ pub struct FilesystemPersister {
 	path_to_channel_data: String,
 }
 
-trait DiskWriteable {
-	fn write(&self, writer: &mut fs::File) -> Result<(), Error>;
-}
-
 impl<ChanSigner: ChannelKeys> DiskWriteable for ChannelMonitor<ChanSigner> {
-	fn write(&self, writer: &mut fs::File) -> Result<(), Error> {
-		Writeable::write(self, writer)
+	fn write_to_file(&self, writer: &mut fs::File) -> Result<(), Error> {
+		self.write(writer)
 	}
 }
 
@@ -60,41 +55,6 @@ impl FilesystemPersister {
 		}
 	}
 
-	fn get_full_filepath(&self, funding_txo: OutPoint) -> String {
-		let mut path = PathBuf::from(&self.path_to_channel_data);
-		path.push(format!("{}_{}", funding_txo.txid.to_hex(), funding_txo.index));
-		path.to_str().unwrap().to_string()
-	}
-
-	// Utility to write a file to disk.
-	fn write_channel_data(&self, funding_txo: OutPoint, monitor: &dyn DiskWriteable) -> std::io::Result<()> {
-		fs::create_dir_all(&self.path_to_channel_data)?;
-		// Do a crazy dance with lots of fsync()s to be overly cautious here...
-		// We never want to end up in a state where we've lost the old data, or end up using the
-		// old data on power loss after we've returned.
-		// The way to atomically write a file on Unix platforms is:
-		// open(tmpname), write(tmpfile), fsync(tmpfile), close(tmpfile), rename(), fsync(dir)
-		let filename = self.get_full_filepath(funding_txo);
-		let tmp_filename = format!("{}.tmp", filename.clone());
-
-		{
-			// Note that going by rust-lang/rust@d602a6b, on MacOS it is only safe to use
-			// rust stdlib 1.36 or higher.
-			let mut f = fs::File::create(&tmp_filename)?;
-			monitor.write(&mut f)?;
-			f.sync_all()?;
-		}
-		fs::rename(&tmp_filename, &filename)?;
-		// Fsync the parent directory on Unix.
-		#[cfg(not(target_os = "windows"))]
-		{
-			let path = Path::new(&filename).parent().unwrap();
-			let dir_file = fs::OpenOptions::new().read(true).open(path)?;
-			unsafe { libc::fsync(dir_file.as_raw_fd()); }
-		}
-		Ok(())
-	}
-
 	#[cfg(test)]
 	fn load_channel_data<Keys: KeysInterface>(&self, keys: &Keys) ->
 		Result<HashMap<OutPoint, ChannelMonitor<Keys::ChanKeySigner>>, ChannelMonitorUpdateErr> {
@@ -132,28 +92,18 @@ impl FilesystemPersister {
 
 impl<ChanSigner: ChannelKeys + Send + Sync> channelmonitor::Persist<ChanSigner> for FilesystemPersister {
 	fn persist_new_channel(&self, funding_txo: OutPoint, monitor: &ChannelMonitor<ChanSigner>) -> Result<(), ChannelMonitorUpdateErr> {
-		self.write_channel_data(funding_txo, monitor)
+		let filename = format!("{}_{}", funding_txo.txid.to_hex(), funding_txo.index);
+		util::write_to_file(self.path_to_channel_data.clone(), filename, monitor)
 		  .map_err(|_| ChannelMonitorUpdateErr::PermanentFailure)
 	}
 
 	fn update_persisted_channel(&self, funding_txo: OutPoint, _update: &ChannelMonitorUpdate, monitor: &ChannelMonitor<ChanSigner>) -> Result<(), ChannelMonitorUpdateErr> {
-		self.write_channel_data(funding_txo, monitor)
+		let filename = format!("{}_{}", funding_txo.txid.to_hex(), funding_txo.index);
+		util::write_to_file(self.path_to_channel_data.clone(), filename, monitor)
 		  .map_err(|_| ChannelMonitorUpdateErr::PermanentFailure)
 	}
 }
 
-#[cfg(test)]
-impl Drop for FilesystemPersister {
-	fn drop(&mut self) {
-		// We test for invalid directory names, so it's OK if directory removal
-		// fails.
-		match fs::remove_dir_all(&self.path_to_channel_data) {
-			Err(e) => println!("Failed to remove test persister directory: {}", e),
-			_ => {}
-		}
-	}
-}
-
 #[cfg(test)]
 mod tests {
 	extern crate lightning;
@@ -162,29 +112,29 @@ mod tests {
 	use bitcoin::blockdata::block::{Block, BlockHeader};
 	use bitcoin::hashes::hex::FromHex;
 	use bitcoin::Txid;
-	use DiskWriteable;
-	use Error;
 	use lightning::chain::channelmonitor::{Persist, ChannelMonitorUpdateErr};
 	use lightning::chain::transaction::OutPoint;
 	use lightning::{check_closed_broadcast, check_added_monitors};
 	use lightning::ln::features::InitFeatures;
 	use lightning::ln::functional_test_utils::*;
 	use lightning::ln::msgs::ErrorAction;
 	use lightning::util::events::{MessageSendEventsProvider, MessageSendEvent};
-	use lightning::util::ser::Writer;
 	use lightning::util::test_utils;
 	use std::fs;
-	use std::io;
 	#[cfg(target_os = "windows")]
 	use {
 		lightning::get_event_msg,
 		lightning::ln::msgs::ChannelMessageHandler,
 	};
 
-	struct TestWriteable{}
-	impl DiskWriteable for TestWriteable {
-		fn write(&self, writer: &mut fs::File) -> Result<(), Error> {
-			writer.write_all(&[42; 1])
+	impl Drop for FilesystemPersister {
+		fn drop(&mut self) {
+			// We test for invalid directory names, so it's OK if directory removal
+			// fails.
+			match fs::remove_dir_all(&self.path_to_channel_data) {
+				Err(e) => println!("Failed to remove test persister directory: {}", e),
+				_ => {}
+			}
 		}
 	}
 
@@ -256,88 +206,6 @@ mod tests {
 		check_persisted_data!(11);
 	}
 
-	// Test that if the persister's path to channel data is read-only, writing
-	// data to it fails. Windows ignores the read-only flag for folders, so this
-	// test is Unix-only.
-	#[cfg(not(target_os = "windows"))]
-	#[test]
-	fn test_readonly_dir() {
-		let persister = FilesystemPersister::new("test_readonly_dir_persister".to_string());
-		let test_writeable = TestWriteable{};
-		let test_txo = OutPoint {
-			txid: Txid::from_hex("8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be").unwrap(),
-			index: 0
-		};
-		// Create the persister's directory and set it to read-only.
-		let path = &persister.path_to_channel_data;
-		fs::create_dir_all(path).unwrap();
-		let mut perms = fs::metadata(path).unwrap().permissions();
-		perms.set_readonly(true);
-		fs::set_permissions(path, perms).unwrap();
-		match persister.write_channel_data(test_txo, &test_writeable) {
-			Err(e) => assert_eq!(e.kind(), io::ErrorKind::PermissionDenied),
-			_ => panic!("Unexpected error message")
-		}
-	}
-
-	// Test failure to rename in the process of atomically creating a channel
-	// monitor's file. We induce this failure by making the `tmp` file a
-	// directory.
-	// Explanation: given "from" = the file being renamed, "to" = the
-	// renamee that already exists: Windows should fail because it'll fail
-	// whenever "to" is a directory, and Unix should fail because if "from" is a
-	// file, then "to" is also required to be a file.
-	#[test]
-	fn test_rename_failure() {
-		let persister = FilesystemPersister::new("test_rename_failure".to_string());
-		let test_writeable = TestWriteable{};
-		let txid_hex = "8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be";
-		let outp_idx = 0;
-		let test_txo = OutPoint {
-			txid: Txid::from_hex(txid_hex).unwrap(),
-			index: outp_idx,
-		};
-		// Create the channel data file and make it a directory.
-		let path = &persister.path_to_channel_data;
-		fs::create_dir_all(format!("{}/{}_{}", path, txid_hex, outp_idx)).unwrap();
-		match persister.write_channel_data(test_txo, &test_writeable) {
-			Err(e) => {
-				#[cfg(not(target_os = "windows"))]
-				assert_eq!(e.kind(), io::ErrorKind::Other);
-				#[cfg(target_os = "windows")]
-				assert_eq!(e.kind(), io::ErrorKind::PermissionDenied);
-			}
-			_ => panic!("Unexpected error message")
-		}
-	}
-
-	// Test failure to create the temporary file in the persistence process.
-	// We induce this failure by having the temp file already exist and be a
-	// directory.
-	#[test]
-	fn test_tmp_file_creation_failure() {
-		let persister = FilesystemPersister::new("test_tmp_file_creation_failure".to_string());
-		let test_writeable = TestWriteable{};
-		let txid_hex = "8984484a580b825b9972d7adb15050b3ab624ccd731946b3eeddb92f4e7ef6be";
-		let outp_idx = 0;
-		let test_txo = OutPoint {
-			txid: Txid::from_hex(txid_hex).unwrap(),
-			index: outp_idx,
-		};
-		// Create the tmp file and make it a directory.
-		let path = &persister.path_to_channel_data;
-		fs::create_dir_all(format!("{}/{}_{}.tmp", path, txid_hex, outp_idx)).unwrap();
-		match persister.write_channel_data(test_txo, &test_writeable) {
-			Err(e) => {
-				#[cfg(not(target_os = "windows"))]
-				assert_eq!(e.kind(), io::ErrorKind::Other);
-				#[cfg(target_os = "windows")]
-				assert_eq!(e.kind(), io::ErrorKind::PermissionDenied);
-			}
-			_ => panic!("Unexpected error message")
-		}
-	}
-
 	// Test that if the persister's path to channel data is read-only, writing a
 	// monitor to it results in the persister returning a PermanentFailure.
 	// Windows ignores the read-only flag for folders, so this test is Unix-only.
diff --git a/lightning-persister/src/util.rs b/lightning-persister/src/util.rs
@@ -0,0 +1,149 @@
+use std::fs;
+use std::path::{Path, PathBuf};
+
+#[cfg(not(target_os = "windows"))]
+use std::os::unix::io::AsRawFd;
+
+pub(crate) trait DiskWriteable {
+	fn write_to_file(&self, writer: &mut fs::File) -> Result<(), std::io::Error>;
+}
+
+pub fn get_full_filepath(filepath: String, filename: String) -> String {
+	let mut path = PathBuf::from(filepath);
+	path.push(filename);
+	path.to_str().unwrap().to_string()
+}
+
+#[allow(bare_trait_objects)]
+pub(crate) fn write_to_file<D: DiskWriteable>(path: String, filename: String, data: &D) -> std::io::Result<()> {
+	fs::create_dir_all(path.clone())?;
+	// Do a crazy dance with lots of fsync()s to be overly cautious here...
+	// We never want to end up in a state where we've lost the old data, or end up using the
+	// old data on power loss after we've returned.
+	// The way to atomically write a file on Unix platforms is:
+	// open(tmpname), write(tmpfile), fsync(tmpfile), close(tmpfile), rename(), fsync(dir)
+	let filename_with_path = get_full_filepath(path, filename);
+	let tmp_filename = format!("{}.tmp", filename_with_path);
+
+	{
+		// Note that going by rust-lang/rust@d602a6b, on MacOS it is only safe to use
+		// rust stdlib 1.36 or higher.
+		let mut f = fs::File::create(&tmp_filename)?;
+		data.write_to_file(&mut f)?;
+		f.sync_all()?;
+	}
+	fs::rename(&tmp_filename, &filename_with_path)?;
+	// Fsync the parent directory on Unix.
+	#[cfg(not(target_os = "windows"))]
+	{
+		let path = Path::new(&filename_with_path).parent().unwrap();
+		let dir_file = fs::OpenOptions::new().read(true).open(path)?;
+		unsafe { libc::fsync(dir_file.as_raw_fd()); }
+	}
+	Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+	use super::{DiskWriteable, get_full_filepath, write_to_file};
+	use std::fs;
+	use std::io;
+	use std::io::Write;
+
+	struct TestWriteable{}
+	impl DiskWriteable for TestWriteable {
+		fn write_to_file(&self, writer: &mut fs::File) -> Result<(), io::Error> {
+			writer.write_all(&[42; 1])
+		}
+	}
+
+	// Test that if the persister's path to channel data is read-only, writing
+	// data to it fails. Windows ignores the read-only flag for folders, so this
+	// test is Unix-only.
+	#[cfg(not(target_os = "windows"))]
+	#[test]
+	fn test_readonly_dir() {
+		let test_writeable = TestWriteable{};
+		let filename = "test_readonly_dir_persister_filename".to_string();
+		let path = "test_readonly_dir_persister_dir";
+		fs::create_dir_all(path.to_string()).unwrap();
+		let mut perms = fs::metadata(path.to_string()).unwrap().permissions();
+		perms.set_readonly(true);
+		fs::set_permissions(path.to_string(), perms).unwrap();
+		match write_to_file(path.to_string(), filename, &test_writeable) {
+			Err(e) => assert_eq!(e.kind(), io::ErrorKind::PermissionDenied),
+			_ => panic!("Unexpected error message")
+		}
+	}
+
+	// Test failure to rename in the process of atomically creating a channel
+	// monitor's file. We induce this failure by making the `tmp` file a
+	// directory.
+	// Explanation: given "from" = the file being renamed, "to" = the
+	// renamee that already exists: Windows should fail because it'll fail
+	// whenever "to" is a directory, and Unix should fail because if "from" is a
+	// file, then "to" is also required to be a file.
+	#[test]
+	fn test_rename_failure() {
+		let test_writeable = TestWriteable{};
+		let filename = "test_rename_failure_filename";
+		let path = "test_rename_failure_dir";
+		// Create the channel data file and make it a directory.
+		fs::create_dir_all(get_full_filepath(path.to_string(), filename.to_string())).unwrap();
+		match write_to_file(path.to_string(), filename.to_string(), &test_writeable) {
+			Err(e) => {
+				#[cfg(not(target_os = "windows"))]
+				assert_eq!(e.kind(), io::ErrorKind::Other);
+				#[cfg(target_os = "windows")]
+				assert_eq!(e.kind(), io::ErrorKind::PermissionDenied);
+			}
+			_ => panic!("Unexpected error message")
+		}
+		fs::remove_dir_all(path).unwrap();
+	}
+
+	#[test]
+	fn test_diskwriteable_failure() {
+		struct FailingWriteable {}
+		impl DiskWriteable for FailingWriteable {
+			fn write_to_file(&self, _writer: &mut fs::File) -> Result<(), std::io::Error> {
+				Err(std::io::Error::new(std::io::ErrorKind::Other, "expected failure"))
+			}
+		}
+
+		let filename = "test_diskwriteable_failure";
+		let path = "test_diskwriteable_failure_dir";
+		let test_writeable = FailingWriteable{};
+		match write_to_file(path.to_string(), filename.to_string(), &test_writeable) {
+			Err(e) => {
+				assert_eq!(e.kind(), std::io::ErrorKind::Other);
+				assert_eq!(e.get_ref().unwrap().to_string(), "expected failure");
+			},
+			_ => panic!("unexpected result")
+		}
+		fs::remove_dir_all(path).unwrap();
+	}
+
+	// Test failure to create the temporary file in the persistence process.
+	// We induce this failure by having the temp file already exist and be a
+	// directory.
+	#[test]
+	fn test_tmp_file_creation_failure() {
+		let test_writeable = TestWriteable{};
+		let filename = "test_tmp_file_creation_failure_filename".to_string();
+		let path = "test_tmp_file_creation_failure_dir".to_string();
+
+		// Create the tmp file and make it a directory.
+		let tmp_path = get_full_filepath(path.clone(), format!("{}.tmp", filename.clone()));
+		fs::create_dir_all(tmp_path).unwrap();
+		match write_to_file(path, filename, &test_writeable) {
+			Err(e) => {
+				#[cfg(not(target_os = "windows"))]
+				assert_eq!(e.kind(), io::ErrorKind::Other);
+				#[cfg(target_os = "windows")]
+				assert_eq!(e.kind(), io::ErrorKind::PermissionDenied);
+			}
+			_ => panic!("Unexpected error message")
+		}
+	}
+}
