From 5a89a14d5f90adacc1377c389153f28036bdf568 Mon Sep 17 00:00:00 2001 From: ashish-kumar-dash Date: Tue, 15 Sep 2026 21:50:03 +0530 Subject: [PATCH 1/3] refactor(smitebot): deduplicate campaign load into CampaignState::load_campaign stop, status, and reproduce each repeated the same 18-line block: resolve runs_dir, build the state path, load state, and log a not-found hint on failure. Extract it into CampaignState::load_campaign(id) -> Option<(Self, PathBuf)>, collapsing each call site to one line. --- smitebot/src/commands/corpus.rs | 40 ++++++++++-------------------- smitebot/src/commands/reproduce.rs | 15 ++--------- smitebot/src/commands/status.rs | 15 ++--------- smitebot/src/commands/stop.rs | 15 ++--------- smitebot/src/state.rs | 18 ++++++++++++++ 5 files changed, 37 insertions(+), 66 deletions(-) diff --git a/smitebot/src/commands/corpus.rs b/smitebot/src/commands/corpus.rs index 93fdcdd7..1da0fb6d 100644 --- a/smitebot/src/commands/corpus.rs +++ b/smitebot/src/commands/corpus.rs @@ -70,38 +70,20 @@ impl CorpusCommand { } } -/// Loads the campaign state for `campaign_id`, logging a not-found hint on error. -fn load_campaign(runs_dir: &Path, campaign_id: &str) -> Option { - let state_path = runs_dir.join(campaign_id).join("state.json"); - match CampaignState::load(&state_path) { - Ok(state) => Some(state), - Err(e) => { - log::error!("{e}"); - log::error!( - "campaign '{campaign_id}' not found; list campaigns with: ls {}", - runs_dir.display() - ); - None - } - } -} - /// Loads every campaign's state, then merges their runner queues into `output`. /// /// All states are loaded before any file is written, so a bad campaign ID fails /// before the output directory is touched rather than leaving a partial merge. fn execute_merge(args: &MergeArgs) -> bool { - let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; - }; - let mut states = Vec::with_capacity(args.campaign_ids.len()); for campaign_id in &args.campaign_ids { - let Some(state) = load_campaign(&runs_dir, campaign_id) else { - return false; - }; - states.push(state); + match CampaignState::load_campaign(campaign_id) { + Ok((state, _)) => states.push(state), + Err(e) => { + log::error!("{e}"); + return false; + } + } } if output_dir_occupied(&args.output) { @@ -222,8 +204,12 @@ fn execute_minimize(args: &MinimizeArgs) -> bool { return false; }; - let Some(state) = load_campaign(&runs_dir, &args.campaign_id) else { - return false; + let state = match CampaignState::load_campaign(&args.campaign_id) { + Ok((state, _)) => state, + Err(e) => { + log::error!("{e}"); + return false; + } }; // The --aflpp-path flag overrides the path recorded at campaign start, for when diff --git a/smitebot/src/commands/reproduce.rs b/smitebot/src/commands/reproduce.rs index 15adb78e..cc0a1c50 100644 --- a/smitebot/src/commands/reproduce.rs +++ b/smitebot/src/commands/reproduce.rs @@ -36,21 +36,10 @@ impl ReproduceCommand { /// `false` only on an operational failure: unknown campaign, missing input, /// missing image, or a Docker spawn error. pub fn execute(args: &ReproduceArgs) -> bool { - let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; - }; - - let state_path = runs_dir.join(&args.campaign_id).join("state.json"); - let state = match CampaignState::load(&state_path) { - Ok(s) => s, + let (state, _) = match CampaignState::load_campaign(&args.campaign_id) { + Ok(x) => x, Err(e) => { log::error!("{e}"); - log::error!( - "campaign '{}' not found; list campaigns with: ls {}", - args.campaign_id, - runs_dir.display() - ); return false; } }; diff --git a/smitebot/src/commands/status.rs b/smitebot/src/commands/status.rs index 000630c3..1c19ba41 100644 --- a/smitebot/src/commands/status.rs +++ b/smitebot/src/commands/status.rs @@ -44,21 +44,10 @@ impl StatusCommand { /// Reports the status of a campaign, either as a one-shot summary or by /// attaching to its live tmux dashboard. pub fn execute(args: &StatusArgs) -> bool { - let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; - }; - let state_path = runs_dir.join(&args.campaign_id).join("state.json"); - - let state = match CampaignState::load(&state_path) { - Ok(state) => state, + let (state, _) = match CampaignState::load_campaign(&args.campaign_id) { + Ok(x) => x, Err(e) => { log::error!("{e}"); - log::error!( - "campaign '{}' not found; list campaigns with: ls {}", - args.campaign_id, - runs_dir.display() - ); return false; } }; diff --git a/smitebot/src/commands/stop.rs b/smitebot/src/commands/stop.rs index 0039c788..b4306e73 100644 --- a/smitebot/src/commands/stop.rs +++ b/smitebot/src/commands/stop.rs @@ -41,21 +41,10 @@ impl StopCommand { /// Stops a campaign: reaps its runner process groups, tears down the tmux /// session, and records the stop in state.json. pub fn execute(args: &StopArgs) -> bool { - let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; - }; - let state_path = runs_dir.join(&args.campaign_id).join("state.json"); - - let mut state = match CampaignState::load(&state_path) { - Ok(state) => state, + let (mut state, state_path) = match CampaignState::load_campaign(&args.campaign_id) { + Ok(x) => x, Err(e) => { log::error!("{e}"); - log::error!( - "campaign '{}' not found; list campaigns with: ls {}", - args.campaign_id, - runs_dir.display() - ); return false; } }; diff --git a/smitebot/src/state.rs b/smitebot/src/state.rs index 85b36cf1..f3adda6f 100644 --- a/smitebot/src/state.rs +++ b/smitebot/src/state.rs @@ -102,6 +102,8 @@ pub enum StateError { path: PathBuf, source: serde_json::Error, }, + #[error("unable to determine home directory")] + HomeDir, } impl CampaignState { @@ -174,6 +176,17 @@ impl CampaignState { source, }) } + + /// Loads state for campaign `id` from `~/.smitebot/runs//state.json`. + /// + /// Returns the state and its path (the path is needed by callers that + /// mutate and re-save state, e.g. `stop`). + pub fn load_campaign(id: &str) -> Result<(Self, PathBuf), StateError> { + let runs_dir = Self::runs_dir().ok_or(StateError::HomeDir)?; + let state_path = runs_dir.join(id).join("state.json"); + let state = Self::load(&state_path)?; + Ok((state, state_path)) + } } #[cfg(test)] @@ -325,6 +338,11 @@ sharedir = "/tmp/nyx" assert_eq!(loaded.stop_time, Some(1_749_469_200)); } + #[test] + fn load_campaign_returns_err_for_missing_campaign() { + assert!(CampaignState::load_campaign("nonexistent-campaign-xkcd-abc123").is_err()); + } + #[test] fn load_reports_missing_file() { let err = CampaignState::load(Path::new("/no/such/state.json")).unwrap_err(); From 693081bacb62b39f32451af3031db658d4b44032 Mon Sep 17 00:00:00 2001 From: ashish-kumar-dash Date: Wed, 23 Sep 2026 15:11:35 +0530 Subject: [PATCH 2/3] changes --- smitebot/src/commands/corpus.rs | 4 ++-- smitebot/src/commands/reproduce.rs | 4 ++-- smitebot/src/commands/start.rs | 32 ++++++++++-------------------- smitebot/src/commands/status.rs | 4 ++-- smitebot/src/commands/stop.rs | 6 +++--- smitebot/src/state.rs | 19 +++++++++--------- 6 files changed, 29 insertions(+), 40 deletions(-) diff --git a/smitebot/src/commands/corpus.rs b/smitebot/src/commands/corpus.rs index 1da0fb6d..bab3a0b5 100644 --- a/smitebot/src/commands/corpus.rs +++ b/smitebot/src/commands/corpus.rs @@ -78,7 +78,7 @@ fn execute_merge(args: &MergeArgs) -> bool { let mut states = Vec::with_capacity(args.campaign_ids.len()); for campaign_id in &args.campaign_ids { match CampaignState::load_campaign(campaign_id) { - Ok((state, _)) => states.push(state), + Ok(state) => states.push(state), Err(e) => { log::error!("{e}"); return false; @@ -205,7 +205,7 @@ fn execute_minimize(args: &MinimizeArgs) -> bool { }; let state = match CampaignState::load_campaign(&args.campaign_id) { - Ok((state, _)) => state, + Ok(s) => s, Err(e) => { log::error!("{e}"); return false; diff --git a/smitebot/src/commands/reproduce.rs b/smitebot/src/commands/reproduce.rs index cc0a1c50..8fa8745b 100644 --- a/smitebot/src/commands/reproduce.rs +++ b/smitebot/src/commands/reproduce.rs @@ -36,8 +36,8 @@ impl ReproduceCommand { /// `false` only on an operational failure: unknown campaign, missing input, /// missing image, or a Docker spawn error. pub fn execute(args: &ReproduceArgs) -> bool { - let (state, _) = match CampaignState::load_campaign(&args.campaign_id) { - Ok(x) => x, + let state = match CampaignState::load_campaign(&args.campaign_id) { + Ok(s) => s, Err(e) => { log::error!("{e}"); return false; diff --git a/smitebot/src/commands/start.rs b/smitebot/src/commands/start.rs index d6e21731..3216ab31 100644 --- a/smitebot/src/commands/start.rs +++ b/smitebot/src/commands/start.rs @@ -118,13 +118,6 @@ impl StartCommand { } }; - let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; - }; - - let state_path = runs_dir.join(&campaign_id).join("state.json"); - let Some(git_hash) = smite_git_hash(&config.smite_dir) else { log::error!("could not determine smite git hash"); return false; @@ -143,21 +136,21 @@ impl StartCommand { tmux_session, ); - if let Err(e) = state.save(&state_path) { + if let Err(e) = state.save_campaign() { log::error!("{e}"); return false; } - if !launch_runners(&config, &seed_dir, &mut state, &state_path) { + if !launch_runners(&config, &seed_dir, &mut state) { return false; } - if let Err(e) = state.save(&state_path) { + if let Err(e) = state.save_campaign() { log::error!("{e}"); return false; } log::info!("campaign {} is running", state.id); - log::info!("state saved to {}", state_path.display()); + log::info!("state saved to ~/.smitebot/runs/{}/state.json", state.id); log::info!("attaching to tmux session '{}'", state.tmux_session); if let Err(e) = tmux::attach(&state.tmux_session) { @@ -170,12 +163,7 @@ impl StartCommand { /// Spawns all runners inside a tmux session, verifies they produce /// `fuzzer_stats`, and updates campaign state with PIDs. -fn launch_runners( - config: &CampaignConfig, - seed_dir: &Path, - state: &mut CampaignState, - state_path: &Path, -) -> bool { +fn launch_runners(config: &CampaignConfig, seed_dir: &Path, state: &mut CampaignState) -> bool { let session = &state.tmux_session; log::info!( "starting {} runners in tmux session '{session}'", @@ -198,7 +186,7 @@ fn launch_runners( if let Err(e) = result { log::error!("failed to create tmux window for runner {id}: {e}"); state.runners = runners; - fail_campaign(state, state_path); + fail_campaign(state); return false; } @@ -207,7 +195,7 @@ fn launch_runners( state.runners = runners; - if let Err(e) = state.save(state_path) { + if let Err(e) = state.save_campaign() { log::warn!("failed to save state: {e}"); } @@ -221,7 +209,7 @@ fn launch_runners( // verify_startup has already logged the specific reason per runner // (window died, or ceiling reached). log::error!("one or more runners failed to start"); - fail_campaign(state, state_path); + fail_campaign(state); return false; } @@ -231,7 +219,7 @@ fn launch_runners( /// Marks the campaign as failed, logs instructions to inspect the tmux session, /// and persists the updated state. -fn fail_campaign(state: &mut CampaignState, state_path: &Path) { +fn fail_campaign(state: &mut CampaignState) { if !state.runners.is_empty() { log::info!( "inspect tmux session '{}' for error output, \ @@ -241,7 +229,7 @@ fn fail_campaign(state: &mut CampaignState, state_path: &Path) { ); } state.status = Status::Failed; - if let Err(e) = state.save(state_path) { + if let Err(e) = state.save_campaign() { log::warn!("failed to save state: {e}"); } } diff --git a/smitebot/src/commands/status.rs b/smitebot/src/commands/status.rs index 1c19ba41..a46b7050 100644 --- a/smitebot/src/commands/status.rs +++ b/smitebot/src/commands/status.rs @@ -44,8 +44,8 @@ impl StatusCommand { /// Reports the status of a campaign, either as a one-shot summary or by /// attaching to its live tmux dashboard. pub fn execute(args: &StatusArgs) -> bool { - let (state, _) = match CampaignState::load_campaign(&args.campaign_id) { - Ok(x) => x, + let state = match CampaignState::load_campaign(&args.campaign_id) { + Ok(s) => s, Err(e) => { log::error!("{e}"); return false; diff --git a/smitebot/src/commands/stop.rs b/smitebot/src/commands/stop.rs index b4306e73..ef6a72b3 100644 --- a/smitebot/src/commands/stop.rs +++ b/smitebot/src/commands/stop.rs @@ -41,8 +41,8 @@ impl StopCommand { /// Stops a campaign: reaps its runner process groups, tears down the tmux /// session, and records the stop in state.json. pub fn execute(args: &StopArgs) -> bool { - let (mut state, state_path) = match CampaignState::load_campaign(&args.campaign_id) { - Ok(x) => x, + let mut state = match CampaignState::load_campaign(&args.campaign_id) { + Ok(s) => s, Err(e) => { log::error!("{e}"); return false; @@ -63,7 +63,7 @@ impl StopCommand { state.status = Status::Stopped; state.stop_time = Some(utils::epoch_secs()); - if let Err(e) = state.save(&state_path) { + if let Err(e) = state.save_campaign() { log::error!( "runners were reaped but recording the stop failed: {e}; \ campaign {} will still show as running in state.json", diff --git a/smitebot/src/state.rs b/smitebot/src/state.rs index f3adda6f..599f9515 100644 --- a/smitebot/src/state.rs +++ b/smitebot/src/state.rs @@ -143,7 +143,7 @@ impl CampaignState { /// Saves the campaign state as JSON, using an atomic write to prevent /// corruption if the process is interrupted. - pub fn save(&self, path: &Path) -> Result<(), StateError> { + fn save(&self, path: &Path) -> Result<(), StateError> { if let Some(parent) = path.parent() { fs::create_dir_all(parent).map_err(|source| StateError::CreateDir { path: parent.to_path_buf(), @@ -165,8 +165,14 @@ impl CampaignState { Ok(()) } + /// Saves the campaign state to `~/.smitebot/runs//state.json`. + pub fn save_campaign(&self) -> Result<(), StateError> { + let runs_dir = Self::runs_dir().ok_or(StateError::HomeDir)?; + self.save(&runs_dir.join(&self.id).join("state.json")) + } + /// Loads campaign state from a JSON file written by `save`. - pub fn load(path: &Path) -> Result { + fn load(path: &Path) -> Result { let contents = fs::read_to_string(path).map_err(|source| StateError::Read { path: path.to_path_buf(), source, @@ -178,14 +184,9 @@ impl CampaignState { } /// Loads state for campaign `id` from `~/.smitebot/runs//state.json`. - /// - /// Returns the state and its path (the path is needed by callers that - /// mutate and re-save state, e.g. `stop`). - pub fn load_campaign(id: &str) -> Result<(Self, PathBuf), StateError> { + pub fn load_campaign(id: &str) -> Result { let runs_dir = Self::runs_dir().ok_or(StateError::HomeDir)?; - let state_path = runs_dir.join(id).join("state.json"); - let state = Self::load(&state_path)?; - Ok((state, state_path)) + Self::load(&runs_dir.join(id).join("state.json")) } } From 2edc79b3bc74853ae9d29eebf56559b722150667 Mon Sep 17 00:00:00 2001 From: ashish-kumar-dash Date: Mon, 5 Oct 2026 20:17:28 +0530 Subject: [PATCH 3/3] refactor(smitebot): return Result from command handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Command handlers logged the error and returned false on failure, which is non-idiomatic and verbose. Return the error instead and log it once in main, so the command bodies can use `?` and logging lives in one place (issue #270). Add CliError in src/error.rs: Config/State wrap the existing module errors via #[from], Msg carries a ready-to-log operational message, and Reported marks the config/doctor report commands whose printed output (including JSON) is the artifact — main sets the exit code but logs nothing further so the output is not duplicated. From lets the bench-exec validators flow through `?`. Functions whose bool/Option is a genuine domain verdict rather than an error channel stay as they are: verify_startup (per-runner diagnostics), stop's reap helpers (clean vs survivors), and the pure predicates. Net -132 lines. --- smitebot/src/commands/bench_exec.rs | 112 ++++---------- smitebot/src/commands/build.rs | 40 ++--- smitebot/src/commands/config.rs | 14 +- smitebot/src/commands/corpus.rs | 228 ++++++++++++---------------- smitebot/src/commands/doctor.rs | 21 +-- smitebot/src/commands/print_ir.rs | 43 +++--- smitebot/src/commands/reproduce.rs | 61 ++++---- smitebot/src/commands/start.rs | 115 ++++++-------- smitebot/src/commands/status.rs | 70 ++++----- smitebot/src/commands/stop.rs | 28 ++-- smitebot/src/error.rs | 34 +++++ smitebot/src/main.rs | 16 +- smitebot/src/utils.rs | 24 ++- 13 files changed, 354 insertions(+), 452 deletions(-) create mode 100644 smitebot/src/error.rs diff --git a/smitebot/src/commands/bench_exec.rs b/smitebot/src/commands/bench_exec.rs index 8704c7d3..0fbc8e83 100644 --- a/smitebot/src/commands/bench_exec.rs +++ b/smitebot/src/commands/bench_exec.rs @@ -20,6 +20,7 @@ use clap::Args; use crate::commands::build::{BuildInputs, run_build}; use crate::config::CampaignConfig; +use crate::error::CliError; use crate::latency_stats::{LatencyStats, avg_duration, mean_stddev}; use crate::libnyx::{Libnyx, NyxReturn, PAYLOAD_HEADER_SIZE}; use crate::utils::{pin_to_cpu, setup_nyx}; @@ -105,75 +106,34 @@ pub struct BenchExecArgs { } impl BenchExecCommand { - /// Runs the benchmark and returns whether it completed successfully. - pub fn execute(args: &BenchExecArgs) -> bool { - let iterations = match validate_iterations(args.iterations) { - Ok(n) => n, - Err(e) => { - log::error!("{e}"); - return false; - } - }; - let repeat = match validate_repeat(args.repeat) { - Ok(n) => n, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + /// Runs the benchmark. + pub fn execute(args: &BenchExecArgs) -> Result<(), CliError> { + let iterations = validate_iterations(args.iterations)?; + let repeat = validate_repeat(args.repeat)?; // A zero timeout causes Nyx to silently turn off the timing the // benchmark exists to measure. if args.timeout_secs == 0 { - log::error!("Nyx requires --timeout to be at least 1 second"); - return false; + return Err(CliError::Msg( + "Nyx requires --timeout to be at least 1 second".to_string(), + )); } - let config = match CampaignConfig::load(&args.path) { - Ok(c) => c, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + let config = CampaignConfig::load(&args.path)?; - let input = match load_input(args) { - Ok(input) => input, - Err(e) => { - log::error!("{e}"); - return false; - } - }; - let max_input_size = match validate_max_input_size(args.max_input_size, input.len()) { - Ok(size) => size, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + let input = load_input(args)?; + let max_input_size = validate_max_input_size(args.max_input_size, input.len())?; - let Some(libnyx_path) = locate_libnyx(&config) else { - return false; - }; + let libnyx_path = locate_libnyx(&config)?; - if !ensure_sharedir(&config, args.no_build) { - return false; - } + ensure_sharedir(&config, args.no_build)?; - let libnyx = match Libnyx::load(&libnyx_path) { - Ok(lib) => lib, - Err(e) => { - log::error!("failed to load {}: {e}", libnyx_path.display()); - return false; - } - }; + let libnyx = Libnyx::load(&libnyx_path) + .map_err(|e| CliError::Msg(format!("failed to load {}: {e}", libnyx_path.display())))?; // Pin before booting: libnyx spawns QEMU as a child, so the VM inherits // this affinity mask. Doing it here also pins the timing loop itself. let cpu = args.cpu.unwrap_or(args.worker_id as usize); - if let Err(e) = pin_to_cpu(cpu) { - log::error!("{e}"); - return false; - } + pin_to_cpu(cpu)?; log::info!("pinned to CPU {cpu}"); // Each repeat is a fresh VM boot so the aggregate captures boot- and @@ -183,7 +143,7 @@ impl BenchExecCommand { if repeat > 1 { log::info!("benchmark run {}/{repeat}", run_index + 1); } - match run_once( + runs.push(run_once( &libnyx, &config, &input, @@ -191,13 +151,7 @@ impl BenchExecCommand { max_input_size, iterations, run_index, - ) { - Ok(result) => runs.push(result), - Err(e) => { - log::error!("{e}"); - return false; - } - } + )?); } let report = BenchReport { @@ -210,7 +164,7 @@ impl BenchExecCommand { }; report.print(); - true + Ok(()) } } @@ -481,22 +435,23 @@ fn load_input(args: &BenchExecArgs) -> Result, String> { /// /// AFL++'s Nyx build is required both to prepare the sharedir (setup-nyx.sh) /// and to drive the VM at runtime, so it is checked before either. -fn locate_libnyx(config: &CampaignConfig) -> Option { +fn locate_libnyx(config: &CampaignConfig) -> Result { if !config.aflpp_path.exists() { - log::error!("aflpp_path does not exist: {}", config.aflpp_path.display()); - return None; + return Err(CliError::Msg(format!( + "aflpp_path does not exist: {}", + config.aflpp_path.display() + ))); } let libnyx_path = config.aflpp_path.join("libnyx.so"); if !libnyx_path.exists() { - log::error!( + return Err(CliError::Msg(format!( "{} not found; build AFL++ with Nyx support (see nyx_mode/README.md)", libnyx_path.display() - ); - return None; + ))); } - Some(libnyx_path) + Ok(libnyx_path) } /// Builds the image and sets up the sharedir, or (when `no_build`) verifies an @@ -504,25 +459,22 @@ fn locate_libnyx(config: &CampaignConfig) -> Option { /// /// libnyx writes `config.ron` into the sharedir during setup-nyx.sh; its /// absence means the sharedir was never prepared. -fn ensure_sharedir(config: &CampaignConfig, no_build: bool) -> bool { +fn ensure_sharedir(config: &CampaignConfig, no_build: bool) -> Result<(), CliError> { if no_build { let config_ron = config.sharedir.join("config.ron"); if !config_ron.exists() { - log::error!( + return Err(CliError::Msg(format!( "Nyx sharedir is not set up ({} missing); drop --no-build, or run \ `smitebot start` / scripts/setup-nyx.sh first", config_ron.display() - ); - return false; + ))); } - return true; + return Ok(()); } let image = config.image_tag(); log::info!("building Docker image {image}"); - if !run_build(&BuildInputs::from_config(config, &image)) { - return false; - } + run_build(&BuildInputs::from_config(config, &image))?; log::info!("setting up Nyx sharedir at {}", config.sharedir.display()); setup_nyx(config, &image) diff --git a/smitebot/src/commands/build.rs b/smitebot/src/commands/build.rs index 4e580c7b..47366440 100644 --- a/smitebot/src/commands/build.rs +++ b/smitebot/src/commands/build.rs @@ -7,6 +7,7 @@ use std::process::{Command, ExitStatus}; use clap::Args; use crate::config::{self, CampaignConfig, Target}; +use crate::error::CliError; /// Command handler for `smitebot build`. pub struct BuildCommand; @@ -118,24 +119,17 @@ impl BuildInputs { } impl BuildCommand { - /// Builds the requested Smite Docker image and returns whether Docker succeeded. - pub fn execute(args: &BuildArgs) -> bool { + /// Builds the requested Smite Docker image. + pub fn execute(args: &BuildArgs) -> Result<(), CliError> { let config = match &args.config { - Some(path) => match CampaignConfig::load(path) { - Ok(c) => Some(c), - Err(e) => { - log::error!("{e}"); - return false; - } - }, + Some(path) => Some(CampaignConfig::load(path)?), None => None, }; let inputs = BuildInputs::resolve(config.as_ref(), args); if let Some(err) = config::check_scenario_exists(&inputs.smite_dir, inputs.target, &inputs.scenario) { - log::error!("{err}"); - return false; + return Err(CliError::Msg(err)); } log::info!( "building {} with {}", @@ -146,30 +140,26 @@ impl BuildCommand { } } -/// Checks that the Dockerfile exists, runs `docker build`, and reports success. +/// Checks that the Dockerfile exists and runs `docker build`. /// /// Shared by `smitebot build` and `smitebot start`. -pub fn run_build(inputs: &BuildInputs) -> bool { +pub fn run_build(inputs: &BuildInputs) -> Result<(), CliError> { if !inputs.dockerfile.exists() { - log::error!("Dockerfile not found: {}", inputs.dockerfile.display()); - return false; + return Err(CliError::Msg(format!( + "Dockerfile not found: {}", + inputs.dockerfile.display() + ))); } - let status = match run_docker_build(inputs) { - Ok(status) => status, - Err(e) => { - log::error!("failed to run docker build: {e}"); - return false; - } - }; + let status = run_docker_build(inputs) + .map_err(|e| CliError::Msg(format!("failed to run docker build: {e}")))?; if !status.success() { - log::error!("docker build failed with {status}"); - return false; + return Err(CliError::Msg(format!("docker build failed with {status}"))); } log::info!("built {}", inputs.image); - true + Ok(()) } /// Returns the default image tag used by Smite's manual Docker build flow. diff --git a/smitebot/src/commands/config.rs b/smitebot/src/commands/config.rs index 14c4c46f..657e2ea2 100644 --- a/smitebot/src/commands/config.rs +++ b/smitebot/src/commands/config.rs @@ -7,6 +7,7 @@ use clap::Args; use serde::Serialize; use crate::config::CampaignConfig; +use crate::error::CliError; /// Command handler for `smitebot config`. pub struct ConfigCommand; @@ -48,7 +49,10 @@ struct ConfigErrorReport { impl ConfigCommand { /// Validates a campaign configuration file and reports the result. - pub fn execute(args: &ConfigArgs) -> bool { + /// + /// The report (plain or JSON) is the artifact and is always printed here, so + /// a failure returns [`CliError::Reported`]: `main` only sets the exit code. + pub fn execute(args: &ConfigArgs) -> Result<(), CliError> { match CampaignConfig::load(&args.path) { Ok(config) => { let errors = config.check_paths(); @@ -96,7 +100,11 @@ impl ConfigCommand { eprintln!("error: {error}"); } } - valid + if valid { + Ok(()) + } else { + Err(CliError::Reported) + } } Err(e) => { if args.json { @@ -110,7 +118,7 @@ impl ConfigCommand { } else { eprintln!("error: {e}"); } - false + Err(CliError::Reported) } } } diff --git a/smitebot/src/commands/corpus.rs b/smitebot/src/commands/corpus.rs index bab3a0b5..d600b650 100644 --- a/smitebot/src/commands/corpus.rs +++ b/smitebot/src/commands/corpus.rs @@ -9,6 +9,7 @@ use std::process::Command; use clap::{Args, Subcommand}; +use crate::error::CliError; use crate::state::CampaignState; use crate::utils::{find_in_path, is_executable}; @@ -62,7 +63,7 @@ pub struct MinimizeArgs { impl CorpusCommand { /// Dispatches to the requested corpus subcommand. - pub fn execute(args: &CorpusArgs) -> bool { + pub fn execute(args: &CorpusArgs) -> Result<(), CliError> { match &args.command { CorpusSubcommand::Merge(a) => execute_merge(a), CorpusSubcommand::Minimize(a) => execute_minimize(a), @@ -74,48 +75,40 @@ impl CorpusCommand { /// /// All states are loaded before any file is written, so a bad campaign ID fails /// before the output directory is touched rather than leaving a partial merge. -fn execute_merge(args: &MergeArgs) -> bool { +fn execute_merge(args: &MergeArgs) -> Result<(), CliError> { let mut states = Vec::with_capacity(args.campaign_ids.len()); for campaign_id in &args.campaign_ids { - match CampaignState::load_campaign(campaign_id) { - Ok(state) => states.push(state), - Err(e) => { - log::error!("{e}"); - return false; - } - } + states.push(CampaignState::load_campaign(campaign_id)?); } - if output_dir_occupied(&args.output) { - return false; + if let Some(conflict) = output_dir_conflict(&args.output) { + return Err(CliError::Msg(conflict)); } - if let Err(e) = fs::create_dir_all(&args.output) { - log::error!( + fs::create_dir_all(&args.output).map_err(|e| { + CliError::Msg(format!( "failed to create output directory {}: {e}", args.output.display() - ); - return false; - } + )) + })?; - let Some((total_in, total_out)) = merge_states(&states, &args.output) else { - log::error!( - "merge failed partway; partial results may remain in {} — remove it before retrying", + let (total_in, total_out) = merge_states(&states, &args.output).map_err(|e| { + CliError::Msg(format!( + "{e}; partial results may remain in {} — remove it before retrying", args.output.display() - ); - return false; - }; + )) + })?; log::info!( "merged {total_in} files across {} campaign(s) → {total_out} unique files written to {}", args.campaign_ids.len(), args.output.display() ); - true + Ok(()) } /// Merges every runner queue of every state into `output`. See [`merge_dirs`]. -fn merge_states(states: &[CampaignState], output: &Path) -> Option<(usize, usize)> { +fn merge_states(states: &[CampaignState], output: &Path) -> Result<(usize, usize), CliError> { let mut dirs = Vec::new(); for state in states { for runner in &state.runners { @@ -133,9 +126,9 @@ fn merge_states(states: &[CampaignState], output: &Path) -> Option<(usize, usize /// themselves, so a large corpus only costs 8 bytes per unique entry in memory. /// A 64-bit hash collision (silently dropping a distinct input) has probability /// ~n²/2⁶⁵, negligible for realistic corpus sizes. Returns `(files_read, -/// files_written)`, or `None` if any read/write failed (logged at the failure -/// site). -fn merge_dirs(sources: &[PathBuf], output: &Path) -> Option<(usize, usize)> { +/// files_written)`, or an error naming the first file that could not be read or +/// written. +fn merge_dirs(sources: &[PathBuf], output: &Path) -> Result<(usize, usize), CliError> { let mut seen: HashSet = HashSet::new(); let mut total_in = 0usize; let mut total_out = 0usize; @@ -146,47 +139,34 @@ fn merge_dirs(sources: &[PathBuf], output: &Path) -> Option<(usize, usize)> { continue; } - let entries = match fs::read_dir(dir) { - Ok(e) => e, - Err(e) => { - log::error!("failed to read {}: {e}", dir.display()); - return None; - } - }; + let entries = fs::read_dir(dir) + .map_err(|e| CliError::Msg(format!("failed to read {}: {e}", dir.display())))?; for entry in entries { - let path = match entry { - Ok(e) => e.path(), - Err(e) => { - log::error!("failed to read an entry in {}: {e}", dir.display()); - return None; - } - }; + let path = entry + .map_err(|e| { + CliError::Msg(format!("failed to read an entry in {}: {e}", dir.display())) + })? + .path(); if !path.is_file() { continue; } - let contents = match fs::read(&path) { - Ok(c) => c, - Err(e) => { - log::error!("failed to read {}: {e}", path.display()); - return None; - } - }; + let contents = fs::read(&path) + .map_err(|e| CliError::Msg(format!("failed to read {}: {e}", path.display())))?; total_in += 1; if seen.insert(content_hash(&contents)) { let dest = output.join(format!("{total_out:06}")); // `contents` is already in hand from the dedup read, so write it // out directly rather than re-reading via fs::copy. - if let Err(e) = fs::write(&dest, &contents) { - log::error!("failed to write {}: {e}", dest.display()); - return None; - } + fs::write(&dest, &contents).map_err(|e| { + CliError::Msg(format!("failed to write {}: {e}", dest.display())) + })?; total_out += 1; } } } - Some((total_in, total_out)) + Ok((total_in, total_out)) } /// Returns a 64-bit hash of `bytes` for content-based deduplication. @@ -198,26 +178,22 @@ fn content_hash(bytes: &[u8]) -> u64 { /// Runs `afl-cmin -X` against the campaign's corpus to remove inputs that /// don't contribute new coverage. -fn execute_minimize(args: &MinimizeArgs) -> bool { +fn execute_minimize(args: &MinimizeArgs) -> Result<(), CliError> { let Some(runs_dir) = CampaignState::runs_dir() else { - log::error!("unable to determine home directory"); - return false; + return Err(CliError::Msg( + "unable to determine home directory".to_string(), + )); }; - let state = match CampaignState::load_campaign(&args.campaign_id) { - Ok(s) => s, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + let state = CampaignState::load_campaign(&args.campaign_id)?; // The --aflpp-path flag overrides the path recorded at campaign start, for when // the AFL++ checkout has since moved. let aflpp = args.aflpp_path.as_deref().or(state.aflpp_path.as_deref()); let Some(afl_cmin) = find_afl_cmin(aflpp) else { - log::error!("afl-cmin not found; pass --aflpp-path or add AFL++ to PATH"); - return false; + return Err(CliError::Msg( + "afl-cmin not found; pass --aflpp-path or add AFL++ to PATH".to_string(), + )); }; let output = args @@ -227,8 +203,8 @@ fn execute_minimize(args: &MinimizeArgs) -> bool { // Fail before booting Nyx if the output already holds a corpus. afl-cmin // guards this too, but it first deletes any `id:*` files in the directory. - if output_dir_occupied(&output) { - return false; + if let Some(conflict) = output_dir_conflict(&output) { + return Err(CliError::Msg(conflict)); } // Minimize the given --input directories, or the campaign's runner queues. @@ -244,7 +220,7 @@ fn execute_minimize(args: &MinimizeArgs) -> bool { let stage_dir = runs_dir.join(&args.campaign_id).join(".corpus-stage"); let result = stage_inputs(&sources, &stage_dir) - && run_afl_cmin(&afl_cmin, &stage_dir, &output, &state.sharedir); + .and_then(|()| run_afl_cmin(&afl_cmin, &stage_dir, &output, &state.sharedir)); // Remove the staging corpus, including on a staging failure; the minimized // output in `output` is what the user keeps. Ignore NotFound — staging may @@ -257,15 +233,21 @@ fn execute_minimize(args: &MinimizeArgs) -> bool { stage_dir.display() ); } + result } /// Runs `afl-cmin -X` over `input`, writing the minimized corpus to `output`. -fn run_afl_cmin(afl_cmin: &Path, input: &Path, output: &Path, sharedir: &Path) -> bool { +fn run_afl_cmin( + afl_cmin: &Path, + input: &Path, + output: &Path, + sharedir: &Path, +) -> Result<(), CliError> { log::info!("running afl-cmin on {}", input.display()); log::info!("output: {}", output.display()); - let status = match Command::new(afl_cmin) + let status = Command::new(afl_cmin) .arg("-i") .arg(input) .arg("-o") @@ -273,85 +255,67 @@ fn run_afl_cmin(afl_cmin: &Path, input: &Path, output: &Path, sharedir: &Path) - .arg("-X") .arg(sharedir) .status() - { - Ok(s) => s, - Err(e) => { - log::error!("failed to run {}: {e}", afl_cmin.display()); - return false; - } - }; + .map_err(|e| CliError::Msg(format!("failed to run {}: {e}", afl_cmin.display())))?; if !status.success() { - log::error!("afl-cmin failed with {status}"); - return false; + return Err(CliError::Msg(format!("afl-cmin failed with {status}"))); } log::info!("minimized corpus written to {}", output.display()); - true + Ok(()) } /// Merges `sources` into `stage_dir` under short, sequential filenames for -/// `afl-cmin -i`, returning whether staging succeeded. +/// `afl-cmin -i`, returning an error if staging fails. /// /// AFL++ produces long input names near `NAME_MAX`, and afl-cmin can then exceed /// the max length during minimization: it stages each input into a temp dir as /// `_`, see /// . /// Merging first gives inputs short names so afl-cmin stays under the limit. -fn stage_inputs(sources: &[PathBuf], stage_dir: &Path) -> bool { +fn stage_inputs(sources: &[PathBuf], stage_dir: &Path) -> Result<(), CliError> { // Start from a clean staging dir; a prior interrupted run may have left one. - if stage_dir.exists() - && let Err(e) = fs::remove_dir_all(stage_dir) - { - log::error!( - "failed to clear staging directory {}: {e}", - stage_dir.display() - ); - return false; + if stage_dir.exists() { + fs::remove_dir_all(stage_dir).map_err(|e| { + CliError::Msg(format!( + "failed to clear staging directory {}: {e}", + stage_dir.display() + )) + })?; } - if let Err(e) = fs::create_dir_all(stage_dir) { - log::error!( + fs::create_dir_all(stage_dir).map_err(|e| { + CliError::Msg(format!( "failed to create staging directory {}: {e}", stage_dir.display() - ); - return false; - } + )) + })?; - let Some((total_in, total_out)) = merge_dirs(sources, stage_dir) else { - return false; - }; + let (total_in, total_out) = merge_dirs(sources, stage_dir)?; log::info!("staged {total_out} unique inputs (from {total_in}) for minimization"); - true + Ok(()) } -/// Reports (with an error log) whether `output` already contains files. +/// Returns an actionable message if `output` cannot safely receive a fresh +/// corpus, or `None` if it is empty or absent. /// /// Both subcommands refuse a populated output directory so an existing corpus is -/// never mixed into or overwritten. A missing directory counts as unoccupied (it -/// will be created); any other `read_dir` error is treated as occupied so we -/// never write over an unknown state. -fn output_dir_occupied(output: &Path) -> bool { +/// never mixed into or overwritten. A missing directory is fine (it will be +/// created); any other `read_dir` error leaves occupancy unknown, so it is +/// refused rather than risk writing over something. +fn output_dir_conflict(output: &Path) -> Option { match output.read_dir() { - Ok(mut entries) => { - if entries.next().is_some() { - log::error!( - "output directory {} already contains files; choose a new directory or remove it first", - output.display() - ); - true - } else { - false - } - } + Ok(mut entries) => entries.next().is_some().then(|| { + format!( + "output directory {} already contains files; choose a new directory or remove it first", + output.display() + ) + }), // A missing directory is the common, expected case — it will be created. - Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, - // Any other error (e.g. a permission problem, or a file where a directory - // was expected) leaves occupancy unknown; refuse rather than risk writing - // over something. - Err(e) => { - log::error!("cannot inspect output directory {}: {e}", output.display()); - true - } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) => Some(format!( + "cannot inspect output directory {}: {e}", + output.display() + )), } } @@ -484,7 +448,7 @@ mod tests { fs::write(src.join(&long_name), "payload").unwrap(); let stage = dir.path().join("stage"); - assert!(stage_inputs(&[src], &stage)); + assert!(stage_inputs(&[src], &stage).is_ok()); let names: Vec = fs::read_dir(&stage) .unwrap() @@ -515,26 +479,26 @@ mod tests { fs::create_dir_all(&stage).unwrap(); fs::write(stage.join("stale"), "old").unwrap(); - assert!(stage_inputs(&[src], &stage)); + assert!(stage_inputs(&[src], &stage).is_ok()); assert!(!stage.join("stale").exists()); assert_eq!(dir_contents_sorted(&stage), ["fresh"]); } #[test] - fn output_dir_occupied_detects_existing_files() { + fn output_dir_conflict_detects_existing_files() { let dir = tempdir().unwrap(); - // Absent directory is not occupied. - assert!(!output_dir_occupied(&dir.path().join("missing"))); + // Absent directory does not conflict. + assert!(output_dir_conflict(&dir.path().join("missing")).is_none()); - // Empty directory is not occupied. + // Empty directory does not conflict. let empty = dir.path().join("empty"); fs::create_dir(&empty).unwrap(); - assert!(!output_dir_occupied(&empty)); + assert!(output_dir_conflict(&empty).is_none()); - // A directory holding a corpus file is occupied. + // A directory holding a corpus file conflicts. fs::write(empty.join("000000"), b"x").unwrap(); - assert!(output_dir_occupied(&empty)); + assert!(output_dir_conflict(&empty).is_some()); } #[test] diff --git a/smitebot/src/commands/doctor.rs b/smitebot/src/commands/doctor.rs index edf06bc2..b54f5569 100644 --- a/smitebot/src/commands/doctor.rs +++ b/smitebot/src/commands/doctor.rs @@ -10,6 +10,7 @@ use clap::Args; use serde::Serialize; use crate::config::CampaignConfig; +use crate::error::CliError; use crate::utils::{find_in_path, is_executable}; /// AFL++ binaries required for campaign execution and corpus minimization. @@ -166,15 +167,13 @@ impl Serialize for CheckFailure { impl DoctorCommand { /// Runs all doctor checks and prints either human-readable or JSON output. - pub fn execute(args: &DoctorArgs) -> bool { + /// + /// The checklist is the artifact and is always printed here, so a failing + /// check returns [`CliError::Reported`]: `main` only sets the exit code. A + /// config that fails to load, by contrast, is a logged operational error. + pub fn execute(args: &DoctorArgs) -> Result<(), CliError> { let config = match &args.config { - Some(path) => match CampaignConfig::load(path) { - Ok(c) => Some(c), - Err(e) => { - log::error!("{e}"); - return false; - } - }, + Some(path) => Some(CampaignConfig::load(path)?), None => None, }; let inputs = DoctorInputs::resolve(config.as_ref(), args); @@ -233,7 +232,11 @@ impl DoctorCommand { print_human_report(&report); } - report.overall + if report.overall { + Ok(()) + } else { + Err(CliError::Reported) + } } } diff --git a/smitebot/src/commands/print_ir.rs b/smitebot/src/commands/print_ir.rs index 17c1eeee..539cb46e 100644 --- a/smitebot/src/commands/print_ir.rs +++ b/smitebot/src/commands/print_ir.rs @@ -11,6 +11,8 @@ use std::path::PathBuf; use clap::Args; use smite_ir::Program; +use crate::error::CliError; + /// Command handler for `smitebot print-ir`. pub struct PrintIrCommand; @@ -23,33 +25,24 @@ pub struct PrintIrArgs { impl PrintIrCommand { /// Decodes the input at `args.path` and prints it as readable IR. - pub fn execute(args: &PrintIrArgs) -> bool { - let bytes = match fs::read(&args.path) { - Ok(bytes) => bytes, - Err(e) => { - log::error!("failed to read {}: {e}", args.path.display()); - return false; - } - }; + pub fn execute(args: &PrintIrArgs) -> Result<(), CliError> { + let bytes = fs::read(&args.path) + .map_err(|e| CliError::Msg(format!("failed to read {}: {e}", args.path.display())))?; + + let text = render(&bytes).map_err(|e| { + CliError::Msg(format!( + "failed to decode IR program from {}: {e}", + args.path.display() + )) + })?; - match render(&bytes) { - Ok(text) => { - // `Program`'s Display terminates every instruction with a - // newline, so an empty program renders to an empty string. - if text.is_empty() { - println!("(empty program)"); - } - print!("{text}"); - true - } - Err(e) => { - log::error!( - "failed to decode IR program from {}: {e}", - args.path.display() - ); - false - } + // `Program`'s Display terminates every instruction with a newline, so an + // empty program renders to an empty string. + if text.is_empty() { + println!("(empty program)"); } + print!("{text}"); + Ok(()) } } diff --git a/smitebot/src/commands/reproduce.rs b/smitebot/src/commands/reproduce.rs index 8fa8745b..dc8e4fd2 100644 --- a/smitebot/src/commands/reproduce.rs +++ b/smitebot/src/commands/reproduce.rs @@ -12,6 +12,7 @@ use std::process::Command; use clap::Args; use crate::config::Target; +use crate::error::CliError; use crate::state::CampaignState; use crate::utils::docker_image_id; @@ -31,33 +32,28 @@ pub struct ReproduceArgs { impl ReproduceCommand { /// Replays `input` against the campaign's target in Docker. /// - /// Returns `true` once the container has run to completion regardless of its + /// Returns `Ok(())` once the container has run to completion regardless of its /// exit status (the target's output, not the exit code, is the result), and - /// `false` only on an operational failure: unknown campaign, missing input, + /// an error only on an operational failure: unknown campaign, missing input, /// missing image, or a Docker spawn error. - pub fn execute(args: &ReproduceArgs) -> bool { - let state = match CampaignState::load_campaign(&args.campaign_id) { - Ok(s) => s, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + pub fn execute(args: &ReproduceArgs) -> Result<(), CliError> { + let state = CampaignState::load_campaign(&args.campaign_id)?; // Resolve to an absolute path: Docker `-v` requires one, and this also // rejects a missing input up front. Mount the parent directory and pass // the file's basename via SMITE_INPUT so colons in AFL crash names (e.g. // `id:000000,sig:06,...`) never appear in the bind-mount spec. - let input = match fs::canonicalize(&args.input) { - Ok(p) => p, - Err(e) => { - log::error!("input file not found: {} ({e})", args.input.display()); - return false; - } - }; + let input = fs::canonicalize(&args.input).map_err(|e| { + CliError::Msg(format!( + "input file not found: {} ({e})", + args.input.display() + )) + })?; if !input.is_file() { - log::error!("input is not a regular file: {}", input.display()); - return false; + return Err(CliError::Msg(format!( + "input is not a regular file: {}", + input.display() + ))); } // docker run takes string arguments, so reject a non-UTF-8 path rather than // silently mangling the mount spec. canonicalize guarantees a parent and a @@ -66,18 +62,17 @@ impl ReproduceCommand { input.parent().and_then(Path::to_str), input.file_name().and_then(|name| name.to_str()), ) else { - log::error!("input path is not valid UTF-8: {}", input.display()); - return false; + return Err(CliError::Msg(format!( + "input path is not valid UTF-8: {}", + input.display() + ))); }; let Some(image_id) = docker_image_id(&state.image) else { - log::error!( + return Err(CliError::Msg(format!( "Docker image '{}' not found; build it with: smitebot build --target {} --scenario {}", - state.image, - state.target, - state.scenario - ); - return false; + state.image, state.target, state.scenario + ))); }; // A campaign's image can be rebuilt under the same tag during development, // so a matching name is not a matching image. Warn on a digest mismatch and @@ -98,13 +93,11 @@ impl ReproduceCommand { state.image ); let run_args = docker_run_args(&state.image, state.target, parent, basename); - match Command::new("docker").args(&run_args).status() { - Ok(_) => true, - Err(e) => { - log::error!("failed to run docker: {e}"); - false - } - } + Command::new("docker") + .args(&run_args) + .status() + .map_err(|e| CliError::Msg(format!("failed to run docker: {e}")))?; + Ok(()) } } diff --git a/smitebot/src/commands/start.rs b/smitebot/src/commands/start.rs index 3216ab31..57d9f921 100644 --- a/smitebot/src/commands/start.rs +++ b/smitebot/src/commands/start.rs @@ -14,6 +14,7 @@ use clap::Args; use crate::commands::build::{BuildInputs, run_build}; use crate::config::CampaignConfig; +use crate::error::CliError; use crate::state::{CampaignState, RunnerState, Status}; use crate::tmux; use crate::utils::{command_stdout, docker_image_id, setup_nyx, shell_quote}; @@ -48,26 +49,19 @@ pub struct StartArgs { impl StartCommand { /// Launches a fuzzing campaign from the given configuration. - pub fn execute(args: &StartArgs) -> bool { - let config = match CampaignConfig::load(&args.path) { - Ok(c) => c, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + pub fn execute(args: &StartArgs) -> Result<(), CliError> { + let config = CampaignConfig::load(&args.path)?; let path_errors = config.check_paths(); if !path_errors.is_empty() { - for err in &path_errors { - log::error!("{err}"); - } - return false; + return Err(CliError::Msg(path_errors.join("; "))); } if !tmux::is_available() { - log::error!("tmux is not available; install it or run `smitebot doctor` for details"); - return false; + return Err(CliError::Msg( + "tmux is not available; install it or run `smitebot doctor` for details" + .to_string(), + )); } let campaign_id = config.campaign_id(); @@ -77,54 +71,45 @@ impl StartCommand { .unwrap_or_else(|| campaign_id.clone()); if tmux::session_exists(&tmux_session) { - log::error!( + return Err(CliError::Msg(format!( "tmux session '{tmux_session}' already exists — is another campaign running?" - ); - return false; + ))); } if let Some(stats) = existing_campaign_runner(&config.output_dir, config.runners) { - log::error!( + return Err(CliError::Msg(format!( "output_dir already holds a campaign ({}); smitebot start only \ begins fresh campaigns and does not resume — remove the directory \ or set a different output_dir", stats.display() - ); - return false; + ))); } let image = config.image_tag(); log::info!("building Docker image {image}"); let inputs = BuildInputs::from_config(&config, &image); - if !run_build(&inputs) { - return false; - } + run_build(&inputs)?; - if config.scenario.starts_with("ir") && !build_ir_mutator(&config.smite_dir) { - return false; + if config.scenario.starts_with("ir") { + build_ir_mutator(&config.smite_dir)?; } log::info!("setting up Nyx sharedir at {}", config.sharedir.display()); - if !setup_nyx(&config, &image) { - return false; - } + setup_nyx(&config, &image)?; - let seed_dir = match ensure_seed_dir(&config) { - Ok(dir) => dir, - Err(e) => { - log::error!("failed to prepare seed directory: {e}"); - return false; - } - }; + let seed_dir = ensure_seed_dir(&config) + .map_err(|e| CliError::Msg(format!("failed to prepare seed directory: {e}")))?; let Some(git_hash) = smite_git_hash(&config.smite_dir) else { - log::error!("could not determine smite git hash"); - return false; + return Err(CliError::Msg( + "could not determine smite git hash".to_string(), + )); }; let Some(image_digest) = docker_image_id(&image) else { - log::error!("could not determine Docker image digest for {image}"); - return false; + return Err(CliError::Msg(format!( + "could not determine Docker image digest for {image}" + ))); }; let mut state = CampaignState::new( @@ -136,18 +121,10 @@ impl StartCommand { tmux_session, ); - if let Err(e) = state.save_campaign() { - log::error!("{e}"); - return false; - } + state.save_campaign()?; - if !launch_runners(&config, &seed_dir, &mut state) { - return false; - } - if let Err(e) = state.save_campaign() { - log::error!("{e}"); - return false; - } + launch_runners(&config, &seed_dir, &mut state)?; + state.save_campaign()?; log::info!("campaign {} is running", state.id); log::info!("state saved to ~/.smitebot/runs/{}/state.json", state.id); @@ -157,13 +134,17 @@ impl StartCommand { log::warn!("failed to attach to tmux session: {e}"); } - true + Ok(()) } } /// Spawns all runners inside a tmux session, verifies they produce /// `fuzzer_stats`, and updates campaign state with PIDs. -fn launch_runners(config: &CampaignConfig, seed_dir: &Path, state: &mut CampaignState) -> bool { +fn launch_runners( + config: &CampaignConfig, + seed_dir: &Path, + state: &mut CampaignState, +) -> Result<(), CliError> { let session = &state.tmux_session; log::info!( "starting {} runners in tmux session '{session}'", @@ -184,10 +165,10 @@ fn launch_runners(config: &CampaignConfig, seed_dir: &Path, state: &mut Campaign }; if let Err(e) = result { - log::error!("failed to create tmux window for runner {id}: {e}"); + let msg = format!("failed to create tmux window for runner {id}: {e}"); state.runners = runners; fail_campaign(state); - return false; + return Err(CliError::Msg(msg)); } runners.push(RunnerState { id, pid: None }); @@ -208,13 +189,14 @@ fn launch_runners(config: &CampaignConfig, seed_dir: &Path, state: &mut Campaign ) { // verify_startup has already logged the specific reason per runner // (window died, or ceiling reached). - log::error!("one or more runners failed to start"); fail_campaign(state); - return false; + return Err(CliError::Msg( + "one or more runners failed to start".to_string(), + )); } state.status = Status::Running; - true + Ok(()) } /// Marks the campaign as failed, logs instructions to inspect the tmux session, @@ -557,26 +539,21 @@ fn ir_mutator_envs(config: &CampaignConfig) -> Vec<(&'static str, String)> { } /// Builds the smite-ir-mutator shared library in release mode. -fn build_ir_mutator(smite_dir: &Path) -> bool { +fn build_ir_mutator(smite_dir: &Path) -> Result<(), CliError> { log::info!("building IR mutator library"); - let status = match Command::new("cargo") + let status = Command::new("cargo") .args(["build", "--release", "-p", "smite-ir-mutator"]) .current_dir(smite_dir) .status() - { - Ok(status) => status, - Err(e) => { - log::error!("failed to run cargo build: {e}"); - return false; - } - }; + .map_err(|e| CliError::Msg(format!("failed to run cargo build: {e}")))?; if !status.success() { - log::error!("cargo build for smite-ir-mutator failed with {status}"); - return false; + return Err(CliError::Msg(format!( + "cargo build for smite-ir-mutator failed with {status}" + ))); } - true + Ok(()) } /// Returns the smite repository git hash, or `None` if not a git repo. diff --git a/smitebot/src/commands/status.rs b/smitebot/src/commands/status.rs index a46b7050..fc738654 100644 --- a/smitebot/src/commands/status.rs +++ b/smitebot/src/commands/status.rs @@ -13,6 +13,7 @@ use std::path::Path; use clap::Args; +use crate::error::CliError; use crate::state::{CampaignState, RunnerState, Status}; use crate::tmux; use crate::utils::{self, shell_quote}; @@ -43,14 +44,8 @@ pub struct StatusArgs { impl StatusCommand { /// Reports the status of a campaign, either as a one-shot summary or by /// attaching to its live tmux dashboard. - pub fn execute(args: &StatusArgs) -> bool { - let state = match CampaignState::load_campaign(&args.campaign_id) { - Ok(s) => s, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + pub fn execute(args: &StatusArgs) -> Result<(), CliError> { + let state = CampaignState::load_campaign(&args.campaign_id)?; // state.status is written only at start and by `stop`, so it can be // stale (e.g. still "running" for a campaign that has since crashed). @@ -59,24 +54,20 @@ impl StatusCommand { let session_alive = tmux::session_exists(&state.tmux_session); if !session_alive || args.summary { - let alive_runner_ids = match alive_runners(&state, session_alive) { - Ok(ids) => ids, - Err(e) => { - // A live session we can't query is anomalous (both this and - // the session check use tmux). Fail loudly rather than - // silently render every runner as dead. - log::error!( - "could not determine per-runner liveness in session '{}': {e}", - state.tmux_session - ); - return false; - } - }; + // A live session we can't query is anomalous (both this and the + // session check use tmux). Fail loudly rather than silently render + // every runner as dead. + let alive_runner_ids = alive_runners(&state, session_alive).map_err(|e| { + CliError::Msg(format!( + "could not determine per-runner liveness in session '{}': {e}", + state.tmux_session + )) + })?; print!( "{}", render_summary(&state, session_alive, &alive_runner_ids) ); - return true; + return Ok(()); } attach_dashboard(&state) @@ -85,7 +76,7 @@ impl StatusCommand { /// Ensures a single reloading status window exists in the campaign session, /// then attaches to it. -fn attach_dashboard(state: &CampaignState) -> bool { +fn attach_dashboard(state: &CampaignState) -> Result<(), CliError> { let session = &state.tmux_session; // Re-run guard: reuse the window if `status` was already run on this live @@ -94,14 +85,16 @@ fn attach_dashboard(state: &CampaignState) -> bool { Ok(true) => log::info!("status window already exists in session '{session}', attaching"), Ok(false) => { let cmd = dashboard_command(&state.id); - if let Err(e) = tmux::add_window(session, STATUS_WINDOW, &cmd) { - log::error!("failed to create status window in session '{session}': {e}"); - return false; - } + tmux::add_window(session, STATUS_WINDOW, &cmd).map_err(|e| { + CliError::Msg(format!( + "failed to create status window in session '{session}': {e}" + )) + })?; } Err(e) => { - log::error!("failed to query tmux session '{session}': {e}"); - return false; + return Err(CliError::Msg(format!( + "failed to query tmux session '{session}': {e}" + ))); } } @@ -109,16 +102,15 @@ fn attach_dashboard(state: &CampaignState) -> bool { // status window is active right after we create it, but if the user switched // to a runner window and detached, a later `status` run would otherwise // attach to that runner window — so select the status window explicitly. - if let Err(e) = tmux::select_window(session, STATUS_WINDOW) { - log::error!("failed to select status window in session '{session}': {e}"); - return false; - } - - if let Err(e) = tmux::attach(session) { - log::error!("failed to attach to tmux session '{session}': {e}"); - return false; - } - true + tmux::select_window(session, STATUS_WINDOW).map_err(|e| { + CliError::Msg(format!( + "failed to select status window in session '{session}': {e}" + )) + })?; + + tmux::attach(session) + .map_err(|e| CliError::Msg(format!("failed to attach to tmux session '{session}': {e}")))?; + Ok(()) } /// Builds the shell command for the reloading status window. diff --git a/smitebot/src/commands/stop.rs b/smitebot/src/commands/stop.rs index ef6a72b3..00b6075c 100644 --- a/smitebot/src/commands/stop.rs +++ b/smitebot/src/commands/stop.rs @@ -12,6 +12,7 @@ use std::time::{Duration, Instant}; use clap::Args; +use crate::error::CliError; use crate::state::{CampaignState, Status}; use crate::tmux; use crate::utils; @@ -40,18 +41,12 @@ pub struct StopArgs { impl StopCommand { /// Stops a campaign: reaps its runner process groups, tears down the tmux /// session, and records the stop in state.json. - pub fn execute(args: &StopArgs) -> bool { - let mut state = match CampaignState::load_campaign(&args.campaign_id) { - Ok(s) => s, - Err(e) => { - log::error!("{e}"); - return false; - } - }; + pub fn execute(args: &StopArgs) -> Result<(), CliError> { + let mut state = CampaignState::load_campaign(&args.campaign_id)?; if state.status == Status::Stopped { log::info!("campaign {} is already stopped", state.id); - return true; + return Ok(()); } let clean = terminate_runners(&state); @@ -63,25 +58,24 @@ impl StopCommand { state.status = Status::Stopped; state.stop_time = Some(utils::epoch_secs()); - if let Err(e) = state.save_campaign() { - log::error!( + state.save_campaign().map_err(|e| { + CliError::Msg(format!( "runners were reaped but recording the stop failed: {e}; \ campaign {} will still show as running in state.json", state.id - ); - return false; - } + )) + })?; if clean { log::info!("campaign {} stopped", state.id); + Ok(()) } else { - log::error!( + Err(CliError::Msg(format!( "campaign {} stopped, but some processes survived SIGKILL; \ inspect with `pgrep -a qemu` and kill them manually", state.id - ); + ))) } - clean } } diff --git a/smitebot/src/error.rs b/smitebot/src/error.rs new file mode 100644 index 00000000..74c6d7ad --- /dev/null +++ b/smitebot/src/error.rs @@ -0,0 +1,34 @@ +//! The error type returned by every command handler. +//! +//! Each `execute` returns `Result<(), CliError>` and `main` logs the error once +//! at the top level, instead of every failure site calling `log::error!` and +//! returning `false` (issue #270). This keeps logging in one place and lets the +//! command bodies use `?`. + +use crate::config::ConfigError; +use crate::state::StateError; + +/// A failure from a command handler, logged once by `main`. +#[derive(Debug, thiserror::Error)] +pub enum CliError { + /// A configuration file failed to load or validate. + #[error(transparent)] + Config(#[from] ConfigError), + /// Campaign state failed to load or save. + #[error(transparent)] + State(#[from] StateError), + /// An operational failure described by a ready-to-log message. + #[error("{0}")] + Msg(String), + /// The `config`/`doctor` report — whose printed output (including its JSON) + /// is the artifact — came back negative. The command already printed the + /// result, so `main` sets a non-zero exit code but logs nothing further. + #[error("")] + Reported, +} + +impl From for CliError { + fn from(message: String) -> Self { + CliError::Msg(message) + } +} diff --git a/smitebot/src/main.rs b/smitebot/src/main.rs index e448009d..00341c57 100644 --- a/smitebot/src/main.rs +++ b/smitebot/src/main.rs @@ -2,6 +2,7 @@ mod commands; mod config; +mod error; mod latency_stats; mod libnyx; mod state; @@ -18,6 +19,7 @@ use commands::{ ReproduceArgs, ReproduceCommand, StartArgs, StartCommand, StatusArgs, StatusCommand, StopArgs, StopCommand, }; +use error::CliError; #[derive(Debug, Parser)] #[command(name = "smitebot", version, about = "Smite campaign manager")] @@ -54,7 +56,7 @@ fn main() -> ExitCode { simple_logger::init_with_env().expect("Failed to initialize logger"); let cli = Cli::parse(); - let success = match cli.command { + let result = match cli.command { Commands::BenchExec(args) => BenchExecCommand::execute(&args), Commands::Build(args) => BuildCommand::execute(&args), Commands::Config(args) => ConfigCommand::execute(&args), @@ -67,9 +69,13 @@ fn main() -> ExitCode { Commands::Stop(args) => StopCommand::execute(&args), }; - if success { - ExitCode::SUCCESS - } else { - ExitCode::FAILURE + match result { + Ok(()) => ExitCode::SUCCESS, + // The command already surfaced the failure itself (see CliError::Reported). + Err(CliError::Reported) => ExitCode::FAILURE, + Err(e) => { + log::error!("{e}"); + ExitCode::FAILURE + } } } diff --git a/smitebot/src/utils.rs b/smitebot/src/utils.rs index d5adae1e..3cd98d4d 100644 --- a/smitebot/src/utils.rs +++ b/smitebot/src/utils.rs @@ -7,6 +7,7 @@ use std::process::Command; use std::time::{SystemTime, UNIX_EPOCH}; use crate::config::CampaignConfig; +use crate::error::CliError; /// Returns the current Unix timestamp in seconds. pub fn epoch_secs() -> u64 { @@ -45,33 +46,28 @@ fn find_in_path_with_path(tool: &str, path_var: &OsStr) -> Option { } /// Runs `scripts/setup-nyx.sh` to prepare the Nyx sharedir. -pub fn setup_nyx(config: &CampaignConfig, image: &str) -> bool { +pub fn setup_nyx(config: &CampaignConfig, image: &str) -> Result<(), CliError> { let script = config.smite_dir.join("scripts").join("setup-nyx.sh"); if !script.exists() { - log::error!("setup-nyx.sh not found: {}", script.display()); - return false; + return Err(CliError::Msg(format!( + "setup-nyx.sh not found: {}", + script.display() + ))); } - let status = match Command::new(&script) + let status = Command::new(&script) .arg(&config.sharedir) .arg(image) .arg(&config.aflpp_path) .status() - { - Ok(status) => status, - Err(e) => { - log::error!("failed to run setup-nyx.sh: {e}"); - return false; - } - }; + .map_err(|e| CliError::Msg(format!("failed to run setup-nyx.sh: {e}")))?; if !status.success() { - log::error!("setup-nyx.sh failed with {status}"); - return false; + return Err(CliError::Msg(format!("setup-nyx.sh failed with {status}"))); } log::info!("Nyx sharedir ready at {}", config.sharedir.display()); - true + Ok(()) } /// Pins the calling thread, and every process it later spawns, to `cpu`.