From 784e9ee24d977c99dcb5ba5aef81dae48cb899fb Mon Sep 17 00:00:00 2001 From: Roman Melnikov Date: Wed, 19 Apr 2023 17:46:43 +0800 Subject: [Chore] Handle 'temp_path' as an actual 'Path' instead of 'String' Problem: 'temp_path' and 'lock_path' are handled as 'String'. This can be a problem when the 'temp_path' directory is a symlink on the target system, e.g. this is the case with the default '/tmp' and macOS, where this directory is actually a symlink to '/private/tmp'. Solution: Handle 'temp_path' and 'lock_path' as actual Paths. Also, canonicalize 'temp_path' to avoid canary file path mismatches when checking filesystem events. As a side effect, also update the 'notify' dependency to the latest stable version. --- src/bin/activate.rs | 27 ++++++++++++++++----------- src/cli.rs | 3 ++- src/data.rs | 3 ++- src/deploy.rs | 30 +++++++++++++++--------------- src/lib.rs | 8 +++++--- 5 files changed, 40 insertions(+), 31 deletions(-) (limited to 'src') diff --git a/src/bin/activate.rs b/src/bin/activate.rs index 4c865f1..bf03538 100644 --- a/src/bin/activate.rs +++ b/src/bin/activate.rs @@ -15,9 +15,9 @@ use tokio::time::timeout; use std::time::Duration; -use std::path::Path; +use std::path::PathBuf; -use notify::{RecommendedWatcher, RecursiveMode, Watcher}; +use notify::{RecommendedWatcher, RecursiveMode, Watcher, recommended_watcher}; use thiserror::Error; @@ -75,7 +75,7 @@ struct ActivateOpts { /// Path for any temporary files that may be needed during activation #[clap(long)] - temp_path: String, + temp_path: PathBuf, } /// Activate a profile @@ -86,7 +86,7 @@ struct WaitOpts { /// Path for any temporary files that may be needed during activation #[clap(long)] - temp_path: String, + temp_path: PathBuf, } /// Activate a profile @@ -232,7 +232,7 @@ async fn danger_zone( pub async fn activation_confirmation( profile_path: String, - temp_path: String, + temp_path: PathBuf, confirm_timeout: u16, closure: String, ) -> Result<(), ActivationConfirmationError> { @@ -240,7 +240,7 @@ pub async fn activation_confirmation( debug!("Ensuring parent directory exists for canary file"); - if let Some(parent) = Path::new(&lock_path).parent() { + if let Some(parent) = lock_path.parent() { fs::create_dir_all(parent) .await .map_err(ActivationConfirmationError::CreateConfirmDir)?; @@ -257,7 +257,7 @@ pub async fn activation_confirmation( let (deleted, done) = mpsc::channel(1); let mut watcher: RecommendedWatcher = - Watcher::new_immediate(move |res: Result| { + recommended_watcher(move |res: Result| { let send_result = match res { Ok(e) if e.kind == notify::EventKind::Remove(notify::event::RemoveKind::File) => { debug!("Got worthy removal event, sending on channel"); @@ -298,7 +298,7 @@ pub enum WaitError { #[error("Error waiting for activation: {0}")] Waiting(#[from] DangerZoneError), } -pub async fn wait(temp_path: String, closure: String) -> Result<(), WaitError> { +pub async fn wait(temp_path: PathBuf, closure: String) -> Result<(), WaitError> { let lock_path = deploy::make_lock_path(&temp_path, &closure); let (created, done) = mpsc::channel(1); @@ -307,11 +307,16 @@ pub async fn wait(temp_path: String, closure: String) -> Result<(), WaitError> { // TODO: fix wasteful clone let lock_path = lock_path.clone(); - Watcher::new_immediate(move |res: Result| { + recommended_watcher(move |res: Result| { let send_result = match res { Ok(e) if e.kind == notify::EventKind::Create(notify::event::CreateKind::File) => { match &e.paths[..] { - [x] if x == Path::new(&lock_path) => created.try_send(Ok(())), + [x] => match lock_path.canonicalize() { + // 'lock_path' may not exist yet when some other files are created in 'temp_path' + // x is already supposed to be canonical path + Ok(lock_path) if x == &lock_path => created.try_send(Ok(())), + _ => Ok (()) + } _ => Ok(()), } } @@ -363,7 +368,7 @@ pub async fn activate( profile_path: String, closure: String, auto_rollback: bool, - temp_path: String, + temp_path: PathBuf, confirm_timeout: u16, magic_rollback: bool, dry_activate: bool, diff --git a/src/cli.rs b/src/cli.rs index f259563..9140d6d 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -14,6 +14,7 @@ use self::deploy::{DeployFlake, ParseFlakeError}; use futures_util::stream::{StreamExt, TryStreamExt}; use log::{debug, error, info, warn}; use serde::Serialize; +use std::path::PathBuf; use std::process::Stdio; use thiserror::Error; use tokio::process::Command; @@ -86,7 +87,7 @@ pub struct Opts { confirm_timeout: Option, /// Where to store temporary files (only used by magic-rollback) #[clap(long)] - temp_path: Option, + temp_path: Option, /// Show what will be activated on the machines #[clap(long)] dry_activate: bool, diff --git a/src/data.rs b/src/data.rs index 90ea331..3b3e2c9 100644 --- a/src/data.rs +++ b/src/data.rs @@ -5,6 +5,7 @@ use merge::Merge; use serde::Deserialize; use std::collections::HashMap; +use std::path::PathBuf; #[derive(Deserialize, Debug, Clone, Merge)] pub struct GenericSettings { @@ -25,7 +26,7 @@ pub struct GenericSettings { #[serde(rename(deserialize = "confirmTimeout"))] pub confirm_timeout: Option, #[serde(rename(deserialize = "tempPath"))] - pub temp_path: Option, + pub temp_path: Option, #[serde(rename(deserialize = "magicRollback"))] pub magic_rollback: Option, #[serde(rename(deserialize = "sudo"))] diff --git a/src/deploy.rs b/src/deploy.rs index cc5d862..574e9b2 100644 --- a/src/deploy.rs +++ b/src/deploy.rs @@ -5,7 +5,7 @@ // SPDX-License-Identifier: MPL-2.0 use log::{debug, info}; -use std::borrow::Cow; +use std::path::Path; use thiserror::Error; use tokio::process::Command; @@ -16,7 +16,7 @@ struct ActivateCommandData<'a> { profile_path: &'a str, closure: &'a str, auto_rollback: bool, - temp_path: &'a str, + temp_path: &'a Path, confirm_timeout: u16, magic_rollback: bool, debug_logs: bool, @@ -38,7 +38,7 @@ fn build_activate_command(data: &ActivateCommandData) -> String { self_activate_command = format!( "{} activate '{}' '{}' --temp-path '{}'", - self_activate_command, data.closure, data.profile_path, data.temp_path + self_activate_command, data.closure, data.profile_path, data.temp_path.display() ); self_activate_command = format!( @@ -77,7 +77,7 @@ fn test_activation_command_builder() { let auto_rollback = true; let dry_activate = false; let boot = false; - let temp_path = "/tmp"; + let temp_path = Path::new("/tmp"); let confirm_timeout = 30; let magic_rollback = true; let debug_logs = true; @@ -105,7 +105,7 @@ fn test_activation_command_builder() { struct WaitCommandData<'a> { sudo: &'a Option, closure: &'a str, - temp_path: &'a str, + temp_path: &'a Path, debug_logs: bool, log_dir: Option<&'a str>, } @@ -123,7 +123,7 @@ fn build_wait_command(data: &WaitCommandData) -> String { self_activate_command = format!( "{} wait '{}' --temp-path '{}'", - self_activate_command, data.closure, data.temp_path, + self_activate_command, data.closure, data.temp_path.display(), ); if let Some(sudo_cmd) = &data.sudo { @@ -137,7 +137,7 @@ fn build_wait_command(data: &WaitCommandData) -> String { fn test_wait_command_builder() { let sudo = Some("sudo -u test".to_string()); let closure = "/nix/store/blah/etc"; - let temp_path = "/tmp"; + let temp_path = Path::new("/tmp"); let debug_logs = true; let log_dir = Some("/tmp/something.txt"); @@ -216,7 +216,7 @@ pub enum ConfirmProfileError { pub async fn confirm_profile( deploy_data: &super::DeployData<'_>, deploy_defs: &super::DeployDefs, - temp_path: Cow<'_, str>, + temp_path: &Path, ssh_addr: &str, ) -> Result<(), ConfirmProfileError> { let mut ssh_confirm_command = Command::new("ssh"); @@ -226,9 +226,9 @@ pub async fn confirm_profile( ssh_confirm_command.arg(ssh_opt); } - let lock_path = super::make_lock_path(&temp_path, &deploy_data.profile.profile_settings.path); + let lock_path = super::make_lock_path(temp_path, &deploy_data.profile.profile_settings.path); - let mut confirm_command = format!("rm {}", lock_path); + let mut confirm_command = format!("rm {}", lock_path.display()); if let Some(sudo_cmd) = &deploy_defs.sudo { confirm_command = format!("{} {}", sudo_cmd, confirm_command); } @@ -286,9 +286,9 @@ pub async fn deploy_profile( ); } - let temp_path: Cow = match &deploy_data.merged_settings.temp_path { - Some(x) => x.into(), - None => "/tmp".into(), + let temp_path: &Path = match &deploy_data.merged_settings.temp_path { + Some(x) => x, + None => Path::new("/tmp"), }; let confirm_timeout = deploy_data.merged_settings.confirm_timeout.unwrap_or(30); @@ -302,7 +302,7 @@ pub async fn deploy_profile( profile_path: &deploy_defs.profile_path, closure: &deploy_data.profile.profile_settings.path, auto_rollback, - temp_path: &temp_path, + temp_path: temp_path, confirm_timeout, magic_rollback, debug_logs: deploy_data.debug_logs, @@ -350,7 +350,7 @@ pub async fn deploy_profile( let self_wait_command = build_wait_command(&WaitCommandData { sudo: &deploy_defs.sudo, closure: &deploy_data.profile.profile_settings.path, - temp_path: &temp_path, + temp_path: temp_path, debug_logs: deploy_data.debug_logs, log_dir: deploy_data.log_dir, }); diff --git a/src/lib.rs b/src/lib.rs index 738fa81..bf1a6dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,10 +12,12 @@ use thiserror::Error; use flexi_logger::*; -pub fn make_lock_path(temp_path: &str, closure: &str) -> String { +use std::path::{Path, PathBuf}; + +pub fn make_lock_path(temp_path: &Path, closure: &str) -> PathBuf { let lock_hash = &closure["/nix/store/".len()..closure.find('-').unwrap_or_else(|| closure.len())]; - format!("{}/deploy-rs-canary-{}", temp_path, lock_hash) + temp_path.join(format!("deploy-rs-canary-{}", lock_hash)) } const fn make_emoji(level: log::Level) -> &'static str { @@ -159,7 +161,7 @@ pub struct CmdOverrides { pub auto_rollback: Option, pub hostname: Option, pub magic_rollback: Option, - pub temp_path: Option, + pub temp_path: Option, pub confirm_timeout: Option, pub sudo: Option, pub dry_activate: bool, -- cgit v1.2.3