feat(rdpeusb): implement urbdrc client by uchouT · Pull Request #1365 · Devolutions/IronRDP

uchouT (uchouT) · 2026-06-08T21:21:33Z

Resolves #1137

The device dvc processor manages a pending_io map internally and offers API for upper layer to construct completion DvcMessage.

uchouT (uchouT) · 2026-06-10T08:41:23Z

Marc-Andre Lureau (@elmarco) Benoît Cortier (@CBenoit)
Hi, I've implemented the DVC processors for the urbdrc client. I would like to implement the urbdrc listener in a subsequent PR, as this PR has become too large and will require some modifications to drdynvc.

[EDIT]: UrbdrcListener is implemented in this PR.

Copilot

⚠️ Not ready to approve

The client state machine currently diverges from the issue’s required startup sequencing and adds substantial new protocol logic without the acceptance-criteria unit tests.

Pull request overview

Adds an ironrdp-rdpeusb client-side implementation of the URBDRC dynamic virtual channel, including a control-channel processor, per-device channel processor, and backend-neutral device metadata conversion used to construct RDPEUSB PDUs.

Changes:

Introduces UrbdrcListener, UrbdrcControlClient, and UrbdrcDeviceClient with a backend trait surface and pending-IO completion helpers.
Adds backend-neutral DeviceInfo → ADD_DEVICE conversion (PnP-style IDs, container IDs, capabilities) following observed FreeRDP behavior.
Integrates RDPEUSB PDUs with the DVC layer by implementing ironrdp_dvc::DvcEncode and adding an ironrdp-dvc dependency.

File summaries

File	Description
crates/ironrdp-rdpeusb/src/pdu/usb_dev/mod.rs	Adds `DvcEncode` impls and a helper to extract transfer request IDs.
crates/ironrdp-rdpeusb/src/pdu/sink.rs	Marks sink PDUs as DVC-encodable (`DvcEncode`).
crates/ironrdp-rdpeusb/src/pdu/notify.rs	Marks `CHANNEL_CREATED` as DVC-encodable (`DvcEncode`).
crates/ironrdp-rdpeusb/src/pdu/iface_manipulation.rs	Marks interface-manipulation PDUs as DVC-encodable (`DvcEncode`).
crates/ironrdp-rdpeusb/src/pdu/header.rs	Exposes `InterfaceId::from_raw` within the crate for client allocation.
crates/ironrdp-rdpeusb/src/pdu/completion/mod.rs	Marks completion PDUs as DVC-encodable (`DvcEncode`).
crates/ironrdp-rdpeusb/src/pdu/caps.rs	Marks capability PDUs as DVC-encodable (`DvcEncode`).
crates/ironrdp-rdpeusb/src/lib.rs	Adds channel name, exports client module, and introduces `InvalidDeviceInterfaceId` error type.
crates/ironrdp-rdpeusb/src/client/mod.rs	Implements the URBDRC listener + control/device client processors and backend traits.
crates/ironrdp-rdpeusb/src/client/device.rs	Implements backend-neutral USB device facts and `ADD_DEVICE` PDU construction.
crates/ironrdp-rdpeusb/Cargo.toml	Adds public dependency on `ironrdp-dvc`.
Cargo.lock	Records the new `ironrdp-dvc` dependency edge.

Copilot's findings

Files reviewed: 11/12 changed files
Comments generated: 8

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Marc-Andre Lureau (elmarco) · 2026-06-29T11:52:25Z

