feat(connector): surface ShareDataPdu variant in unexpected-PDU errors#1329
Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Open
Conversation
When the server sends a ShareControlPdu::Data wrapping a ShareDataPdu the client did not expect, the three error sites in legacy.rs and connection_activation.rs previously reported only "Data" via the outer ShareControlPdu::as_short_name(). For the common case where the server rejects a session by sending ServerSetErrorInfo, this hid the information the client actually needed to diagnose the failure. A new pub(crate) helper describe_unexpected_share_control_pdu drills into the Data wrapper and surfaces the inner ShareDataPdu variant name. When the inner variant is ServerSetErrorInfo, the ErrorInfo description is appended so callers can see why the server rejected the session without substring matching on the Reason string. The three sites in decode_share_data, decode_io_channel, and the ConnectionActivation::CapabilitiesExchange handler route through the helper. Non-Data variants continue to use the outer as_short_name(), so diagnostics for ServerDeactivateAll and ClientConfirmActive are preserved verbatim. Three unit tests cover the helper: a non-Data variant, a Data wrapper around a non-SetErrorInfo inner, and a Data wrapper around ServerSetErrorInfo carrying a ProtocolIndependentCode error. Refs Devolutions#1232.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ShareControlPdu::Datawrapping an unexpectedShareDataPdu, the three error sites inlegacy.rsandconnection_activation.rsnow drill into theDatawrapper and surface the inner variant name instead of reporting only"Data".ServerSetErrorInfospecifically (the asker's high-value case), the existingErrorInfo::description()is appended so callers can see why the server rejected the session without substring matching on theReasonstring.pub(crate) fn describe_unexpected_share_control_pduinlegacy.rscentralizes the formatting;decode_share_data,decode_io_channel, andConnectionActivation::CapabilitiesExchangeall route through it.Datavariants continue to use the outeras_short_name(), so diagnostics forServerDeactivateAllandClientConfirmActiveare preserved verbatim.Validation
legacy::testscover the helper: a non-Datavariant (ServerDeactivateAll), aDatawrapper around a non-SetErrorInfo inner (Update(Vec::new())), and aDatawrapper aroundServerSetErrorInfocarryingProtocolIndependentCode::ServerDeniedConnection.cargo xtask ciclean locally on1.89.0: fmt, lints, tests, typos, locks all green.Notes
pub(crate); no public API surface change.ConnectorErrorKindvariant for "server rejected at capabilities phase") is intentionally deferred. The asker framed it as optional and the wire-level information is now available in theReasonstring.Refs #1232rather thanClosesso the issue stays open while you decide on ask 2.