diff --git a/Cargo.lock b/Cargo.lock index c956612..45e9b9b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -94,6 +94,7 @@ dependencies = [ "serde", "serde_json", "snailquote", + "socket2", ] [[package]] @@ -156,7 +157,6 @@ dependencies = [ "clap", "libssh-rs", "sha256", - "socket2", ] [[package]] diff --git a/ares-install/src/install.rs b/ares-install/src/install.rs index 42f837a..ffee649 100644 --- a/ares-install/src/install.rs +++ b/ares-install/src/install.rs @@ -2,11 +2,12 @@ use std::fmt::{Display, Formatter}; use std::fs::File; use std::io::{Error as IoError, ErrorKind}; use std::path::Path; +use std::sync::LazyLock; use std::time::Duration; use ares_connection_lib::luna::{Luna, LunaError, Message}; use ares_connection_lib::session::DeviceSession; -use ares_connection_lib::transfer::{FileTransfer, TransferError}; +use ares_connection_lib::transfer::{Transfer, TransferError}; use indicatif::{ProgressBar, ProgressStyle}; use regex::Regex; use serde::{Deserialize, Serialize}; @@ -18,13 +19,21 @@ pub(crate) trait InstallApp { #[derive(Debug)] pub enum InstallError { - Response { error_code: i32, reason: String }, + Response { + error_code: i32, + reason: String, + }, /// The install stream ended without ever saying how it went. NoVerdict, /// The connection went down with the install already under way, so what /// the device did with the package is not known. - Interrupted { device: String }, - ChecksumMismatch { expected: String, actual: String }, + Interrupted { + device: String, + }, + ChecksumMismatch { + expected: String, + actual: String, + }, Luna(LunaError), Transfer(TransferError), Io(IoError), @@ -103,7 +112,14 @@ impl InstallApp for DeviceSession { .map(|s| s.to_string_lossy()) .unwrap_or_else(|| package.as_ref().to_string_lossy()); - self.mkdir(Path::new("/media/developer/temp"), 0o777)?; + // One transport for the whole install. Each `Transfer::open` asks the + // device for the sftp subsystem, which execs an sftp-server there, so + // opening one per step paid for four where one does - and the one + // sha256sum opened was never used at all, since that runs over an exec + // channel. ares-push and ares-pull already work this way. + let transfer = Transfer::open(self); + + transfer.mkdir(Path::new("/media/developer/temp"), 0o777)?; let pb = ProgressBar::new(file_size); pb.suspend(|| { @@ -117,7 +133,7 @@ impl InstallApp for DeviceSession { pb.set_style(ProgressStyle::with_template("{prefix:10.bold.dim} {spinner} {percent:>3}% [{wide_bar}] {bytes}/{total_bytes} {eta} ETA") .unwrap()); - self.put(&mut file, &ipk_path, |transferred| { + transfer.put(&mut file, &ipk_path, |transferred| { pb.set_position(transferred as u64); })?; @@ -127,7 +143,7 @@ impl InstallApp for DeviceSession { pb.set_prefix("Verifying"); pb.set_message("Checking uploaded package"); - let verified = verify_upload(self, &ipk_path, &checksum); + let verified = verify_upload(&transfer, &ipk_path, &checksum); if let Err(e) = &verified { pb.suspend(|| eprintln!("Upload of {package_display_name} is broken: {e}")); } @@ -153,18 +169,14 @@ impl InstallApp for DeviceSession { ) { Ok(subscription) => subscription .filter_map(|item| { - map_installer_message( - item, - &Regex::new(r"(?i)installed").unwrap(), - |progress| { - pb.set_message( - progress - .strip_prefix("installing : ") - .unwrap_or(&progress) - .to_string(), - ); - }, - ) + map_installer_message(item, &INSTALLED, |progress| { + pb.set_message( + progress + .strip_prefix("installing : ") + .unwrap_or(&progress) + .to_string(), + ); + }) }) .next() // Reaching the end of the stream having seen neither @@ -191,7 +203,15 @@ impl InstallApp for DeviceSession { }); if let Ok(package_id) = &result { - pb.suspend(|| println!("Installed package {}!", package_id)); + pb.suspend(|| { + // The device does not always name what it installed. Better to + // say nothing than to print "Installed package !". + if package_id.is_empty() { + println!("Installed the package."); + } else { + println!("Installed package {package_id}!"); + } + }); } pb.set_prefix("Cleanup"); @@ -203,7 +223,7 @@ impl InstallApp for DeviceSession { // behind instead, and let the real error through. if self.is_connected() { pb.suspend(|| println!("Deleting uploaded package...")); - if let Err(e) = self.rm(&ipk_path) { + if let Err(e) = transfer.rm(&ipk_path) { pb.suspend(|| eprintln!("Failed to delete {ipk_path}: {e}")); } } else { @@ -222,12 +242,8 @@ impl InstallApp for DeviceSession { } /// Compare the uploaded package against the local file. Devices without `sha256sum` skip the check. -fn verify_upload( - session: &DeviceSession, - ipk_path: &str, - expected: &str, -) -> Result<(), InstallError> { - let Some(actual) = session.sha256sum(ipk_path)? else { +fn verify_upload(transfer: &Transfer, ipk_path: &str, expected: &str) -> Result<(), InstallError> { + let Some(actual) = transfer.sha256sum(ipk_path)? else { return Ok(()); }; if actual != expected { @@ -239,6 +255,15 @@ fn verify_upload( Ok(()) } +/// The device reports progress a line at a time, so these are matched once per +/// line. Building them per line meant compiling three regexes per message, and +/// three `unwrap`s that could panic on a hot path. +static FAILED: LazyLock = LazyLock::new(|| Regex::new(r"(?i)FAILED").unwrap()); +static SUCCEEDED: LazyLock = LazyLock::new(|| Regex::new(r"(?i)^SUCCESS").unwrap()); +pub(crate) static INSTALLED: LazyLock = + LazyLock::new(|| Regex::new(r"(?i)installed").unwrap()); +pub(crate) static REMOVED: LazyLock = LazyLock::new(|| Regex::new(r"(?i)removed").unwrap()); + pub(crate) fn map_installer_message( item: std::io::Result, expected: &Regex, @@ -249,14 +274,12 @@ pub(crate) fn map_installer_message( Ok(resp) => { if let Some(details) = resp.details { if let Some(state) = details.state { - if Regex::new(r"(?i)FAILED").unwrap().is_match(&state) { + if FAILED.is_match(&state) { return Some(Err(InstallError::Response { error_code: details.error_code.unwrap_or(0), reason: details.reason.unwrap_or(String::from("unknown error")), })); - } else if Regex::new(r"(?i)^SUCCESS").unwrap().is_match(&state) - || expected.is_match(&state) - { + } else if SUCCEEDED.is_match(&state) || expected.is_match(&state) { return Some(Ok(details.package_id.unwrap_or(String::from("")))); } else { progress(state); diff --git a/ares-install/src/remove.rs b/ares-install/src/remove.rs index 6085f01..d45fc58 100644 --- a/ares-install/src/remove.rs +++ b/ares-install/src/remove.rs @@ -1,9 +1,8 @@ use ares_connection_lib::luna::Luna; use ares_connection_lib::session::DeviceSession; -use regex::Regex; use serde::Serialize; -use crate::install::{InstallError, map_installer_message}; +use crate::install::{InstallError, REMOVED, map_installer_message}; pub(crate) trait RemoveApp { fn remove_app(&self, package_id: &str) -> Result; @@ -28,7 +27,7 @@ impl RemoveApp for DeviceSession { ) { Ok(subscription) => subscription .filter_map(|item| { - map_installer_message(item, &Regex::new(r"(?i)removed").unwrap(), |progress| { + map_installer_message(item, &REMOVED, |progress| { println!("{}", progress); }) }) @@ -36,6 +35,9 @@ impl RemoveApp for DeviceSession { Err(e) => Some(Err(e.into())), }; - result.unwrap() + // A stream that ends having said neither "removed" nor a failure used + // to panic here. It is the same silence an install can meet, and it + // deserves the same answer. + result.unwrap_or(Err(InstallError::NoVerdict)) } } diff --git a/ares-novacom/Cargo.toml b/ares-novacom/Cargo.toml index 04f462a..7a42e2c 100644 --- a/ares-novacom/Cargo.toml +++ b/ares-novacom/Cargo.toml @@ -19,7 +19,6 @@ ares-device-lib = { workspace = true } ares-connection-lib = { workspace = true } clap = { workspace = true, features = ["derive", "env"] } libssh-rs = { workspace = true } -socket2 = { workspace = true } sha256 = { workspace = true } [package.metadata.deb] diff --git a/ares-novacom/README.md b/ares-novacom/README.md index 341075c..fab535a 100644 --- a/ares-novacom/README.md +++ b/ares-novacom/README.md @@ -48,9 +48,10 @@ them. Both keep running until you stop them with Ctrl+C. A device that goes away without closing the connection — one dropping off -Wi-Fi, rather than one shutting the session down — used to leave the forward -waiting on a tunnel that no longer carried anything. The connection is kept -alive now, so the forward notices within about a minute and exits saying so. +Wi-Fi, rather than one shutting the session down — would otherwise leave the +forward waiting on a tunnel that no longer carries anything. Every connection +is kept alive, so the forward notices within about a minute and exits saying +so. ## Examples diff --git a/ares-novacom/src/main.rs b/ares-novacom/src/main.rs index 6007069..9b7a5bf 100644 --- a/ares-novacom/src/main.rs +++ b/ares-novacom/src/main.rs @@ -1,9 +1,5 @@ use std::io::{Error as IoError, ErrorKind, Read, Write}; use std::net::{TcpListener, TcpStream}; -#[cfg(unix)] -use std::os::fd::{AsRawFd, BorrowedFd}; -#[cfg(windows)] -use std::os::windows::io::{AsRawSocket, BorrowedSocket}; use std::path::Path; use std::process::exit; use std::sync::Arc; @@ -16,7 +12,6 @@ use ares_device_lib::cli::unwrap_or_exit; use ares_device_lib::{DeviceManager, PrivateKey}; use clap::Parser; use libssh_rs::{Channel, Error as SshError}; -use socket2::{SockRef, TcpKeepalive}; #[derive(Parser, Debug)] #[command(about)] @@ -86,33 +81,6 @@ fn main() { } } -/// A forward sits idle for hours, and a device that goes away without closing -/// the connection - a TV dropping off Wi-Fi, rather than one that shuts the -/// session down - leaves the host holding a socket it still believes in. The -/// forward then waits forever for connections that can no longer arrive. -/// -/// TCP keepalive is what notices. When the probes go unanswered the socket -/// fails, libssh sees the error, and the loop below finds `is_connected()` -/// false and says so. -fn keep_alive(session: &DeviceSession) { - let keepalive = TcpKeepalive::new() - .with_time(Duration::from_secs(30)) - .with_interval(Duration::from_secs(10)); - // Windows counts its own retries and has no knob for it. - #[cfg(not(windows))] - let keepalive = keepalive.with_retries(3); - - // Best effort: a forward that cannot set this still works, it just goes on - // trusting a dead socket for as long as TCP does. - #[cfg(unix)] - let socket = unsafe { BorrowedFd::borrow_raw(session.as_raw_fd()) }; - #[cfg(windows)] - let socket = unsafe { BorrowedSocket::borrow_raw(session.as_raw_socket()) }; - if let Err(e) = SockRef::from(&socket).set_tcp_keepalive(&keepalive) { - eprintln!("Could not set keepalive on the connection: {e}"); - } -} - /// Local port-forward: accept TCP connections on a host port and tunnel each /// through the device's SSH session to `localhost:` on the device. fn forward(manager: &DeviceManager, device: Option<&str>, port_spec: Option<&str>) { @@ -135,7 +103,6 @@ fn forward(manager: &DeviceManager, device: Option<&str>, port_spec: Option<&str }; let session = unwrap_or_exit(device.new_session(), &format!("connect to {}", device.host)); - keep_alive(&session); let session = Arc::new(session); let listener = unwrap_or_exit( @@ -190,7 +157,6 @@ fn reverse(manager: &DeviceManager, device: Option<&str>, port_spec: Option<&str }; let session = unwrap_or_exit(device.new_session(), &format!("connect to {}", device.host)); - keep_alive(&session); // Bind the device's loopback, not all of its interfaces: the point is to // let something running on the device reach the host, not to put the host diff --git a/ares-package/src/input/validation.rs b/ares-package/src/input/validation.rs index ae0fb75..91de917 100644 --- a/ares-package/src/input/validation.rs +++ b/ares-package/src/input/validation.rs @@ -45,9 +45,10 @@ impl Validation for ComponentInfo { let size = dir_size(&self.path, self.excludes.as_ref())?; let mut arch: Option = None; if let (Some(engine), Some(executable)) = (&self.info.engine, &self.info.executable) - && engine == "native" { - arch = infer_arch(self.path.join(executable), force_arch)?; - } + && engine == "native" + { + arch = infer_arch(self.path.join(executable), force_arch)?; + } Ok(ValidationInfo { arch, size }) } } @@ -88,7 +89,10 @@ fn infer_arch>(path: P, allow_unknown: bool) -> Result Result; @@ -27,9 +33,44 @@ pub fn connect(device: &Device) -> Result { session.set_option(SshOption::Port(device.port))?; session.set_option(SshOption::User(Some(device.username.clone())))?; session.connect()?; + keep_alive(&session); Ok(session) } +/// Ask TCP to notice a device that stops answering. +/// +/// A device that goes away without closing the connection - one dropping off +/// Wi-Fi, rather than one shutting the session down - sends no FIN and no RST, +/// so the host goes on believing in a socket that leads nowhere. Anything +/// waiting to read then waits forever: an install sitting on a progress +/// subscription, a forward waiting for connections that can no longer arrive. +/// +/// Probing turns that silence into an error the caller can report. The probes +/// are answered by the peer's TCP stack rather than by whatever it is running, +/// so a device that is merely busy - unpacking a package, or swapping - keeps +/// answering and is left alone. +/// +/// Called for you by [`connect`]. Call it yourself only when you build a +/// session some other way. +pub fn keep_alive(session: &Session) { + let keepalive = TcpKeepalive::new() + .with_time(Duration::from_secs(30)) + .with_interval(Duration::from_secs(10)); + // Windows counts its own retries and has no knob for it. + #[cfg(not(windows))] + let keepalive = keepalive.with_retries(3); + + // Best effort: a session that cannot set this still works, it just goes on + // trusting a dead socket for as long as TCP does. + #[cfg(unix)] + let socket = unsafe { BorrowedFd::borrow_raw(session.as_raw_fd()) }; + #[cfg(windows)] + let socket = unsafe { BorrowedSocket::borrow_raw(session.as_raw_socket()) }; + if let Err(e) = SockRef::from(&socket).set_tcp_keepalive(&keepalive) { + eprintln!("Could not set keepalive on the connection: {e}"); + } +} + /// Authenticate a connected session as `device`. /// /// `key` is the private key itself, in OpenSSH format. The caller reads it, diff --git a/common/device/src/manager.rs b/common/device/src/manager.rs index a8f6492..7b19018 100644 --- a/common/device/src/manager.rs +++ b/common/device/src/manager.rs @@ -330,7 +330,9 @@ mod tests { }); let stored = manager.add(&tv).unwrap(); - assert!(matches!(stored.private_key, Some(PrivateKey::Name { name }) if name == "webos_tv")); + assert!( + matches!(stored.private_key, Some(PrivateKey::Name { name }) if name == "webos_tv") + ); remove_dir_all(&dir).ok(); } @@ -345,7 +347,9 @@ mod tests { }); let stored = manager.add(&tv).unwrap(); - assert!(matches!(stored.private_key, Some(PrivateKey::Name { name }) if name == "webos_tv")); + assert!( + matches!(stored.private_key, Some(PrivateKey::Name { name }) if name == "webos_tv") + ); remove_dir_all(&dir).ok(); }