+    const fn alloc(&mut self) -> InterfaceId {
+        self.id += 1;
+        if self.id > 0x3F_FF_FF_FF {
+            panic!("USB device amount overflow")
+        }
+        InterfaceId::from_raw(self.id)
+    }
+}
+
+impl DvcChannelListener for UrbdrcListener {
+    fn channel_name(&self) -> &str {
+        CHANNEL_NAME
+    }
+
+    fn create(&mut self, channel_id: u32) -> Option<Box<dyn DvcProcessor>> {
+        if let Some(callback) = self.on_capability_exchanged.take() {
+            self.device_man.control_channel_assigned(channel_id);
+            Some(Box::new(UrbdrcControlClient::new(callback)))
+        } else {
+            #[expect(clippy::as_conversions)]
+            self.device_man.take_device_for_channel(channel_id).map(|backend| {
+                Box::new(UrbdrcDeviceClient::new(self.iface_man.alloc(), backend).expect("invalid interface id"))
+                    as Box<dyn DvcProcessor>
+            })


uchouT (@uchouT) this is intentionally post-poned? You could turn alloc() -> Option and remove all the expect/panic.

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) · 2026-06-17T18:40:22Z

Updated ironrdp-dvc deps to launch ci, and accepted style suggestions by Copilot. No other changes. Benoît Cortier (@CBenoit)

`ADD_DEVICE` message is constructed following FreeRDP's pattern. `UrbdrcDeviceClient` maintains a pending IO request map. When the packet is handled asynchronously, the information for the completion message construction (such as output buffer size) will be stored in this map, and `UrbdrcDeviceClient` provides public APIs for completion message construction, which will consume the map items internally. Signed-off-by: uchouT <i@uchout.moe>

Signed-off-by: uchouT <i@uchout.moe> Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Add test for new device sequence and channel setup sequence. Use hardcoded message id when constructing `ADD_VIRTUAL_CHANNEL`. Signed-off-by: uchouT <i@uchout.moe>

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

+        // Follow FreeRDP use device id as message id
+        Ok(Box::new(AddVirtualChannel {
+            msg_id: ADD_VIRTUAL_CHANNEL_MSG_ID,
+        }))


+#[derive(Debug, Clone)]
+pub struct DeviceText {
+    pub hresult: u32,
+    pub description: String,
+}


+// Ref: [New Device Sequence][1.3.1.2]
+// [1.3.1.2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/7e3da218-9cdc-4ebd-bb76-e70202c7f264
+#[test]
+fn new_device_sequence() {
+    let udev_iface = InterfaceId::try_from(4).expect("valid device interface id");
+    let backend = Box::new(TestDeviceBackend::new(simple_device_info()));
+    let mut client = UrbdrcDeviceClient::new(udev_iface, backend).expect("device client should be created");
+
+    assert!(!client.ready_for_io());
+
+    let resp = client
+        .process(
+            99,
+            &encode_pdu(&UrbdrcServerDevicePdu::ChanCreated(ChannelCreated {
+                msg_id: 21,
+                direction: Direction::ToClient,
+            })),
+        )
+        .expect("channel-created notification should succeed");
+    assert_eq!(resp.len(), 1);
+    let UrbdrcClientDevicePdu::ChanCreated(response) = decode_device_msg(&resp[0]) else {
+        panic!("expected channel-created response");
+    };
+    assert_eq!(response.msg_id, 21);
+    assert_eq!(response.direction, Direction::ToServer);
+    assert!(!client.ready_for_io());
+
+    let resp = client
+        .process(
+            99,
+            &encode_pdu(&UrbdrcServerDevicePdu::IfaceRelease(InterfaceRelease {
+                iface_id: proxy_iface_id(InterfaceId::NOTIFY_CLIENT),
+                msg_id: 22,
+            })),
+        )
+        .expect("notification release should succeed");
+    assert_eq!(resp.len(), 1);
+    assert!(client.ready_for_io());
+
+    let UrbdrcClientDevicePdu::AddDev(add_device) = decode_device_msg(&resp[0]) else {
+        panic!("expected add device");
+    };
+    assert_eq!(add_device.usb_device, udev_iface);
+}


uchouT (uchouT) · 2026-06-26T18:22:28Z

I added some tests covering new device add sequence and channel setup sequence, and I plan to add io sequence integration test when server state machine is settled.

Benoît Cortier (CBenoit)

Hi! Great work!

There is an acceptance criteria from issue #1137 that is not upheld:

Protocol-level unit tests with mock backend

It would be nice to have a "protocol-level unit tests with mock backend" covering the control channel setup sequence, the device channel setup sequence, and IO dispatch/completion round-trips. That being said, I’m happy if this is submitted as a follow up 🙂

Overall, I think it’s a very solid PR, and I’m good with the idea of merging now for velocity, and you could send follow up PRs for anything remaining. How do you want to proceed?

Benoît Cortier (CBenoit) · 2026-06-26T15:27:49Z

+use crate::{CHANNEL_NAME, InvalidDeviceInterfaceId};
+
+pub mod device;
+pub use device::*;


style:  We tend to prefer explicit re-exports for clarity: pub use device::{DeviceInfo, UsbConnectionSpeed, ...}

Benoît Cortier (CBenoit) · 2026-06-28T13:53:59Z

+                if no_ack {
+                    self.backend
+                        .transfer_out(channel_id, request_id.into(), transfer_out_pdu)?;
+                    Ok(Vec::new())
+                } else {


The UrbdrcDeviceBackend::transfer_out contract documents None = pending and Some = immediate completion. In the no-ack path, Some(response) is silently swallowed. Either the contract should explicitly say "returning Some is meaningless in no-ack context" or the backend shouldn't be expected to produce a response at all (different method signature for no-ack vs. ack).

Benoît Cortier (CBenoit) · 2026-06-28T13:55:35Z

+                    Ok(Vec::new())
+                }
+            }
+            IoCtl(io_ctl_pdu) => {


thought: The IoCtl and InternalIoCtl arms are nearly identical. A shared helper taking a Pending variant + msg_id + max_output_buf_size could eliminate the repetition cleanly. As you prefer.

Benoît Cortier (CBenoit) · 2026-06-28T14:23:20Z

I added some tests covering new device add sequence and channel setup sequence, and I plan to add io sequence integration test when server state machine is settled.

Oops, I just saw this after submitting my review I was drafting over several days. Sounds good to me. Thank you!

uchouT (uchouT) · 2026-06-29T00:49:34Z

Benoît Cortier (@CBenoit) Thanks for the review!

I've addressed the two inconsistent problems. And all other issues you pointed out make sense to me, but I'd like to address them in the follow-up PR since I've modified them locally when implementing server state machine.

Marc-Andre Lureau (elmarco) · 2026-06-29T11:52:25Z

+    const fn alloc(&mut self) -> InterfaceId {
+        self.id += 1;
+        if self.id > 0x3F_FF_FF_FF {
+            panic!("USB device amount overflow")
+        }
+        InterfaceId::from_raw(self.id)
+    }
+}
+
+impl DvcChannelListener for UrbdrcListener {
+    fn channel_name(&self) -> &str {
+        CHANNEL_NAME
+    }
+
+    fn create(&mut self, channel_id: u32) -> Option<Box<dyn DvcProcessor>> {
+        if let Some(callback) = self.on_capability_exchanged.take() {
+            self.device_man.control_channel_assigned(channel_id);
+            Some(Box::new(UrbdrcControlClient::new(callback)))
+        } else {
+            #[expect(clippy::as_conversions)]
+            self.device_man.take_device_for_channel(channel_id).map(|backend| {
+                Box::new(UrbdrcDeviceClient::new(self.iface_man.alloc(), backend).expect("invalid interface id"))
+                    as Box<dyn DvcProcessor>
+            })


uchouT (@uchouT) this is intentionally post-poned? You could turn alloc() -> Option and remove all the expect/panic.

Benoît Cortier (CBenoit)

Thank you Marc-Andre Lureau (@elmarco) uchouT (@uchouT)

I think we got to a pretty decent state, and the PR is big enough, let’s merge and address any remaining concern in follow up PRs 🙂

uchouT (uchouT) force-pushed the urbdrc-client branch from 6bfdada to 9b3f945 Compare June 8, 2026 21:31

uchouT (uchouT) marked this pull request as draft June 9, 2026 09:42

uchouT (uchouT) force-pushed the urbdrc-client branch from 97479e8 to acacd98 Compare June 10, 2026 08:21

uchouT (uchouT) marked this pull request as ready for review June 10, 2026 08:36

uchouT (uchouT) mentioned this pull request Jun 10, 2026

feat(dvc): expose dynamic channel accessors #1368

Merged

uchouT (uchouT) changed the title ~~feat(rdpeusb): implement urbdrc control client~~ feat(rdpeusb): implement urbdrc client dvc processor Jun 10, 2026

uchouT (uchouT) changed the title ~~feat(rdpeusb): implement urbdrc client dvc processor~~ feat(rdpeusb): implement urbdrc client Jun 11, 2026

uchouT (uchouT) force-pushed the urbdrc-client branch 10 times, most recently from c25c053 to b478576 Compare June 15, 2026 19:33

Benoît Cortier (CBenoit) requested a review from Copilot June 17, 2026 15:52

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) June 17, 2026 15:52 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

uchouT (uchouT) force-pushed the urbdrc-client branch from 8303938 to 06f93df Compare June 17, 2026 17:49

control client processor

2a8fe6c

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) force-pushed the urbdrc-client branch from 06f93df to 6c2eaaf Compare June 17, 2026 18:36

uchouT (uchouT) and others added 2 commits June 24, 2026 19:26

urbdrc listener

e2cc7f9

Signed-off-by: uchouT <i@uchout.moe> Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

uchouT (uchouT) force-pushed the urbdrc-client branch from 6c2eaaf to e2cc7f9 Compare June 24, 2026 11:35

uchouT (uchouT) mentioned this pull request Jun 25, 2026

RFE: Add MS-RDPEUSB support #1077

Open

add test and remove dev_id arg in add_virtual_channel

8de1c77

Add test for new device sequence and channel setup sequence. Use hardcoded message id when constructing `ADD_VIRTUAL_CHANNEL`. Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) requested a review from Copilot June 26, 2026 18:02

Copilot started reviewing on behalf of uchouT (uchouT) June 26, 2026 18:02 View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

Benoît Cortier (CBenoit) reviewed Jun 28, 2026

View reviewed changes

address inconsistent problem

f708145

Marc-Andre Lureau (elmarco) approved these changes Jun 29, 2026

View reviewed changes

avoid panic when alloc interface id; use FnOnce for callback

7a740c1

Benoît Cortier (CBenoit) approved these changes Jun 29, 2026

View reviewed changes

Benoît Cortier (CBenoit) enabled auto-merge (squash) June 29, 2026 12:52

Benoît Cortier (CBenoit) merged commit 66c9b5d into Devolutions:master Jun 29, 2026
22 checks passed

Uh oh!

Conversation

uchouT (uchouT) commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uchouT (uchouT) commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

⚠️ Not ready to approve

Copilot's findings

Uh oh!

Marc-Andre Lureau (elmarco) Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uchouT (uchouT) commented Jun 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

uchouT (uchouT) commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Jun 28, 2026

Uh oh!

uchouT (uchouT) commented Jun 29, 2026

Uh oh!

Marc-Andre Lureau (elmarco) Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

uchouT (uchouT) commented Jun 8, 2026 •

edited

Loading

uchouT (uchouT) commented Jun 10, 2026 •

edited

Loading

uchouT (uchouT) commented Jun 26, 2026 •

edited

Loading