diff --git a/embassy/src/embassy_channel.rs b/embassy/src/embassy_channel.rs index 890cfe786b2b8e58b2b758560ef81ce4d308f711..735e10b1ebf2df05909ffffd8e4192214f485121 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 ca9a3517b55db08214949611cc455d5f502cf6cc..5dab90f9c8833ae81d4f7a9750c71d94efa590b3 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 ac05b25ddf3cc24f5677e17b3defdb6709f18769..352044d24476766e3f76f65670c0ef53c320fbdc 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 389dbc0e784b5727e99572fc6476fcd5a5760ae2..700d8ed5ec230558a4d015c124838a4e18e4c596 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 9a0a945157255112f97aed637f33997b71b6718e..363598d29c795a97ae2c6c127987bf8fb75da78b 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.