From fa413e20ab7871d4d3d0f3a3fde1d65531008bd6 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 30 Apr 2026 11:05:13 -0500 Subject: [PATCH 1/5] feat(server)!: introduce typed ServerError on the public API boundary Introduces a typed error story for ironrdp-server, mirroring the shape already used by ironrdp-connector and the rest of the connection-management layer: pub type ServerError = ironrdp_error::Error; pub type ServerResult = Result; ServerErrorKind is a non-exhaustive enum with concretely typed variants for the failure modes the crate surfaces (Encode, Decode, Io, Channel, Unsupported, Reason, General, Custom). Sources are attached through ironrdp_error::Error::with_source rather than embedded as Box in variant data, matching ConnectorErrorKind exactly. This first PR converts public boundaries only: - RdpServer::run -> ServerResult<()> - RdpServer::run_connection -> ServerResult<()> - TlsIdentityCtx::init_from_paths -> ServerResult - TlsIdentityCtx::make_acceptor -> ServerResult - EchoServerHandle::send_request -> ServerResult<()> Internal call sites continue to use anyhow::Result during the staged migration. A private helper bridges at the public boundary. A follow-up PR converts internal sites to use the typed kinds directly and drops the anyhow dependency. A second follow-up changes the ConnectionHandler::on_disconnected error parameter from Option<&anyhow::Error> to Option<&ServerError>. This is a breaking change to consumers of the listed public functions: return types change from anyhow::Result to ServerResult. Tracking: #1209 --- Cargo.lock | 1 + crates/ironrdp-server/Cargo.toml | 3 +- crates/ironrdp-server/src/echo.rs | 9 +- crates/ironrdp-server/src/error.rs | 209 ++++++++++++++++++++++++++++ crates/ironrdp-server/src/helper.rs | 14 +- crates/ironrdp-server/src/lib.rs | 2 + crates/ironrdp-server/src/server.rs | 22 ++- crates/ironrdp/examples/server.rs | 2 +- 8 files changed, 251 insertions(+), 11 deletions(-) create mode 100644 crates/ironrdp-server/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index 209e53361..876422715 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2852,6 +2852,7 @@ dependencies = [ "ironrdp-dvc", "ironrdp-echo", "ironrdp-egfx", + "ironrdp-error", "ironrdp-graphics", "ironrdp-nscodec", "ironrdp-pdu", diff --git a/crates/ironrdp-server/Cargo.toml b/crates/ironrdp-server/Cargo.toml index c01b60668..1be7d6105 100644 --- a/crates/ironrdp-server/Cargo.toml +++ b/crates/ironrdp-server/Cargo.toml @@ -39,8 +39,9 @@ tokio-rustls = "0.26" # public async-trait = "0.1" ironrdp-async = { path = "../ironrdp-async", version = "0.9" } ironrdp-ainput = { path = "../ironrdp-ainput", version = "0.7" } -ironrdp-core = { path = "../ironrdp-core", version = "0.2" } +ironrdp-core = { path = "../ironrdp-core", version = "0.2" } # public ironrdp-egfx = { path = "../ironrdp-egfx", version = "0.2", optional = true } +ironrdp-error = { path = "../ironrdp-error", version = "0.2", features = ["std"] } # public ironrdp-nscodec = { path = "../ironrdp-nscodec", version = "0.1", optional = true, features = ["encoder"] } ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.8" } # public ironrdp-svc = { path = "../ironrdp-svc", version = "0.7" } # public diff --git a/crates/ironrdp-server/src/echo.rs b/crates/ironrdp-server/src/echo.rs index 8caf69b3d..27b6fe1a1 100644 --- a/crates/ironrdp-server/src/echo.rs +++ b/crates/ironrdp-server/src/echo.rs @@ -3,13 +3,14 @@ use std::collections::{BTreeMap, VecDeque}; use std::sync::{Arc, Mutex, MutexGuard}; use std::time::Instant; -use anyhow::{Context as _, Result, bail}; +use anyhow::{Context as _, Result}; use ironrdp_core::impl_as_any; use ironrdp_dvc::{DvcMessage, DvcProcessor, DvcServerProcessor}; use ironrdp_echo::server::EchoServer; use ironrdp_pdu::PduResult; use tokio::sync::mpsc; +use crate::error::{ServerError, ServerErrorExt as _, ServerResult}; use crate::server::ServerEvent; #[derive(Debug, Clone)] @@ -55,14 +56,14 @@ impl EchoServerHandle { /// Sends a runtime ECHO request. /// /// The payload must be at least one byte, as required by MS-RDPEECO section 3.1.5.1. - pub fn send_request(&self, payload: Vec) -> Result<()> { + pub fn send_request(&self, payload: Vec) -> ServerResult<()> { if payload.is_empty() { - bail!("echoRequest payload must be at least one byte"); + return Err(ServerError::reason("echo request", "payload must be at least one byte")); } self.sender .send(ServerEvent::Echo(EchoServerMessage::SendRequest { payload })) - .map_err(|_error| anyhow::anyhow!("send ECHO request event")) + .map_err(|_error| ServerError::channel("send ECHO request event")) } /// Drains collected RTT measurements. diff --git a/crates/ironrdp-server/src/error.rs b/crates/ironrdp-server/src/error.rs new file mode 100644 index 000000000..24354feb2 --- /dev/null +++ b/crates/ironrdp-server/src/error.rs @@ -0,0 +1,209 @@ +//! Typed error types for the public API of the `ironrdp-server` crate. +//! +//! Mirrors the shape of [`ironrdp_connector::ConnectorError`]: a thin +//! [`ironrdp_error::Error`] wrapper around a typed [`ServerErrorKind`] enum, +//! with a static `&'static str` context and an opaque `source` for arbitrary +//! upstream errors. The wrapper provides `with_source` so concrete errors +//! from consumer-supplied components can be attached without forcing the +//! variant taxonomy to encode every possible source type. +//! +//! See [#1209] for the migration discussion. +//! +//! [#1209]: https://github.com/Devolutions/IronRDP/issues/1209 + +use core::fmt; +use std::io; + +use ironrdp_core::{DecodeError, EncodeError}; + +/// Categorizes the failure modes the server crate exposes through +/// [`ServerError`]. +/// +/// Marked `#[non_exhaustive]` so additional variants do not constitute a +/// breaking change. +#[non_exhaustive] +#[derive(Debug)] +pub enum ServerErrorKind { + /// PDU encoding failed. + Encode(EncodeError), + /// PDU decoding failed. + Decode(DecodeError), + /// I/O error during TLS setup, listener setup, or client communication. + Io(io::Error), + /// A required virtual channel was missing or a channel send failed. + /// The specific channel and failure mode is named in the [`ServerError`] + /// context. + Channel, + /// A feature requested by the client is not supported by this server. + /// The specific feature is named in the [`ServerError`] context. + Unsupported, + /// Generic failure with a runtime description. Prefer a specific variant. + Reason(String), + /// Catch-all with no specific cause. Prefer a specific variant. + General, + /// Custom failure with the actual source attached via + /// [`ironrdp_error::Error::with_source`]. + Custom, +} + +impl fmt::Display for ServerErrorKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Encode(_) => write!(f, "encode error"), + Self::Decode(_) => write!(f, "decode error"), + Self::Io(_) => write!(f, "I/O error"), + Self::Channel => write!(f, "channel error"), + Self::Unsupported => write!(f, "unsupported"), + Self::Reason(reason) => write!(f, "reason: {reason}"), + Self::General => write!(f, "general error"), + Self::Custom => write!(f, "custom error"), + } + } +} + +impl core::error::Error for ServerErrorKind { + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { + match self { + Self::Encode(e) => Some(e), + Self::Decode(e) => Some(e), + Self::Io(e) => Some(e), + Self::Channel | Self::Unsupported | Self::Reason(_) | Self::General | Self::Custom => None, + } + } +} + +/// Server-side failure type. +/// +/// A typed alias of [`ironrdp_error::Error`] specialized to +/// [`ServerErrorKind`]. The wrapper adds a static `&'static str` context and +/// an optional opaque `source` to whichever kind of failure occurred. +pub type ServerError = ironrdp_error::Error; + +/// Convenience alias for `Result`. +pub type ServerResult = Result; + +/// Constructors for [`ServerError`] that match the shape of +/// [`ironrdp_connector::ConnectorErrorExt`]. +pub trait ServerErrorExt { + /// Build a [`ServerErrorKind::Encode`] error from an [`EncodeError`]. + fn encode(error: EncodeError) -> Self; + /// Build a [`ServerErrorKind::Decode`] error from a [`DecodeError`]. + fn decode(error: DecodeError) -> Self; + /// Build a [`ServerErrorKind::Io`] error with a static context and an + /// [`io::Error`] source. + fn io(context: &'static str, error: io::Error) -> Self; + /// Build a [`ServerErrorKind::Channel`] error with the channel name or + /// failure description carried in the context. + fn channel(context: &'static str) -> Self; + /// Build a [`ServerErrorKind::Unsupported`] error with the unsupported + /// feature named in the context. + fn unsupported(context: &'static str) -> Self; + /// Build a [`ServerErrorKind::General`] error with a static context. + fn general(context: &'static str) -> Self; + /// Build a [`ServerErrorKind::Reason`] error with a static context and a + /// runtime description. + fn reason(context: &'static str, reason: impl Into) -> Self; + /// Build a [`ServerErrorKind::Custom`] error with a static context and an + /// arbitrary source. + fn custom(context: &'static str, error: E) -> Self + where + E: core::error::Error + Sync + Send + 'static; +} + +impl ServerErrorExt for ServerError { + fn encode(error: EncodeError) -> Self { + Self::new("encode error", ServerErrorKind::Encode(error)) + } + + fn decode(error: DecodeError) -> Self { + Self::new("decode error", ServerErrorKind::Decode(error)) + } + + fn io(context: &'static str, error: io::Error) -> Self { + Self::new(context, ServerErrorKind::Io(error)) + } + + fn channel(context: &'static str) -> Self { + Self::new(context, ServerErrorKind::Channel) + } + + fn unsupported(context: &'static str) -> Self { + Self::new(context, ServerErrorKind::Unsupported) + } + + fn general(context: &'static str) -> Self { + Self::new(context, ServerErrorKind::General) + } + + fn reason(context: &'static str, reason: impl Into) -> Self { + Self::new(context, ServerErrorKind::Reason(reason.into())) + } + + fn custom(context: &'static str, error: E) -> Self + where + E: core::error::Error + Sync + Send + 'static, + { + Self::new(context, ServerErrorKind::Custom).with_source(error) + } +} + +/// Result-side helpers mirroring [`ironrdp_connector::ConnectorResultExt`]. +pub trait ServerResultExt { + /// Replace the `&'static str` context on any error in `Self`. + #[must_use] + fn with_context(self, context: &'static str) -> Self; + /// Attach a source to any error in `Self`. + #[must_use] + fn with_source(self, source: E) -> Self + where + E: core::error::Error + Sync + Send + 'static; +} + +impl ServerResultExt for ServerResult { + fn with_context(self, context: &'static str) -> Self { + self.map_err(|mut e| { + e.set_context(context); + e + }) + } + + fn with_source(self, source: E) -> Self + where + E: core::error::Error + Sync + Send + 'static, + { + self.map_err(|e| e.with_source(source)) + } +} + +/// Bridges anyhow errors at the public API boundary while the migration to +/// typed errors is staged. +/// +/// Internal call sites still use `anyhow::Result`; conversion happens here so +/// the public signatures can advertise [`ServerResult`] today without forcing +/// every internal site to convert in this PR. PR #2 in the staged migration +/// (see [#1209]) removes the remaining `anyhow` usage and this helper. +/// +/// [#1209]: https://github.com/Devolutions/IronRDP/issues/1209 +pub(crate) fn from_anyhow(error: anyhow::Error) -> ServerError { + ServerError::new("server error", ServerErrorKind::Custom).with_source(AnyhowError(error)) +} + +/// Newtype wrapper that gives [`anyhow::Error`] a `core::error::Error` impl +/// suitable for `ironrdp_error::Error::with_source`. +#[derive(Debug)] +struct AnyhowError(anyhow::Error); + +impl fmt::Display for AnyhowError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#}", self.0) + } +} + +impl core::error::Error for AnyhowError { + /// Forwards to the wrapped [`anyhow::Error`]'s cause chain so callers + /// traversing [`core::error::Error::source`] reach the original root + /// cause rather than stopping at this newtype. + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { + self.0.source() + } +} diff --git a/crates/ironrdp-server/src/helper.rs b/crates/ironrdp-server/src/helper.rs index 6d4055715..84a18a7e4 100644 --- a/crates/ironrdp-server/src/helper.rs +++ b/crates/ironrdp-server/src/helper.rs @@ -9,6 +9,8 @@ use tokio_rustls::rustls::pki_types::pem::PemObject as _; use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer}; use tokio_rustls::{TlsAcceptor, rustls}; +use crate::error::{ServerResult, from_anyhow}; + pub struct TlsIdentityCtx { pub certs: Vec>, pub priv_key: PrivateKeyDer<'static>, @@ -19,7 +21,11 @@ impl TlsIdentityCtx { /// A constructor to create a `TlsIdentityCtx` from the given certificate and key paths. /// /// The file format can be either PEM (if the file extension ends with .pem) or DER. - pub fn init_from_paths(cert_path: &Path, key_path: &Path) -> anyhow::Result { + pub fn init_from_paths(cert_path: &Path, key_path: &Path) -> ServerResult { + Self::init_from_paths_inner(cert_path, key_path).map_err(from_anyhow) + } + + fn init_from_paths_inner(cert_path: &Path, key_path: &Path) -> anyhow::Result { let certs = if cert_path.extension().is_some_and(|ext| ext == "pem") { CertificateDer::pem_file_iter(cert_path) .with_context(|| format!("reading server cert `{cert_path:?}`"))? @@ -62,7 +68,11 @@ impl TlsIdentityCtx { }) } - pub fn make_acceptor(&self) -> anyhow::Result { + pub fn make_acceptor(&self) -> ServerResult { + self.make_acceptor_inner().map_err(from_anyhow) + } + + fn make_acceptor_inner(&self) -> anyhow::Result { let mut server_config = rustls::ServerConfig::builder() .with_no_client_auth() .with_single_cert(self.certs.clone(), self.priv_key.clone_key()) diff --git a/crates/ironrdp-server/src/lib.rs b/crates/ironrdp-server/src/lib.rs index 4ae6b7167..7f422fd61 100644 --- a/crates/ironrdp-server/src/lib.rs +++ b/crates/ironrdp-server/src/lib.rs @@ -13,6 +13,7 @@ mod clipboard; mod display; mod echo; mod encoder; +mod error; #[cfg(feature = "egfx")] mod gfx; mod handler; @@ -27,6 +28,7 @@ pub use display::{ RdpServerDisplayUpdates, }; pub use echo::{EchoDvcBridge, EchoRoundTripMeasurement, EchoServerHandle, EchoServerMessage}; +pub use error::{ServerError, ServerErrorExt, ServerErrorKind, ServerResult, ServerResultExt}; #[cfg(feature = "egfx")] pub use gfx::{EgfxServerMessage, GfxDvcBridge, GfxServerFactory, GfxServerHandle}; pub use handler::{KeyboardEvent, MouseEvent, RdpServerInputHandler}; diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 0be2a0707..b29dc1625 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -39,6 +39,7 @@ use crate::clipboard::CliprdrServerFactory; use crate::display::{DisplayUpdate, RdpServerDisplay}; use crate::echo::{EchoDvcBridge, EchoServerHandle, EchoServerMessage, build_echo_request}; use crate::encoder::{UpdateEncoder, UpdateEncoderCodecs}; +use crate::error::{ServerResult, from_anyhow}; #[cfg(feature = "egfx")] use crate::gfx::{EgfxServerMessage, GfxServerFactory}; use crate::handler::RdpServerInputHandler; @@ -671,7 +672,14 @@ impl RdpServer { acceptor.attach_static_channel(dvc); } - pub async fn run_connection(&mut self, stream: S) -> Result<()> + pub async fn run_connection(&mut self, stream: S) -> ServerResult<()> + where + S: AsyncRead + AsyncWrite + Send + Sync + Unpin, + { + self.run_connection_inner(stream).await.map_err(from_anyhow) + } + + async fn run_connection_inner(&mut self, stream: S) -> Result<()> where S: AsyncRead + AsyncWrite + Send + Sync + Unpin, { @@ -750,7 +758,11 @@ impl RdpServer { Ok(()) } - pub async fn run(&mut self) -> Result<()> { + pub async fn run(&mut self) -> ServerResult<()> { + self.run_inner().await.map_err(from_anyhow) + } + + async fn run_inner(&mut self) -> Result<()> { // Create socket with control over options before binding. // Using TcpSocket instead of TcpListener::bind() allows setting // SO_REUSEADDR and IPv6 dual-stack mode. @@ -816,7 +828,11 @@ impl RdpServer { drop(stream); } else { let started = tokio::time::Instant::now(); - let result = self.run_connection(stream).await; + // Internal accept loop uses the anyhow-returning inner method so the + // existing `on_disconnected(error: Option<&anyhow::Error>)` parameter + // continues to receive an anyhow value. Migration of that parameter + // to ServerError is tracked as a follow-up PR per #1209. + let result = self.run_connection_inner(stream).await; let duration = started.elapsed(); if let Err(ref error) = result { diff --git a/crates/ironrdp/examples/server.rs b/crates/ironrdp/examples/server.rs index 71db6d021..30a199b6a 100644 --- a/crates/ironrdp/examples/server.rs +++ b/crates/ironrdp/examples/server.rs @@ -437,5 +437,5 @@ async fn run( domain: None, })); - server.run().await + server.run().await.map_err(|e| anyhow::anyhow!(e)) } From f2ff13a8bb5f64413947451852dbb1ca55ebf427 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 30 Apr 2026 11:17:09 -0500 Subject: [PATCH 2/5] feat(server): convert encoder/helper/echo internals from anyhow to ServerError Second step of the staged migration started in the previous commit. Replaces anyhow construction sites in modules whose internal flow does not pass through the ConnectionHandler::on_disconnected callback (which still takes &anyhow::Error and is the subject of a separate follow-up PR per #1209): - encoder/mod.rs: ~15 anyhow! / .context() / bail! sites converted to typed ServerError variants (Encode, Reason, Custom). EncodeError sources go through ServerError::encode (matching ConnectorErrorExt::encode). spawn_blocking JoinError, qoi codec errors, and zstd error codes go through ServerError::custom or ServerError::reason as appropriate. - helper.rs: TLS cert/key loading paths construct ServerError::io for std::io::Error sources, ServerError::reason for missing-key cases, ServerError::custom for x509-cert and PEM parsing errors. Removes the from_anyhow bridge and the inner-fn split introduced in the previous commit. - echo.rs: build_echo_request returns ServerResult, builds errors via ServerError::custom directly. send_request keeps its already- typed Channel and Reason variants. server.rs internals stay on anyhow because they propagate into ConnectionHandler::on_disconnected which still takes &anyhow::Error; that conversion plus the remaining server.rs internal sites land in PR #3. The anyhow dep stays in Cargo.toml for now and will be dropped when the public traits (ConnectionHandler, RdpServerDisplay, and RdpServerDisplayUpdates) finish their typed migration. --- crates/ironrdp-server/src/echo.rs | 5 +- crates/ironrdp-server/src/encoder/mod.rs | 157 ++++++++++++++++------- crates/ironrdp-server/src/helper.rs | 43 +++---- 3 files changed, 130 insertions(+), 75 deletions(-) diff --git a/crates/ironrdp-server/src/echo.rs b/crates/ironrdp-server/src/echo.rs index 27b6fe1a1..513b5a590 100644 --- a/crates/ironrdp-server/src/echo.rs +++ b/crates/ironrdp-server/src/echo.rs @@ -3,7 +3,6 @@ use std::collections::{BTreeMap, VecDeque}; use std::sync::{Arc, Mutex, MutexGuard}; use std::time::Instant; -use anyhow::{Context as _, Result}; use ironrdp_core::impl_as_any; use ironrdp_dvc::{DvcMessage, DvcProcessor, DvcServerProcessor}; use ironrdp_echo::server::EchoServer; @@ -152,6 +151,6 @@ impl DvcProcessor for EchoDvcBridge { impl DvcServerProcessor for EchoDvcBridge {} -pub(crate) fn build_echo_request(payload: Vec) -> Result { - EchoServer::request_message(payload).context("build ECHO request message") +pub(crate) fn build_echo_request(payload: Vec) -> ServerResult { + EchoServer::request_message(payload).map_err(|e| ServerError::custom("build ECHO request message", e)) } diff --git a/crates/ironrdp-server/src/encoder/mod.rs b/crates/ironrdp-server/src/encoder/mod.rs index d3f83c9e9..c41a45a07 100644 --- a/crates/ironrdp-server/src/encoder/mod.rs +++ b/crates/ironrdp-server/src/encoder/mod.rs @@ -1,7 +1,7 @@ use core::fmt; use core::num::NonZeroU16; -use anyhow::{Context as _, Result, anyhow}; +use crate::error::{ServerError, ServerErrorExt as _, ServerResult}; use ironrdp_acceptor::DesktopSize; use ironrdp_graphics::diff::{Rect, find_different_rects_sub}; use ironrdp_pdu::encode_vec; @@ -128,13 +128,13 @@ impl UpdateEncoder { surface_flags: CmdFlags, codecs: UpdateEncoderCodecs, max_request_size: u32, - ) -> Result { + ) -> ServerResult { let bitmap_updater = if surface_flags.contains(CmdFlags::SET_SURFACE_BITS) { match codecs { #[cfg(feature = "qoiz")] - UpdateEncoderCodecs { qoiz: Some(id), .. } => { - BitmapUpdater::Qoiz(QoizHandler::new(id).context("failed to initialize qoiz handler")?) - } + UpdateEncoderCodecs { qoiz: Some(id), .. } => BitmapUpdater::Qoiz( + QoizHandler::new(id).map_err(|e| ServerError::custom("failed to initialize qoiz handler", e))?, + ), #[cfg(feature = "qoi")] UpdateEncoderCodecs { qoi: Some(id), .. } => BitmapUpdater::Qoi(QoiHandler::new(id)), UpdateEncoderCodecs { @@ -162,7 +162,8 @@ impl UpdateEncoder { desktop_size, framebuffer: None, bitmap_updater: Some(bitmap_updater), - max_request_size: usize::try_from(max_request_size).context("max_request_size")?, + max_request_size: usize::try_from(max_request_size) + .map_err(|e| ServerError::custom("max_request_size", e))?, }) } @@ -182,7 +183,7 @@ impl UpdateEncoder { .set_desktop_size(size); } - fn rgba_pointer(ptr: RGBAPointer) -> Result { + fn rgba_pointer(ptr: RGBAPointer) -> ServerResult { let xor_mask = ptr.data; let hot_spot = Point16 { @@ -201,10 +202,13 @@ impl UpdateEncoder { xor_bpp: 32, color_pointer, }; - Ok(UpdateFragmenter::new(UpdateCode::NewPointer, encode_vec(&ptr)?)) + Ok(UpdateFragmenter::new( + UpdateCode::NewPointer, + encode_vec(&ptr).map_err(ServerError::encode)?, + )) } - fn color_pointer(ptr: ColorPointer) -> Result { + fn color_pointer(ptr: ColorPointer) -> ServerResult { let hot_spot = Point16 { x: ptr.hot_x, y: ptr.hot_y, @@ -217,24 +221,33 @@ impl UpdateEncoder { xor_mask: &ptr.xor_mask, and_mask: &ptr.and_mask, }; - Ok(UpdateFragmenter::new(UpdateCode::ColorPointer, encode_vec(&ptr)?)) + Ok(UpdateFragmenter::new( + UpdateCode::ColorPointer, + encode_vec(&ptr).map_err(ServerError::encode)?, + )) } - fn cached_pointer(cache_index: u16) -> Result { + fn cached_pointer(cache_index: u16) -> ServerResult { let ptr = CachedPointerAttribute { cache_index }; - Ok(UpdateFragmenter::new(UpdateCode::CachedPointer, encode_vec(&ptr)?)) + Ok(UpdateFragmenter::new( + UpdateCode::CachedPointer, + encode_vec(&ptr).map_err(ServerError::encode)?, + )) } - fn default_pointer() -> Result { + fn default_pointer() -> ServerResult { Ok(UpdateFragmenter::new(UpdateCode::DefaultPointer, vec![])) } - fn hide_pointer() -> Result { + fn hide_pointer() -> ServerResult { Ok(UpdateFragmenter::new(UpdateCode::HiddenPointer, vec![])) } - fn pointer_position(pos: PointerPositionAttribute) -> Result { - Ok(UpdateFragmenter::new(UpdateCode::PositionPointer, encode_vec(&pos)?)) + fn pointer_position(pos: PointerPositionAttribute) -> ServerResult { + Ok(UpdateFragmenter::new( + UpdateCode::PositionPointer, + encode_vec(&pos).map_err(ServerError::encode)?, + )) } fn bitmap_diffs(&mut self, bitmap: &BitmapUpdate) -> Vec { @@ -330,7 +343,7 @@ impl UpdateEncoder { } } - async fn bitmap(&mut self, bitmap: BitmapUpdate) -> Result { + async fn bitmap(&mut self, bitmap: BitmapUpdate) -> ServerResult { // Move the bitmap updater to satisfy spawn_blocking 'static requirement. // It is restored after the blocking operation completes. let mut updater = self.bitmap_updater.take().expect("bitmap updater always Some"); @@ -339,7 +352,8 @@ impl UpdateEncoder { let result = time_warn!("Encoding bitmap", 10, updater.handle(&bitmap)); (result, updater) }) - .await?; + .await + .map_err(|e| ServerError::custom("bitmap encoder task", e))?; self.bitmap_updater = Some(updater); @@ -367,7 +381,7 @@ pub(crate) struct EncoderIter<'a> { impl EncoderIter<'_> { #[cfg_attr(feature = "__bench", visibility::make(pub))] - pub(crate) async fn next(&mut self) -> Option> { + pub(crate) async fn next(&mut self) -> Option> { loop { let state = core::mem::take(&mut self.state); let encoder = &mut self.encoder; @@ -406,25 +420,55 @@ impl EncoderIter<'_> { let x = match u16::try_from(x) { Ok(x) => x, - Err(_) => return Some(Err(anyhow!("invalid `x`: out of range integral conversion"))), + Err(_) => { + return Some(Err(ServerError::reason( + "bitmap diff", + "invalid `x`: out of range integral conversion", + ))); + } }; let y = match u16::try_from(y) { Ok(y) => y, - Err(_) => return Some(Err(anyhow!("invalid `y`: out of range integral conversion"))), + Err(_) => { + return Some(Err(ServerError::reason( + "bitmap diff", + "invalid `y`: out of range integral conversion", + ))); + } }; let width = match u16::try_from(width) { Ok(width) => match NonZeroU16::new(width) { Some(width) => width, - None => return Some(Err(anyhow!("rectangle width cannot be zero"))), + None => { + return Some(Err(ServerError::reason( + "bitmap diff", + "rectangle width cannot be zero", + ))); + } }, - Err(_) => return Some(Err(anyhow!("invalid `width`: out of range integral conversion"))), + Err(_) => { + return Some(Err(ServerError::reason( + "bitmap diff", + "invalid `width`: out of range integral conversion", + ))); + } }; let height = match u16::try_from(height) { Ok(height) => match NonZeroU16::new(height) { Some(height) => height, - None => return Some(Err(anyhow!("rectangle height cannot be zero"))), + None => { + return Some(Err(ServerError::reason( + "bitmap diff", + "rectangle height cannot be zero", + ))); + } }, - Err(_) => return Some(Err(anyhow!("invalid `height`: out of range integral conversion"))), + Err(_) => { + return Some(Err(ServerError::reason( + "bitmap diff", + "invalid `height`: out of range integral conversion", + ))); + } }; let Some(sub) = bitmap.sub(x, y, width, height) else { @@ -460,7 +504,7 @@ enum BitmapUpdater { } impl BitmapUpdater { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { match self { Self::None(up) => up.handle(bitmap), Self::Bitmap(up) => up.handle(bitmap), @@ -482,14 +526,14 @@ impl BitmapUpdater { } trait BitmapUpdateHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result; + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult; } #[derive(Clone, Debug)] struct NoneHandler; impl BitmapUpdateHandler for NoneHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let stride = usize::from(bitmap.format.bytes_per_pixel()) * usize::from(bitmap.width.get()); let mut data = Vec::with_capacity(stride * usize::from(bitmap.height.get())); for row in bitmap.data.chunks(bitmap.stride.get()).rev() { @@ -519,7 +563,7 @@ impl BitmapHandler { } impl BitmapUpdateHandler for BitmapHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let mut buffer = vec![0; bitmap.data.len() * 2]; // TODO: estimate bitmap encoded size let len = loop { match self.bitmap.encode(bitmap, buffer.as_mut_slice()) { @@ -529,9 +573,9 @@ impl BitmapUpdateHandler for BitmapHandler { buffer.resize(buffer.len() * 2, 0); debug!("encoder buffer resized to: {}", buffer.len() * 2); } - _ => Err(e).context("bitmap encode error")?, + _ => return Err(ServerError::encode(e)), }, - BitmapEncodeError::Rle(e) => Err(e).context("bitmap RLE encode error")?, + BitmapEncodeError::Rle(e) => return Err(ServerError::custom("bitmap RLE encode error", e)), }, Ok(len) => break len, } @@ -564,7 +608,7 @@ impl RemoteFxHandler { } impl BitmapUpdateHandler for RemoteFxHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let mut buffer = vec![0; bitmap.data.len()]; let len = loop { match self @@ -576,7 +620,7 @@ impl BitmapUpdateHandler for RemoteFxHandler { buffer.resize(buffer.len() * 2, 0); debug!("encoder buffer resized to: {}", buffer.len() * 2); } - _ => Err(e).context("RemoteFX encode error")?, + _ => return Err(ServerError::encode(e)), }, Ok(len) => break len, } @@ -601,7 +645,7 @@ impl QoiHandler { #[cfg(feature = "qoi")] impl BitmapUpdateHandler for QoiHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let data = qoi_encode(bitmap)?; set_surface(bitmap, self.codec_id, &data) } @@ -622,23 +666,29 @@ impl fmt::Debug for QoizHandler { #[cfg(feature = "qoiz")] impl QoizHandler { - fn new(codec_id: u8) -> Result { + fn new(codec_id: u8) -> ServerResult { let mut zctxt = zstd_safe::CCtx::default(); zctxt .set_parameter(zstd_safe::CParameter::CompressionLevel(3)) .map_err(|code| { - anyhow!( - "failed to set zstd compression level: {}", - zstd_safe::get_error_name(code) + ServerError::reason( + "qoiz init", + format!( + "failed to set zstd compression level: {}", + zstd_safe::get_error_name(code) + ), ) })?; zctxt .set_parameter(zstd_safe::CParameter::EnableLongDistanceMatching(true)) .map_err(|code| { - anyhow!( - "failed to set zstd enable long distance matching: {}", - zstd_safe::get_error_name(code) + ServerError::reason( + "qoiz init", + format!( + "failed to set zstd enable long distance matching: {}", + zstd_safe::get_error_name(code) + ), ) })?; @@ -648,7 +698,7 @@ impl QoizHandler { #[cfg(feature = "qoiz")] impl BitmapUpdateHandler for QoizHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let qoi = qoi_encode(bitmap)?; let mut inb = zstd_safe::InBuffer::around(&qoi); let mut data = vec![0; qoi.len()]; @@ -664,7 +714,12 @@ impl BitmapUpdateHandler for QoizHandler { &mut inb, zstd_safe::zstd_sys::ZSTD_EndDirective::ZSTD_e_flush, ) - .map_err(|code| anyhow!("failed to Zstd compress: {}", zstd_safe::get_error_name(code)))?; + .map_err(|code| { + ServerError::reason( + "qoiz compress", + format!("failed to Zstd compress: {}", zstd_safe::get_error_name(code)), + ) + })?; if res == 0 { break; } @@ -695,7 +750,7 @@ impl NsCodecHandler { #[cfg(feature = "nscodec")] impl BitmapUpdateHandler for NsCodecHandler { - fn handle(&mut self, bitmap: &BitmapUpdate) -> Result { + fn handle(&mut self, bitmap: &BitmapUpdate) -> ServerResult { let data = ironrdp_nscodec::encoder::encode( &bitmap.data, bitmap.width.get(), @@ -709,7 +764,7 @@ impl BitmapUpdateHandler for NsCodecHandler { } #[cfg(feature = "qoi")] -fn qoi_encode(bitmap: &BitmapUpdate) -> Result> { +fn qoi_encode(bitmap: &BitmapUpdate) -> ServerResult> { use ironrdp_graphics::image_processing::PixelFormat::*; // Map every 4-byte input — whether it nominally has an alpha byte or // an "X" filler — to the 3-channel-output `*x` variant of @@ -735,11 +790,12 @@ fn qoi_encode(bitmap: &BitmapUpdate) -> Result> { let enc = qoi::EncoderBuilder::new(&bitmap.data, bitmap.width.get().into(), bitmap.height.get().into()) .stride(bitmap.stride.get()) .raw_channels(raw_channels) - .build()?; - Ok(enc.encode_to_vec()?) + .build() + .map_err(|e| ServerError::custom("qoi encoder build", e))?; + enc.encode_to_vec().map_err(|e| ServerError::custom("qoi encode", e)) } -fn set_surface(bitmap: &BitmapUpdate, codec_id: u8, data: &[u8]) -> Result { +fn set_surface(bitmap: &BitmapUpdate, codec_id: u8, data: &[u8]) -> ServerResult { let destination = ExclusiveRectangle { left: bitmap.x, top: bitmap.y, @@ -759,5 +815,8 @@ fn set_surface(bitmap: &BitmapUpdate, codec_id: u8, data: &[u8]) -> Result>, @@ -22,42 +21,44 @@ impl TlsIdentityCtx { /// /// The file format can be either PEM (if the file extension ends with .pem) or DER. pub fn init_from_paths(cert_path: &Path, key_path: &Path) -> ServerResult { - Self::init_from_paths_inner(cert_path, key_path).map_err(from_anyhow) - } - - fn init_from_paths_inner(cert_path: &Path, key_path: &Path) -> anyhow::Result { let certs = if cert_path.extension().is_some_and(|ext| ext == "pem") { CertificateDer::pem_file_iter(cert_path) - .with_context(|| format!("reading server cert `{cert_path:?}`"))? + .map_err(|e| ServerError::custom("reading server cert", e))? .collect::, _>>() - .with_context(|| format!("collecting server cert `{cert_path:?}`"))? + .map_err(|e| ServerError::custom("collecting server cert", e))? } else { certs(&mut BufReader::new( - File::open(cert_path).with_context(|| format!("opening server cert `{cert_path:?}`"))?, + File::open(cert_path).map_err(|e| ServerError::io("opening server cert", e))?, )) .collect::, _>>() - .with_context(|| format!("collecting server cert `{cert_path:?}`"))? + .map_err(|e| ServerError::io("collecting server cert", e))? }; let priv_key = if key_path.extension().is_some_and(|ext| ext == "pem") { - PrivateKeyDer::from_pem_file(key_path).with_context(|| format!("reading server key `{key_path:?}`"))? + PrivateKeyDer::from_pem_file(key_path).map_err(|e| ServerError::custom("reading server key", e))? } else { - pkcs8_private_keys(&mut BufReader::new(File::open(key_path)?)) - .next() - .context("no private key")? - .map(PrivateKeyDer::from)? + pkcs8_private_keys(&mut BufReader::new( + File::open(key_path).map_err(|e| ServerError::io("opening server key", e))?, + )) + .next() + .ok_or_else(|| ServerError::reason("server key", "no private key"))? + .map(PrivateKeyDer::from) + .map_err(|e| ServerError::io("reading server key", e))? }; let pub_key = { use x509_cert::der::Decode as _; - let cert = certs.first().ok_or_else(|| std::io::Error::other("invalid cert"))?; - let cert = x509_cert::Certificate::from_der(cert).map_err(std::io::Error::other)?; + let cert = certs + .first() + .ok_or_else(|| ServerError::reason("server cert", "invalid cert"))?; + let cert = + x509_cert::Certificate::from_der(cert).map_err(|e| ServerError::custom("parsing server cert", e))?; cert.tbs_certificate .subject_public_key_info .subject_public_key .as_bytes() - .ok_or_else(|| std::io::Error::other("subject public key BIT STRING is not aligned"))? + .ok_or_else(|| ServerError::reason("server cert", "subject public key BIT STRING is not aligned"))? .to_owned() }; @@ -69,14 +70,10 @@ impl TlsIdentityCtx { } pub fn make_acceptor(&self) -> ServerResult { - self.make_acceptor_inner().map_err(from_anyhow) - } - - fn make_acceptor_inner(&self) -> anyhow::Result { let mut server_config = rustls::ServerConfig::builder() .with_no_client_auth() .with_single_cert(self.certs.clone(), self.priv_key.clone_key()) - .context("bad certificate/key")?; + .map_err(|e| ServerError::custom("bad certificate/key", e))?; // This adds support for the SSLKEYLOGFILE env variable (https://wiki.wireshark.org/TLS#using-the-pre-master-secret) server_config.key_log = Arc::new(rustls::KeyLogFile::new()); From 196fd3d07a961028fd927f2a55f9d06ee74d3f90 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 20 May 2026 17:46:50 -0500 Subject: [PATCH 3/5] refactor(server): use ServerResultExt::with_context at QoizHandler::new Replaces the closure-based `.map_err(|e| ServerError::custom(...))?` pattern with the `ServerResultExt::with_context` ext-trait introduced in #1242. Preserves the inner `Reason` kind from QoizHandler::new (was being lost programmatically inside a Custom-wrapping outer error) while replacing the context string with the more informative "failed to initialize qoiz handler" wording. Precheck-driven cleanup; no behavioral change beyond preserving the typed kind at this site. --- crates/ironrdp-server/src/encoder/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ironrdp-server/src/encoder/mod.rs b/crates/ironrdp-server/src/encoder/mod.rs index c41a45a07..e4f8a7212 100644 --- a/crates/ironrdp-server/src/encoder/mod.rs +++ b/crates/ironrdp-server/src/encoder/mod.rs @@ -1,7 +1,7 @@ use core::fmt; use core::num::NonZeroU16; -use crate::error::{ServerError, ServerErrorExt as _, ServerResult}; +use crate::error::{ServerError, ServerErrorExt as _, ServerResult, ServerResultExt as _}; use ironrdp_acceptor::DesktopSize; use ironrdp_graphics::diff::{Rect, find_different_rects_sub}; use ironrdp_pdu::encode_vec; @@ -132,9 +132,9 @@ impl UpdateEncoder { let bitmap_updater = if surface_flags.contains(CmdFlags::SET_SURFACE_BITS) { match codecs { #[cfg(feature = "qoiz")] - UpdateEncoderCodecs { qoiz: Some(id), .. } => BitmapUpdater::Qoiz( - QoizHandler::new(id).map_err(|e| ServerError::custom("failed to initialize qoiz handler", e))?, - ), + UpdateEncoderCodecs { qoiz: Some(id), .. } => { + BitmapUpdater::Qoiz(QoizHandler::new(id).with_context("failed to initialize qoiz handler")?) + } #[cfg(feature = "qoi")] UpdateEncoderCodecs { qoi: Some(id), .. } => BitmapUpdater::Qoi(QoiHandler::new(id)), UpdateEncoderCodecs { From cd5af8e125e539a2ee7a5b191b16886144764ecd Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 30 Apr 2026 11:23:47 -0500 Subject: [PATCH 4/5] feat(server)!: convert server.rs internals + on_disconnected to ServerError Third step of the staged migration started in #1242 and continued in the previous commit. Combines the on_disconnected signature change with the server.rs internal site conversion since both touch anyhow-flowing code; doing them together avoids a temporary anyhow-to-ServerError two-way bridge during the intermediate state. Public API change: ConnectionHandler::on_disconnected error: Option<&anyhow::Error> -> Option<&ServerError> This is a breaking change for handler implementations. Internal changes: - The two run_inner / run_connection_inner wrapper functions are folded back into run / run_connection. The accept loop calls the public method directly; result.as_ref().err() now feeds the new ServerError-typed parameter naturally. - ~25 .context() / bail! sites in run, run_connection, accept_finalize, handle_io_channel_data, handle_x224, handle_input_backlog, and the encode_share_data_pdu / deactivate_all helpers replaced with typed ServerError variants. Pattern alignment with ConnectorErrorExt: EncodeError sources -> ServerError::encode, DecodeError sources -> ServerError::decode, std::io::Error sources -> ServerError::io, Option with .ok_or_else -> ServerError::channel, bail!("Fastpath output not supported!") -> ServerError::unsupported, anything else -> ServerError::custom with a static context. - The from_anyhow bridge is retained only at one boundary: the RdpServerDisplay::updates() trait method still returns anyhow::Result, so the call site in run_connection wraps the anyhow result via from_anyhow. PR #4 of this migration converts the display traits and drops the bridge entirely. The anyhow dependency stays in Cargo.toml because RdpServerDisplay, RdpServerDisplayUpdates, and a small handful of consumer-facing trait returns still use anyhow::Result. PR #4 finishes that conversion and drops the dep. Tracking: #1209 --- crates/ironrdp-server/src/server.rs | 219 ++++++++++++++++------------ 1 file changed, 129 insertions(+), 90 deletions(-) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index b29dc1625..b407f3771 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -5,7 +5,6 @@ use core::time::Duration; use std::rc::Rc; use std::sync::Arc; -use anyhow::{Context as _, Result, bail}; use ironrdp_acceptor::{Acceptor, AcceptorResult, BeginResult, DesktopSize}; use ironrdp_async::Framed; use ironrdp_cliprdr::CliprdrServer; @@ -39,7 +38,7 @@ use crate::clipboard::CliprdrServerFactory; use crate::display::{DisplayUpdate, RdpServerDisplay}; use crate::echo::{EchoDvcBridge, EchoServerHandle, EchoServerMessage, build_echo_request}; use crate::encoder::{UpdateEncoder, UpdateEncoderCodecs}; -use crate::error::{ServerResult, from_anyhow}; +use crate::error::{ServerError, ServerErrorExt as _, ServerResult, from_anyhow}; #[cfg(feature = "egfx")] use crate::gfx::{EgfxServerMessage, GfxServerFactory}; use crate::handler::RdpServerInputHandler; @@ -85,7 +84,7 @@ pub trait ConnectionHandler: Send { &mut self, peer: SocketAddr, duration: Duration, - error: Option<&anyhow::Error>, + error: Option<&ServerError>, ) -> PostConnectionAction { let _ = (peer, duration, error); PostConnectionAction::Continue @@ -673,13 +672,6 @@ impl RdpServer { } pub async fn run_connection(&mut self, stream: S) -> ServerResult<()> - where - S: AsyncRead + AsyncWrite + Send + Sync + Unpin, - { - self.run_connection_inner(stream).await.map_err(from_anyhow) - } - - async fn run_connection_inner(&mut self, stream: S) -> Result<()> where S: AsyncRead + AsyncWrite + Send + Sync + Unpin, { @@ -705,7 +697,7 @@ impl RdpServer { let res = ironrdp_acceptor::accept_begin(framed, &mut acceptor) .await - .context("accept_begin failed")?; + .map_err(|e| ServerError::custom("accept_begin failed", e))?; match res { BeginResult::ShouldUpgrade(stream) => { @@ -739,7 +731,8 @@ impl RdpServer { pub_key.clone(), None, ) - .await?; + .await + .map_err(|e| ServerError::custom("accept_credssp", e))?; } let framed = self.accept_finalize(framed, acceptor).await?; @@ -759,15 +752,11 @@ impl RdpServer { } pub async fn run(&mut self) -> ServerResult<()> { - self.run_inner().await.map_err(from_anyhow) - } - - async fn run_inner(&mut self) -> Result<()> { // Create socket with control over options before binding. // Using TcpSocket instead of TcpListener::bind() allows setting // SO_REUSEADDR and IPv6 dual-stack mode. let socket = match self.opts.addr { - SocketAddr::V4(_) => TcpSocket::new_v4().context("create IPv4 socket")?, + SocketAddr::V4(_) => TcpSocket::new_v4().map_err(|e| ServerError::io("create IPv4 socket", e))?, SocketAddr::V6(_) => { // IPv6 socket: on Linux, dual-stack is the default // (net.ipv6.bindv6only=0), so IPv4 clients connect as @@ -775,7 +764,7 @@ impl RdpServer { // where IPV6_V6ONLY defaults to 1 (Windows, some BSDs), // only IPv6 clients will be accepted and a separate IPv4 // listener would be needed. - TcpSocket::new_v6().context("create IPv6 socket")? + TcpSocket::new_v6().map_err(|e| ServerError::io("create IPv6 socket", e))? } }; @@ -784,12 +773,18 @@ impl RdpServer { // on Windows SO_REUSEADDR has different semantics that allow a // second process to bind the same port, which is a security risk. #[cfg(unix)] - socket.set_reuseaddr(true).context("set SO_REUSEADDR")?; + socket + .set_reuseaddr(true) + .map_err(|e| ServerError::io("set SO_REUSEADDR", e))?; - socket.bind(self.opts.addr).context("bind listen address")?; + socket + .bind(self.opts.addr) + .map_err(|e| ServerError::io("bind listen address", e))?; - let listener = socket.listen(LISTENER_BACKLOG).context("start listener")?; - let local_addr = listener.local_addr()?; + let listener = socket + .listen(LISTENER_BACKLOG) + .map_err(|e| ServerError::io("start listener", e))?; + let local_addr = listener.local_addr().map_err(|e| ServerError::io("local_addr", e))?; debug!("Listening for connections on {local_addr}"); self.local_addr = Some(local_addr); @@ -828,11 +823,7 @@ impl RdpServer { drop(stream); } else { let started = tokio::time::Instant::now(); - // Internal accept loop uses the anyhow-returning inner method so the - // existing `on_disconnected(error: Option<&anyhow::Error>)` parameter - // continues to receive an anyhow value. Migration of that parameter - // to ServerError is tracked as a follow-up PR per #1209. - let result = self.run_connection_inner(stream).await; + let result = self.run_connection(stream).await; let duration = started.elapsed(); if let Err(ref error) = result { @@ -878,10 +869,10 @@ impl RdpServer { writer: &mut impl FramedWrite, io_channel_id: u16, user_channel_id: u16, - ) -> Result { + ) -> ServerResult { match action { Action::FastPath => { - let input = decode(&bytes)?; + let input = decode(&bytes).map_err(ServerError::decode)?; self.handle_fastpath(input).await; } @@ -889,7 +880,7 @@ impl RdpServer { if self .handle_x224(writer, io_channel_id, user_channel_id, &bytes) .await - .context("X224 input error")? + .map_err(|e| ServerError::custom("X224 input error", e))? { debug!("Got disconnect request"); return Ok(RunState::Disconnect); @@ -907,7 +898,7 @@ impl RdpServer { io_channel_id: u16, buffer: &mut Vec, mut encoder: UpdateEncoder, - ) -> Result<(RunState, UpdateEncoder)> { + ) -> ServerResult<(RunState, UpdateEncoder)> { if let DisplayUpdate::Resize(desktop_size) = update { debug!(?desktop_size, "Display resize"); encoder.set_desktop_size(desktop_size); @@ -921,7 +912,7 @@ impl RdpServer { break; }; - let mut fragmenter = fragmenter.context("error while encoding")?; + let mut fragmenter = fragmenter?; if fragmenter.size_hint() > buffer.len() { buffer.resize(fragmenter.size_hint(), 0); } @@ -930,7 +921,7 @@ impl RdpServer { writer .write_all(&buffer[..len]) .await - .context("failed to write display update")?; + .map_err(|e| ServerError::custom("failed to write display update", e))?; } } @@ -943,7 +934,7 @@ impl RdpServer { writer: &mut impl FramedWrite, io_channel_id: u16, user_channel_id: u16, - ) -> Result { + ) -> ServerResult { // Avoid wave messages queuing up and causing extra delay. When a // batch carries more than `WAVE_KEEP` waves, drop the OLDEST ones // and keep the most recent — playing stale audio just bakes the @@ -994,12 +985,16 @@ impl RdpServer { continue; } } - .context("failed to send rdpsnd event")?; + .map_err(|e| ServerError::custom("failed to send rdpsnd event", e))?; let channel_id = self .get_channel_id_by_type::() - .context("SVC channel not found")?; - let data = server_encode_svc_messages(msgs.into(), channel_id, user_channel_id)?; - writer.write_all(&data).await?; + .ok_or_else(|| ServerError::channel("SVC channel not found"))?; + let data = server_encode_svc_messages(msgs.into(), channel_id, user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; } ServerEvent::Clipboard(c) => { let Some(cliprdr) = self.get_svc_processor::() else { @@ -1017,12 +1012,16 @@ impl RdpServer { continue; } } - .context("failed to send clipboard event")?; + .map_err(|e| ServerError::custom("failed to send clipboard event", e))?; let channel_id = self .get_channel_id_by_type::() - .context("SVC channel not found")?; - let data = server_encode_svc_messages(msgs.into(), channel_id, user_channel_id)?; - writer.write_all(&data).await?; + .ok_or_else(|| ServerError::channel("SVC channel not found"))?; + let data = server_encode_svc_messages(msgs.into(), channel_id, user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; } ServerEvent::Echo(msg) => match msg { EchoServerMessage::SendRequest { payload } => { @@ -1045,14 +1044,19 @@ impl RdpServer { let request = build_echo_request(payload)?; let messages = - dvc::encode_dvc_messages(echo_channel_id, vec![request], ChannelFlags::SHOW_PROTOCOL)?; + dvc::encode_dvc_messages(echo_channel_id, vec![request], ChannelFlags::SHOW_PROTOCOL) + .map_err(ServerError::encode)?; let drdynvc_channel_id = self .get_channel_id_by_type::() - .context("DRDYNVC channel not found")?; - - let data = server_encode_svc_messages(messages, drdynvc_channel_id, user_channel_id)?; - writer.write_all(&data).await?; + .ok_or_else(|| ServerError::channel("DRDYNVC channel not found"))?; + + let data = server_encode_svc_messages(messages, drdynvc_channel_id, user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; } }, #[cfg(feature = "egfx")] @@ -1060,9 +1064,13 @@ impl RdpServer { EgfxServerMessage::SendMessages { messages } => { let drdynvc_channel_id = self .get_channel_id_by_type::() - .context("DRDYNVC channel not found")?; - let data = server_encode_svc_messages(messages, drdynvc_channel_id, user_channel_id)?; - writer.write_all(&data).await?; + .ok_or_else(|| ServerError::channel("DRDYNVC channel not found"))?; + let data = server_encode_svc_messages(messages, drdynvc_channel_id, user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; } }, ServerEvent::AutoDetectRttRequest => { @@ -1074,7 +1082,10 @@ impl RdpServer { io_channel_id, user_channel_id, )?; - writer.write_all(&data).await?; + writer + .write_all(&data) + .await + .map_err(|e| ServerError::io("write_all", e))?; } } } @@ -1090,13 +1101,13 @@ impl RdpServer { io_channel_id: u16, user_channel_id: u16, mut encoder: UpdateEncoder, - ) -> Result + ) -> ServerResult where R: FramedRead, W: FramedWrite, { debug!("Starting client loop"); - let mut display_updates = self.display.lock().await.updates().await?; + let mut display_updates = self.display.lock().await.updates().await.map_err(from_anyhow)?; let mut writer = SharedWriter::new(writer); let mut display_writer = writer.clone(); let mut event_writer = writer.clone(); @@ -1106,7 +1117,10 @@ impl RdpServer { let this = Rc::clone(&s); let dispatch_pdu = async move { loop { - let (action, bytes) = reader.read_pdu().await?; + let (action, bytes) = reader + .read_pdu() + .await + .map_err(|e| ServerError::custom("read pdu", e))?; let mut this = this.lock().await; match this .dispatch_pdu(action, bytes, &mut writer, io_channel_id, user_channel_id) @@ -1192,7 +1206,7 @@ impl RdpServer { reader: &mut Framed, writer: &mut Framed, result: AcceptorResult, - ) -> Result + ) -> ServerResult where R: FramedRead, W: FramedWrite, @@ -1212,12 +1226,12 @@ impl RdpServer { Ok(CredentialDecision::Reject) => { warn!("Credential validation rejected"); send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; - bail!("credential validation rejected"); + return Err(ServerError::reason("credential validation", "rejected by validator")); } Err(e) => { error!(error = %e, "Credential validator backend error"); send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; - bail!("credential validation backend error"); + return Err(ServerError::custom("credential validation", e)); } } } else { @@ -1243,9 +1257,13 @@ impl RdpServer { let Some(channel_id) = channel_id else { continue; }; - let svc_responses = channel.start()?; - let response = server_encode_svc_messages(svc_responses, channel_id, result.user_channel_id)?; - writer.write_all(&response).await?; + let svc_responses = channel.start().map_err(|e| ServerError::custom("svc start", e))?; + let response = server_encode_svc_messages(svc_responses, channel_id, result.user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&response) + .await + .map_err(|e| ServerError::io("write svc response", e))?; } } @@ -1256,7 +1274,7 @@ impl RdpServer { CapabilitySet::General(c) => { let fastpath = c.extra_flags.contains(GeneralExtraFlags::FASTPATH_OUTPUT_SUPPORTED); if !fastpath { - bail!("Fastpath output not supported!"); + return Err(ServerError::unsupported("Fastpath output")); } } CapabilitySet::Bitmap(b) => { @@ -1336,12 +1354,20 @@ impl RdpServer { let desktop_size = self.display.lock().await.size().await; let encoder = UpdateEncoder::new(desktop_size, surface_flags, update_codecs, self.opts.max_request_size) - .context("failed to initialize update encoder")?; + .map_err(|e| { + let mut e = e; + e.set_context("failed to initialize update encoder"); + e + })?; let state = self .client_loop(reader, writer, result.io_channel_id, result.user_channel_id, encoder) .await - .context("client loop failure")?; + .map_err(|e| { + let mut e = e; + e.set_context("client loop failure"); + e + })?; Ok(state) } @@ -1352,11 +1378,11 @@ impl RdpServer { io_channel_id: u16, user_channel_id: u16, frames: Vec>, - ) -> Result<()> { + ) -> ServerResult<()> { for frame in frames { match Action::from_fp_output_header(frame[0]) { Ok(Action::FastPath) => { - let input = decode(&frame)?; + let input = decode(&frame).map_err(ServerError::decode)?; self.handle_fastpath(input).await; } @@ -1408,8 +1434,8 @@ impl RdpServer { } } - async fn handle_io_channel_data(&mut self, data: SendDataRequest<'_>) -> Result { - let control: rdp::headers::ShareControlHeader = decode(data.user_data.as_ref())?; + async fn handle_io_channel_data(&mut self, data: SendDataRequest<'_>) -> ServerResult { + let control: rdp::headers::ShareControlHeader = decode(data.user_data.as_ref()).map_err(ServerError::decode)?; match control.share_control_pdu { ShareControlPdu::Data(header) => match header.share_data_pdu { @@ -1479,8 +1505,8 @@ impl RdpServer { io_channel_id: u16, user_channel_id: u16, frame: &[u8], - ) -> Result { - let message = decode::>>(frame)?; + ) -> ServerResult { + let message = decode::>>(frame).map_err(ServerError::decode)?; match message.0 { mcs::McsMessage::SendDataRequest(data) => { debug!( @@ -1494,9 +1520,15 @@ impl RdpServer { } if let Some(svc) = self.static_channels.get_by_channel_id_mut(data.channel_id) { - let response_pdus = svc.process(&data.user_data)?; - let response = server_encode_svc_messages(response_pdus, data.channel_id, user_channel_id)?; - writer.write_all(&response).await?; + let response_pdus = svc + .process(&data.user_data) + .map_err(|e| ServerError::custom("svc process", e))?; + let response = server_encode_svc_messages(response_pdus, data.channel_id, user_channel_id) + .map_err(ServerError::encode)?; + writer + .write_all(&response) + .await + .map_err(|e| ServerError::io("write svc response", e))?; } else { warn!(channel_id = data.channel_id, "Unexpected channel received: ID",); } @@ -1549,14 +1581,18 @@ impl RdpServer { } } - async fn accept_finalize(&mut self, mut framed: TokioFramed, mut acceptor: Acceptor) -> Result> + async fn accept_finalize( + &mut self, + mut framed: TokioFramed, + mut acceptor: Acceptor, + ) -> ServerResult> where S: AsyncRead + AsyncWrite + Sync + Send + Unpin, { loop { let (new_framed, result) = ironrdp_acceptor::accept_finalize(framed, &mut acceptor) .await - .context("failed to accept client during finalize")?; + .map_err(|e| ServerError::custom("failed to accept client during finalize", e))?; let (mut reader, mut writer) = split_tokio_framed(new_framed); @@ -1573,7 +1609,8 @@ impl RdpServer { acceptor, core::mem::take(&mut self.static_channels), desktop_size, - )?; + ) + .map_err(|e| ServerError::custom("deactivation-reactivation acceptor", e))?; framed = unsplit_tokio_framed(reader, writer); continue; } @@ -1601,7 +1638,7 @@ fn encode_share_data_pdu( share_data_pdu: rdp::headers::ShareDataPdu, io_channel_id: u16, user_channel_id: u16, -) -> Result> { +) -> ServerResult> { let header = rdp::headers::ShareDataHeader { share_data_pdu, stream_priority: rdp::headers::StreamPriority::Medium, @@ -1613,34 +1650,33 @@ fn encode_share_data_pdu( pdu_source: user_channel_id, share_control_pdu: ShareControlPdu::Data(header), }; - let user_data = encode_vec(&pdu)?.into(); + let user_data = encode_vec(&pdu).map_err(ServerError::encode)?.into(); let mcs_pdu = SendDataIndication { initiator_id: user_channel_id, channel_id: io_channel_id, user_data, }; - Ok(encode_vec(&X224(mcs_pdu))?) + encode_vec(&X224(mcs_pdu)).map_err(ServerError::encode) } -async fn deactivate_all( - io_channel_id: u16, - user_channel_id: u16, - writer: &mut impl FramedWrite, -) -> Result<(), anyhow::Error> { +async fn deactivate_all(io_channel_id: u16, user_channel_id: u16, writer: &mut impl FramedWrite) -> ServerResult<()> { let pdu = ShareControlPdu::ServerDeactivateAll(ServerDeactivateAll); let pdu = rdp::headers::ShareControlHeader { share_id: 0, pdu_source: io_channel_id, share_control_pdu: pdu, }; - let user_data = encode_vec(&pdu)?.into(); + let user_data = encode_vec(&pdu).map_err(ServerError::encode)?.into(); let pdu = SendDataIndication { initiator_id: user_channel_id, channel_id: io_channel_id, user_data, }; - let msg = encode_vec(&X224(pdu))?; - writer.write_all(&msg).await?; + let msg = encode_vec(&X224(pdu)).map_err(ServerError::encode)?; + writer + .write_all(&msg) + .await + .map_err(|e| ServerError::io("write deactivate_all", e))?; Ok(()) } @@ -1652,18 +1688,21 @@ async fn send_access_denied( io_channel_id: u16, user_channel_id: u16, writer: &mut impl FramedWrite, -) -> Result<(), anyhow::Error> { +) -> ServerResult<()> { let info = ServerSetErrorInfoPdu(ErrorInfo::ProtocolIndependentCode( ProtocolIndependentCode::ServerDeniedConnection, )); - let user_data = encode_vec(&info)?.into(); + let user_data = encode_vec(&info).map_err(ServerError::encode)?.into(); let pdu = SendDataIndication { initiator_id: user_channel_id, channel_id: io_channel_id, user_data, }; - let msg = encode_vec(&X224(pdu))?; - writer.write_all(&msg).await?; + let msg = encode_vec(&X224(pdu)).map_err(ServerError::encode)?; + writer + .write_all(&msg) + .await + .map_err(|e| ServerError::io("write access_denied", e))?; Ok(()) } From 5101afee6b150dbf110c70c36c627ff63e9d4468 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 20 May 2026 17:47:54 -0500 Subject: [PATCH 5/5] refactor(server): use ServerResultExt::with_context at two server.rs sites Replaces the closure-based set_context pattern at the UpdateEncoder::new and client_loop call sites in run_connection with the ServerResultExt::with_context ext-trait. Same semantics (preserves the inner kind, replaces the context string), substantially less verbose. Precheck-driven cleanup; no behavioral change. --- crates/ironrdp-server/src/server.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index b407f3771..163689b65 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -38,7 +38,7 @@ use crate::clipboard::CliprdrServerFactory; use crate::display::{DisplayUpdate, RdpServerDisplay}; use crate::echo::{EchoDvcBridge, EchoServerHandle, EchoServerMessage, build_echo_request}; use crate::encoder::{UpdateEncoder, UpdateEncoderCodecs}; -use crate::error::{ServerError, ServerErrorExt as _, ServerResult, from_anyhow}; +use crate::error::{ServerError, ServerErrorExt as _, ServerResult, ServerResultExt as _, from_anyhow}; #[cfg(feature = "egfx")] use crate::gfx::{EgfxServerMessage, GfxServerFactory}; use crate::handler::RdpServerInputHandler; @@ -1354,20 +1354,12 @@ impl RdpServer { let desktop_size = self.display.lock().await.size().await; let encoder = UpdateEncoder::new(desktop_size, surface_flags, update_codecs, self.opts.max_request_size) - .map_err(|e| { - let mut e = e; - e.set_context("failed to initialize update encoder"); - e - })?; + .with_context("failed to initialize update encoder")?; let state = self .client_loop(reader, writer, result.io_channel_id, result.user_channel_id, encoder) .await - .map_err(|e| { - let mut e = e; - e.set_context("client loop failure"); - e - })?; + .with_context("client loop failure")?; Ok(state) }