ares-install: one transport per install, and three smaller fixes

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<Regex> = LazyLock::new(|| Regex::new(r"(?i)FAILED").unwrap());
+static SUCCEEDED: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(?i)^SUCCESS").unwrap());
+pub(crate) static INSTALLED: LazyLock<Regex> =
+    LazyLock::new(|| Regex::new(r"(?i)installed").unwrap());
+pub(crate) static REMOVED: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(?i)removed").unwrap());
+
 pub(crate) fn map_installer_message<F: Fn(String)>(
     item: std::io::Result<Message>,
     expected: &Regex,
@@ -249,14 +274,12 @@ pub(crate) fn map_installer_message<F: Fn(String)>(
             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);

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<String, InstallError>;
@@ -28,14 +27,17 @@ 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);
                     })
                 })
                 .next(),
             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))
     }
 }

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]

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
 

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:<device_port>` 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

ares-package/src/input/validation.rs

@@ -45,9 +45,10 @@ impl Validation for ComponentInfo<ServiceInfo> {
         let size = dir_size(&self.path, self.excludes.as_ref())?;
         let mut arch: Option<PackageArch> = 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<P: AsRef<Path>>(path: P, allow_unknown: bool) -> Result<Option<Pac
             } else {
                 Err(Error::new(
                     ErrorKind::InvalidData,
-                    format!("Unsupported binary machine type {}", e_machine_to_string(other)),
+                    format!(
+                        "Unsupported binary machine type {}",
+                        e_machine_to_string(other)
+                    ),
                 ))
             }
         }

ares-package/src/main.rs

@@ -83,7 +83,10 @@ fn main() {
         return;
     }
     if forced.is_some() {
-        eprintln!("Warning: architecture {} was explicitly forced via -A", arch);
+        eprintln!(
+            "Warning: architecture {} was explicitly forced via -A",
+            arch
+        );
     }
 
     let path = outdir.join(format!(

common/connection/Cargo.toml

@@ -19,6 +19,7 @@ ares-device-lib = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 libssh-rs = { workspace = true }
+socket2 = { workspace = true }
 httparse = { workspace = true }
 snailquote = "0.3.1"
 path-slash = "0.2.1"