-
Notifications
You must be signed in to change notification settings - Fork 301
feat(client): report which transport carries the session #2015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -927,6 +927,15 @@ impl RdpeudpConnection { | |
| self.params.as_ref().map(|p| p.mtu) | ||
| } | ||
|
|
||
| /// The protocol version the handshake settled on, once it is complete: version 1 or 2 | ||
| /// for MS-RDPEUDP, version 3 for MS-RDPEUDP2. | ||
| pub fn negotiated_version(&self) -> Option<UdpVersion> { | ||
| self.params.as_ref().map(|params| match params.wire { | ||
| WireFormat::V1 { version } => UdpVersion(version), | ||
| WireFormat::V2 => UdpVersion::V3, | ||
| }) | ||
| } | ||
|
Comment on lines
+930
to
+937
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [skeptical] negotiated_version returns Some before the handshake completes on the server path — low 🟡 — The doc states the value is available 'once it is complete', but accept() populates params with WireFormat::V2 while still in SynReceived, so a server-side connection reports Some(UdpVersion::V3) before the final ACK establishes the session. A caller using Some as an establishment signal is misled. In-tree use is unaffected because the tokio driver records the version only on Event::Connected, and the sibling mtu() accessor shares this early-params timing, but this is new public API text whose stated contract the implementation violates on the public accept path. Fix the doc or gate on the Established state. |
||
|
|
||
| /// Diagnostics for the MS-RDPEUDP version 1/2 data path; `None` on MS-RDPEUDP2. | ||
| pub fn v1_stats(&self) -> Option<V1Stats> { | ||
| let params = self.params.as_ref()?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -620,6 +620,12 @@ impl RpcApp { | |
| debug!(?control, "RAIL control received"); | ||
| } | ||
| RdpOutputEvent::WindowingOrders(_) => {} | ||
| RdpOutputEvent::Transport { | ||
| reliable_udp, | ||
| udp_version, | ||
| } => { | ||
| info!(reliable_udp, ?udp_version, "Session transport"); | ||
| } | ||
|
Comment on lines
+623
to
+628
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [skeptical + code-compressor] Transport state logged twice at info for the in-tree viewer — low 🟡 — The client logs info!(reliable_udp, ?udp_version, "Session transport") when it emits the new event (crates/ironrdp-client/src/rdp.rs:4272, added by this PR), and the viewer's new match arm logs the identical fields with the identical message on receipt (crates/ironrdp-viewer/src/app.rs:623-628, also added here). The viewer runs the client in-process, so every transport change produces two consecutive equivalent info lines in one process, in a PR whose purpose is log-based diagnostics. Any other embedder already gets the library-side line plus the event payload. Exactly one of the two logs is needed: either shrink the viewer arm to an empty match like the WindowingOrders arm above it, or drop the client-side info! in favor of embedder-owned presentation. Diagnostics-only impact, hence low severity.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 723ae4f. The client no longer logs the transport itself. Embedders log the event as they do the other session events, so the viewer arm keeps the single info line. |
||
| // Only produced when the client is built with `.with_desktop_updates()`, which the | ||
| // viewer does not opt into: it always presents full-frame `Image` snapshots instead. | ||
| RdpOutputEvent::DesktopUpdate(_) => {} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[skeptical] reliable_udp doc overclaims that the graphics pipeline moved onto UDP — low 🟡 — The flag is read from ActiveStage::reliable_udp_dvc_tunnel_in_use, which is documented as 'whether Soft-Sync moved any DVC to the reliable UDP tunnel' and is implemented via has_channels_on_tunnel, an any-channel check. With declined channels producing partial moves (handled by PR
#2007), some DVCs can stay on TCP, potentially including the graphics pipeline. The doc asserts they all travel over UDP, so an embedder keying diagnostics or status off reliable_udp can report graphics-on-UDP when it is not. Narrow the wording to 'at least one dynamic channel' or expose graphics-channel-specific state.