Skip to content

refactor(server): define the USB device announce type - #1933

Open
uchouT (uchouT) wants to merge 1 commit into
Devolutions:masterfrom
uchouT:announce
Open

refactor(server): define the USB device announce type#1933
uchouT (uchouT) wants to merge 1 commit into
Devolutions:masterfrom
uchouT:announce

Conversation

@uchouT

Copy link
Copy Markdown
Contributor

Follow-up to #1417:

RDPEUSB types still reaches the consumer. RdpUsbDeviceAnnounceInfo carries the raw DeviceAnnounce, so a consumer that wants the device speed reads DeviceSpeed, an RDPEUSB PDU type. The goal is for a downstream bridge to need no ironrdp-rdpeusb dependency at all. I would rather agree on the facade shape than guess at it, so it is left as a follow-up.

Backends received ironrdp-rdpeusb's DeviceAnnounce, so implementing UsbRedirDevice meant naming RDPEUSB wire types. UsbDeviceAnnounce reports the bus speed as ironrdp-usb's UsbSpeed and drops the Windows USBDI capability fields, leaving the facade free of RDPEUSB vocabulary.

Downstream like qemu-rdp can thus drop ironrdp-rdpeusb deps.

Backends received ironrdp-rdpeusb's DeviceAnnounce, so implementing
UsbRedirDevice meant naming RDPEUSB wire types. UsbDeviceAnnounce
reports the bus speed as ironrdp-usb's UsbSpeed and drops the Windows
USBDI capability fields, leaving the facade free of RDPEUSB vocabulary.

Signed-off-by: uchouT <i@uchout.moe>
Copilot AI balanced review requested due to automatic review settings September 9, 2026 16:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Sep 9, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review confirms this is a well-scoped server facade refactor: RdpUsbDeviceAnnounceInfo now carries a server-owned UsbDeviceAnnounce (strings plus ironrdp-usb UsbSpeed) instead of the raw RDPEUSB DeviceAnnounce, DeviceAnnounce is dropped from the ironrdp-server re-exports, and a From<DeviceSpeed> for UsbSpeed conversion is added beside the wire type. No wire-format, framing, or state-machine changes; no in-repo consumers of the removed re-export remain, and ironrdp-rdpeusb already depended on ironrdp-usb. Two findings survive: the speed conversion silently reports undefined nonzero DeviceIsHighSpeed encodings as high speed (low), and the new 'first callback, invoked exactly once' contract on device_added, which my inspection of the rdpeusb server state machine partially verifies (duplicate ADD_DEVICE is ignored via the AwaitingDevice gate), but which overpromises because a channel closed before ADD_DEVICE invokes only close() and DevTextRsp is not state-gated (low, refined).

Comment thread crates/ironrdp-rdpeusb/src/pdu/sink.rs
Comment on lines +1428 to +1429
///
/// This is the first callback, and it is invoked exactly once.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[skeptical] 'First callback, invoked exactly once' contract on device_added is over-strong — low 🟡 — Inspection of ironrdp-rdpeusb's UrbdrcDeviceServer resolves the prior question: AddDev is forwarded to the backend only when state == AwaitingDevice and then transitions to Ready, so duplicate announcements are ignored and device_added runs at most once; no other backend callback precedes it for a well-behaved client. But the contract as written still overpromises: a device channel closed before ADD_DEVICE invokes only close(), so device_added may never run, and DevTextRsp is processed in any non-retracted state, so a nonconforming client can deliver device_text before device_added. Soften to 'at most once, and the first callback in normal operation' or note the close-before-announce path, especially since the new device_instance_id doc encourages keying per-device state on the announcement.

@github-actions github-actions Bot added ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure

Development

Successfully merging this pull request may close these issues.

2 participants