From 11d8c2280539f08d55c5bd0b019adbd5cfa373dc Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Tue, 16 May 2023 16:21:43 -0400 Subject: [PATCH] fix: don't panic on missing shorebird.yaml (#18) * fix: don't panic on missing shorebird.yaml * fix next_boot_patch C api * don't log error in updater * Cleanup * Remove anyhow::bail import --- library/src/c_api.rs | 56 +++++++++--------- library/src/config.rs | 20 +++---- library/src/lib.rs | 2 +- library/src/updater.rs | 128 ++++++++++++++++++++--------------------- 4 files changed, 98 insertions(+), 108 deletions(-) diff --git a/library/src/c_api.rs b/library/src/c_api.rs index fb3c4f22..b56aaead 100644 --- a/library/src/c_api.rs +++ b/library/src/c_api.rs @@ -7,13 +7,11 @@ use std::ffi::{CStr, CString}; use std::os::raw::c_char; -use anyhow::Context; - use crate::updater; // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests #[cfg(test)] -use std::println as error; // Workaround to use println! for logs. +use std::{println as info, println as error}; // Workaround to use println! for logs. /// Struct containing configuration parameters for the updater. /// Passed to all updater functions. @@ -49,6 +47,14 @@ fn allocate_c_string(rust_string: &str) -> anyhow::Result<*mut c_char> { Ok(c_str.into_raw()) } +/// Converts a Rust string to a C string, caller must free the C string. +fn to_c_string(maybe_string: Option) -> anyhow::Result<*mut c_char> { + Ok(match maybe_string { + Some(v) => allocate_c_string(&v)?, + None => std::ptr::null_mut(), + }) +} + fn to_rust_vector( c_array: *const *const libc::c_char, size: libc::c_int, @@ -83,14 +89,10 @@ fn log_on_error(f: F, context: &str, error_result: R) -> R where F: FnOnce() -> Result, { - let result = f(); - match result { - Ok(r) => r, - Err(e) => { - error!("Error {}: {:?}", context, e); - error_result - } - } + f().unwrap_or_else(|e| { + error!("Error {}: {:?}", context, e); + error_result + }) } /// Configures updater. First parameter is a struct containing configuration @@ -119,8 +121,8 @@ pub extern "C" fn shorebird_init( pub extern "C" fn shorebird_next_boot_patch_number() -> *mut c_char { log_on_error( || { - let patch = updater::next_boot_patch().context("failed to fetch patch info")?; - allocate_c_string(&patch.number.to_string()) + let maybe_patch_number = updater::next_boot_patch()?.map(|p| p.number.to_string()); + to_c_string(maybe_patch_number) }, "fetching next_boot_patch_number", std::ptr::null_mut(), @@ -133,8 +135,8 @@ pub extern "C" fn shorebird_next_boot_patch_number() -> *mut c_char { pub extern "C" fn shorebird_next_boot_patch_path() -> *mut c_char { log_on_error( || { - let patch = updater::next_boot_patch().context("failed to fetch patch info")?; - allocate_c_string(&patch.path) + let maybe_path = updater::next_boot_patch()?.map(|p| p.path); + to_c_string(maybe_path) }, "fetching next_boot_patch_path", std::ptr::null_mut(), @@ -155,13 +157,17 @@ pub extern "C" fn shorebird_free_string(c_string: *mut c_char) { /// Check for an update. Returns true if an update is available. #[no_mangle] pub extern "C" fn shorebird_check_for_update() -> bool { - return updater::check_for_update(); + log_on_error(updater::check_for_update, "checking for update", false) } /// Synchronously download an update if one is available. #[no_mangle] pub extern "C" fn shorebird_update() { - updater::update(); + log_on_error( + || updater::update().and_then(|result| Ok(info!("Update result: {}", result))), + "downloading update", + (), + ); } /// Start a thread to download an update if one is available. @@ -177,11 +183,7 @@ pub extern "C" fn shorebird_start_update_thread() { /// shorebird_report_launch_success or shorebird_report_launch_failure. #[no_mangle] pub extern "C" fn shorebird_report_launch_start() { - log_on_error( - || updater::report_launch_start(), - "reporting launch start", - (), - ); + log_on_error(updater::report_launch_start, "reporting launch start", ()); } /// Report that the app failed to launch. This will cause the updater to @@ -190,10 +192,7 @@ pub extern "C" fn shorebird_report_launch_start() { #[no_mangle] pub extern "C" fn shorebird_report_launch_failure() { log_on_error( - || { - updater::report_launch_failure()?; - Ok(()) - }, + updater::report_launch_failure, "reporting launch failure", (), ); @@ -209,10 +208,7 @@ pub extern "C" fn shorebird_report_launch_failure() { #[no_mangle] pub extern "C" fn shorebird_report_launch_success() { log_on_error( - || { - updater::report_launch_success()?; - Ok(()) - }, + updater::report_launch_success, "reporting launch success", (), ); diff --git a/library/src/config.rs b/library/src/config.rs index fad35674..7ef1508a 100644 --- a/library/src/config.rs +++ b/library/src/config.rs @@ -1,5 +1,4 @@ // This file handles the global config for the updater library. - #[cfg(test)] use crate::network::{DownloadFileFn, PatchCheckRequestFn}; #[cfg(test)] @@ -7,6 +6,7 @@ use std::cell::RefCell; use crate::updater::AppConfig; use crate::yaml::YamlConfig; +use crate::UpdateError; #[cfg(not(test))] use once_cell::sync::OnceCell; @@ -60,20 +60,20 @@ pub fn testing_reset_config() { }); } -pub fn check_initialized_and_call(f: F, config: &ResolvedConfig) -> R +pub fn check_initialized_and_call(f: F, config: &ResolvedConfig) -> anyhow::Result where - F: FnOnce(&ResolvedConfig) -> R, + F: FnOnce(&ResolvedConfig) -> anyhow::Result, { if !config.is_initialized { - panic!("Must call shorebird_init() before using the updater."); + anyhow::bail!(UpdateError::ConfigNotInitialized); } return f(&config); } #[cfg(test)] -pub fn with_thread_config(f: F) -> R +pub fn with_thread_config(f: F) -> anyhow::Result where - F: FnOnce(&ThreadConfig) -> R, + F: FnOnce(&ThreadConfig) -> anyhow::Result, { THREAD_CONFIG.with(|thread_config| { let thread_config = thread_config.borrow(); @@ -93,9 +93,9 @@ where } #[cfg(not(test))] -pub fn with_config(f: F) -> R +pub fn with_config(f: F) -> anyhow::Result where - F: FnOnce(&ResolvedConfig) -> R, + F: FnOnce(&ResolvedConfig) -> anyhow::Result, { // expect() here should be OK, it's job is to propagate a panic across // threads if the lock is poisoned. @@ -106,9 +106,9 @@ where } #[cfg(test)] -pub fn with_config(f: F) -> R +pub fn with_config(f: F) -> anyhow::Result where - F: FnOnce(&ResolvedConfig) -> R, + F: FnOnce(&ResolvedConfig) -> anyhow::Result, { // Rust unit tests run on multiple threads in parallel, so we use // a per-thread config when unit testing instead of a global config. diff --git a/library/src/lib.rs b/library/src/lib.rs index d163d808..b1b5df79 100644 --- a/library/src/lib.rs +++ b/library/src/lib.rs @@ -5,7 +5,7 @@ // C doesn't care about the namespaces, but Rust does. pub mod c_api; -// Declare other .rs file/module exists, but make them public. +// Declare other .rs file/module exists, but make them private. mod cache; mod config; mod logging; diff --git a/library/src/updater.rs b/library/src/updater.rs index fdf8e82f..4d828b97 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -1,15 +1,17 @@ // This file's job is to be the Rust API for the updater. use std::fmt::{Display, Formatter}; +use std::fs; +use std::io::{Cursor, Read, Seek}; +use std::path::{Path, PathBuf}; + +use anyhow::Context; use crate::cache::{PatchInfo, UpdaterState}; use crate::config::{set_config, with_config, ResolvedConfig}; use crate::logging::init_logging; use crate::network::{download_to_path, send_patch_check_request}; use crate::yaml::YamlConfig; -use std::fs; -use std::io::{Cursor, Read, Seek}; -use std::path::{Path, PathBuf}; // https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests #[cfg(test)] @@ -23,8 +25,6 @@ pub use crate::network::{ testing_set_network_hooks, DownloadFileFn, Patch, PatchCheckRequest, PatchCheckRequestFn, }; -use anyhow::Context; - pub enum UpdateStatus { NoUpdate, UpdateAvailable, @@ -51,6 +51,7 @@ pub enum UpdateError { InvalidState(String), BadServerResponse, FailedToSaveState, + ConfigNotInitialized, } impl std::error::Error for UpdateError {} @@ -64,6 +65,7 @@ impl Display for UpdateError { UpdateError::InvalidState(msg) => write!(f, "Invalid State: {}", msg), UpdateError::FailedToSaveState => write!(f, "Failed to save state"), UpdateError::BadServerResponse => write!(f, "Bad server response"), + UpdateError::ConfigNotInitialized => write!(f, "Config not initialized"), } } } @@ -89,26 +91,14 @@ pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> { set_config(app_config, config).map_err(|err| UpdateError::InvalidState(err.to_string())) } -fn check_for_update_internal(config: &ResolvedConfig) -> bool { - // Load UpdaterState from disk - // If there is no state, make an empty state. - let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); - // Send info from app + current slot to server. - let response_result = send_patch_check_request(&config, &state); - match response_result { - Err(err) => { - error!("Failed update check: {err}"); - return false; - } - Ok(response) => { - return response.patch_available; - } - } -} - /// Synchronously checks for an update and returns true if an update is available. -pub fn check_for_update() -> bool { - return with_config(check_for_update_internal); +pub fn check_for_update() -> anyhow::Result { + with_config(|config| { + // Load UpdaterState from disk + // If there is no state, make an empty state. + let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); + send_patch_check_request(&config, &state).map(|res| res.patch_available) + }) } fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result { @@ -396,21 +386,21 @@ where /// The patch which will be run on next boot (which may still be the same /// as the current boot). /// This may be changed any time update() or start_update_thread() are called. -pub fn next_boot_patch() -> Option { - return with_config(|config| { +pub fn next_boot_patch() -> anyhow::Result> { + with_config(|config| { let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); - return state.next_boot_patch(); - }); + return Ok(state.next_boot_patch()); + }) } /// The patch which is currently booted. This is None until /// report_launch_start() is called at which point it is copied from /// next_boot_patch. -pub fn current_boot_patch() -> Option { - return with_config(|config| { +pub fn current_boot_patch() -> anyhow::Result> { + with_config(|config| { let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); - return state.current_boot_patch(); - }); + return Ok(state.current_boot_patch()); + }) } pub fn report_launch_start() -> anyhow::Result<()> { @@ -426,46 +416,46 @@ pub fn report_launch_start() -> anyhow::Result<()> { /// Report that the current active path failed to launch. /// This will mark the patch as bad and activate the next best patch. -pub fn report_launch_failure() -> Result<(), UpdateError> { +pub fn report_launch_failure() -> anyhow::Result<()> { info!("Reporting failed launch."); with_config(|config| { let mut state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); - let patch = state - .current_boot_patch() - .ok_or(UpdateError::InvalidState("No current patch".to_string()))?; + let patch = + state + .current_boot_patch() + .ok_or(anyhow::Error::from(UpdateError::InvalidState( + "No current patch".to_string(), + )))?; state.mark_patch_as_bad(&patch); - state.activate_latest_bootable_patch() + state + .activate_latest_bootable_patch() + .map_err(|err| anyhow::Error::from(err)) }) } -pub fn report_launch_success() -> Result<(), UpdateError> { +pub fn report_launch_success() -> anyhow::Result<()> { with_config(|config| { let mut state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); - let patch = state - .current_boot_patch() - .ok_or(UpdateError::InvalidState("No current patch".to_string()))?; + let patch = + state + .current_boot_patch() + .ok_or(anyhow::Error::from(UpdateError::InvalidState( + "No current patch".to_string(), + )))?; state.mark_patch_as_good(&patch); - state.save().map_err(|_| UpdateError::FailedToSaveState) + state + .save() + .map_err(|_| anyhow::Error::from(UpdateError::FailedToSaveState)) }) } /// Synchronously checks for an update and downloads and installs it if available. -pub fn update() -> UpdateStatus { - return with_config(|config| { - let result = update_internal(&config); - match result { - Err(err) => { - error!("Problem updating: {err}"); - error!("{}", err.backtrace()); - return UpdateStatus::UpdateHadError; - } - Ok(status) => status, - } - }); +pub fn update() -> anyhow::Result { + with_config(|config| update_internal(&config)) } /// This does not return status. The only output is the change to the saved @@ -476,7 +466,7 @@ pub fn start_update_thread() { // call which is wrong. We should be able to release the lock during the // network requests. std::thread::spawn(move || { - let status = update(); + let status = update().unwrap_or(UpdateStatus::UpdateHadError); info!("Update thread finished with status: {}", status); }); } @@ -526,17 +516,19 @@ mod tests { }) .expect("move failed"); state.save().expect("save failed"); - }); - assert!(crate::next_boot_patch().is_some()); + Ok(()) + }) + .unwrap(); + assert!(crate::next_boot_patch().unwrap().is_some()); // pretend we booted from it crate::report_launch_start().unwrap(); crate::report_launch_success().unwrap(); - assert!(crate::next_boot_patch().is_some()); + assert!(crate::next_boot_patch().unwrap().is_some()); // mark it bad. crate::report_launch_failure().unwrap(); // Technically might need to "reload" // ask for current patch (should get none). - assert!(crate::next_boot_patch().is_none()); + assert!(crate::next_boot_patch().unwrap().is_none()); } #[test] @@ -597,16 +589,18 @@ mod tests { let tmp_dir = TempDir::new("example").unwrap(); init_for_testing(&tmp_dir); assert_eq!( - crate::report_launch_failure(), - Err(crate::UpdateError::InvalidState( - "No current patch".to_string() - )) + crate::report_launch_failure() + .unwrap_err() + .downcast::() + .unwrap(), + crate::UpdateError::InvalidState("No current patch".to_string()) ); assert_eq!( - crate::report_launch_success(), - Err(crate::UpdateError::InvalidState( - "No current patch".to_string() - )) + crate::report_launch_success() + .unwrap_err() + .downcast::() + .unwrap(), + crate::UpdateError::InvalidState("No current patch".to_string()) ); } }