From af2404686e149437200df4154de95bf29aa0e111 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sun, 21 May 2023 11:45:27 +0800
Subject: [PATCH] Better comments and delete some dead code
---
embassy/src/embassy_channel.rs | 3 +++
embassy/src/embassy_sunset.rs | 11 +++++++----
src/channel.rs | 28 ++++++----------------------
src/config.rs | 11 ++++-------
src/runner.rs | 20 ++++----------------
5 files changed, 24 insertions(+), 49 deletions(-)
diff --git a/embassy/src/embassy_channel.rs b/embassy/src/embassy_channel.rs
index 890cfe7..735e10b 100644
--- a/embassy/src/embassy_channel.rs
+++ b/embassy/src/embassy_channel.rs
@@ -1,4 +1,7 @@
//! Presents SSH channels as async
+//!
+//! Most code in this module is a convenience wrapper for methods in
+//! [embassy_sunset].
#[allow(unused_imports)]
use log::{debug, error, info, log, trace, warn};
diff --git a/embassy/src/embassy_sunset.rs b/embassy/src/embassy_sunset.rs
index ca9a351..5dab90f 100644
--- a/embassy/src/embassy_sunset.rs
+++ b/embassy/src/embassy_sunset.rs
@@ -51,7 +51,8 @@ struct Inner<'a, C: CliBehaviour, S: ServBehaviour> {
wakers: Wakers,
- // May only be safely modified when `chan_refcounts` is zero.
+ // May only be safely modified when the corresponding
+ // `chan_refcounts` is zero.
chan_handles: [Option<ChanHandle>; MAX_CHANNELS],
}
@@ -268,9 +269,10 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> EmbassySunset<'a, C, S> {
/// Check for channels that have reached zero refcount
///
- /// This runs periodically from an async context to clean up channels
- /// that were refcounted down to 0 called from an async context (when
- /// a ChanIO is dropped)
+ /// When a ChanIO is dropped the refcount may reach 0, but
+ /// without "async Drop" it isn't possible to take the `inner` lock during
+ /// `drop()`.
+ /// Instead this runs periodically from an async context to release channels.
fn clear_refcounts(&self, inner: &mut Inner<C, S>) -> Result<()> {
for (ch, count) in inner.chan_handles.iter_mut().zip(self.chan_refcounts.iter()) {
let count = count.load(Relaxed);
@@ -503,6 +505,7 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> EmbassySunset<'a, C, S> {
}
pub(crate) fn dec_chan(&self, num: ChanNum) {
+ // refcounts that hit zero will be cleaned up later in clear_refcounts()
let c = self.chan_refcounts[num.0 as usize].fetch_sub(1, SeqCst);
debug_assert_ne!(c, 0);
// perhaps not necessary? is cheap?
diff --git a/src/channel.rs b/src/channel.rs
index ac05b25..352044d 100644
--- a/src/channel.rs
+++ b/src/channel.rs
@@ -22,6 +22,11 @@ use runner::ChanHandle;
use snafu::ErrorCompat;
+pub(crate) struct Channels<C: CliBehaviour, S: ServBehaviour> {
+ ch: [Option<Channel>; config::MAX_CHANNELS],
+ _ph: (PhantomData<C>, PhantomData<S>),
+}
+
impl<C: CliBehaviour, S: ServBehaviour> Channels<C, S> {
pub fn new() -> Self {
Channels {
@@ -478,8 +483,6 @@ impl TryFrom<&packets::PtyReq<'_>> for Pty {
})
}
}
-pub(crate) type ExecString = heapless::String<MAX_EXEC>;
-
/// Like a `packets::ChannelReqType` but with storage.
/// Lifetime-free variants have the packet part directly.
#[derive(Debug)]
@@ -551,20 +554,6 @@ impl<'a, S: AsRef<str> + 'a> Into<Req<'a>> for &'a SessionCommand<S> {
}
}
-
-// // Variants match packets::ChannelReqType, without data
-// enum ReqKind {
-// Shell,
-// Exec,
-// Pty,
-// Subsystem,
-// WinChange,
-// Signal,
-// ExitStatus,
-// ExitSignal,
-// Break,
-// }
-
/// Per-direction channel variables
#[derive(Debug)]
struct ChanDir {
@@ -626,6 +615,7 @@ impl Channel {
// last_req: Deque::new(),
recv: ChanDir {
num: num.0,
+ // TODO these should depend on SSH rx buffer size minus overhead
max_packet: config::DEFAULT_MAX_PACKET,
window: config::DEFAULT_WINDOW,
},
@@ -850,12 +840,6 @@ pub enum ChanOpened {
Failure((ChanFail, ChanHandle))
}
-pub(crate) struct Channels<C: CliBehaviour, S: ServBehaviour> {
- ch: [Option<Channel>; config::MAX_CHANNELS],
-
- _ph: (PhantomData<C>, PhantomData<S>),
-}
-
/// A SSH protocol local channel number
///
/// The number will always be in the range `0 <= num < MAX_CHANNELS`
diff --git a/src/config.rs b/src/config.rs
index 389dbc0..700d8ed 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -2,16 +2,13 @@
pub const DEFAULT_WINDOW: usize = 1000;
pub const DEFAULT_MAX_PACKET: usize = 1000;
-// TODO: perhaps this is a parameter, the Channel Vec storage type itself is a parameter
-// and boundless for alloc.
-
-// This can be increased arbitrarily, though note that some code paths assume
+// TODO: Perhaps instead of MAX_CHANNELS we could have a type alias
+// of either heapless::Vec<> or std::vec::Vec<>
+//
+// This size is arbitrary and may be increased, though note that some code paths assume
// a linear scan of channels can happen quickly, so may need reworking for performance.
pub const MAX_CHANNELS: usize = 4;
-// TODO
-pub const MAX_EXEC: usize = 200;
-
// Enough for longest 23 of "screen.konsole-256color" on my system
// Unsure if this is specified somewhere
pub const MAX_TERM: usize = 32;
diff --git a/src/runner.rs b/src/runner.rs
index 9a0a945..363598d 100644
--- a/src/runner.rs
+++ b/src/runner.rs
@@ -212,22 +212,6 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> Runner<'a, C, S> {
// TODO: move somewhere client specific?
pub fn open_client_session(&mut self) -> Result<ChanHandle> {
trace!("open_client_session");
- // let mut init_req = channel::InitReqs::new();
- // if let Some(pty) = pty {
- // init_req.push(channel::ReqDetails::Pty(pty)).trap()?;
- // }
-
- // match cmd {
- // SessionCommand::Shell => {
- // init_req.push(channel::ReqDetails::Shell).trap()?;
- // }
- // }
- // if let Some(cmd) = exec {
- // let mut s = channel::ExecString::new();
- // s.push_str(cmd).trap()?;
- // init_req.push(channel::ReqDetails::Exec(s)).trap()?;
- // } else {
- // }
let (chan, p) = self.conn.channels.open(packets::ChannelOpenType::Session)?;
self.traf_out.send_packet(p, &mut self.keys)?;
@@ -423,6 +407,9 @@ impl<'a, C: CliBehaviour, S: ServBehaviour> Runner<'a, C, S> {
/// Represents an open channel, owned by the application.
///
/// Must be released by calling [`Runner::channel_done()`]
+
+// Inner contents are crate-private to ensure that arbitrary
+// channel numbers cannot be used after closing/reuse.
pub struct ChanHandle(pub(crate) ChanNum);
impl ChanHandle {
@@ -431,6 +418,7 @@ impl ChanHandle {
/// This can be used by applications as an index.
/// Channel numbers satisfy
/// `0 <= num < sunset::config::MAX_CHANNELS`.
+ ///
/// An index may be reused after a call to [`Runner::channel_done()`],
/// applications must take care not to keep using this `num()` index after
/// that.
--
GitLab