feat(dvc): close channel API for server and client#1302
feat(dvc): close channel API for server and client#1302uchouT (uchouT) wants to merge 7 commits into
Conversation
The response is optional: the server shouldn't recycle the channel id in the meantime! I think it should teardown the channel, but make sure we don't recycle IDs..
I think so (but mind the ids recycling) |
|
If the server shouldn't recycle the channel id, maybe we should change If this looks good to you, I'll change the data structure, and teardown the channel both sending |
I think we have to (otherwise the client could race with new channel creation and send close for channels ID that are recycled)
seems good |
There was a problem hiding this comment.
Pull request overview
This PR updates the ironrdp-dvc DRDYNVC implementation to support an explicit “close channel” API on both client and server, and adjusts Close PDU handling to better match the MS-RDPEDYC requirement to ignore unknown ChannelIds (and not respond to them).
Changes:
- Add
close_channelAPIs toDrdynvcServerandDrdynvcClient. - Rework server-side channel storage (replace
slabwith an internalBTreeMapallocator) and ensure channels triggerDvcProcessor::close(...)on teardown. - Update client Close handling to only respond when the channel actually existed (ignore unknown IDs without responding).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-dvc/src/server.rs | Adds server close_channel, changes channel allocation/storage, and updates server-side Close PDU processing. |
| crates/ironrdp-dvc/src/client.rs | Adds client close_channel and updates Close PDU behavior to ignore unknown channel IDs. |
| crates/ironrdp-dvc/src/lib.rs | Ensures DynamicVirtualChannel calls DvcProcessor::close(...) on drop when opened. |
| crates/ironrdp-dvc/Cargo.toml | Removes the slab dependency (no longer needed). |
| Cargo.lock | Removes slab from the lockfile due to dependency removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
29b5755 to
6900944
Compare
MS-RDPEDYC 3.2.5.2: > A channel can be closed by either a DVC client manager or a DVC server > manager. A DYNVC_CLOSE (section 2.2.4) PDU is used for both a close > request and a close response. DYNVC_CLOSE is a symmetric PDU. > If the client receives a DYNVC_CLOSE (section 2.2.4) PDU for a channel > that is not in the list of active ChannelIds, the client MUST ignore the > PDU and SHOULD NOT respond with a DYNVC_CLOSE (section 2.2.4) PDU. Currently the client always responds with a DYNVC_CLOSE Signed-off-by: uchouT <i@uchout.moe>
Signed-off-by: uchouT <i@uchout.moe>
Signed-off-by: uchouT <i@uchout.moe>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
DynamicChannel/DynamicVirtualChannel only call processor.close() on Drop when start() succeeded Signed-off-by: uchouT <i@uchout.moe>
1e75191 to
7fff1ec
Compare
|
Nothing to add. Looks great from here. |
Summary
Quesions
The server-side
close_channelis not implemented yet.A server-initiated
DYNVC_CLOSEmay receive a response from the client. Should the server perform the local channel teardown when sending the request, or only after the client's response arrives?When the server currently handles an incoming
ClosePdu, it just marksChannelState::Closed. Should it instead remove the DVC instance entirely?Marc-Andre Lureau (@elmarco)