Skip to content
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

feat: add support for dydynvc #419

Merged
merged 57 commits into from
Mar 29, 2024
Merged

feat: add support for dydynvc #419

merged 57 commits into from
Mar 29, 2024

Conversation

ibeckermayer
Copy link
Collaborator

@ibeckermayer ibeckermayer commented Mar 20, 2024

Apologies for the gigantic PR, but much of this necessitated being done in one gasp. A lot of the changes here are moving PDUs that were under crates/ironrdp-pdu/src/rdp/vc/dvc and using the old PduParsing paradigm into crates/ironrdp-dvc and forming them to the newer PduEncode/PduDecode paradigm.

I also added some practical facilities for working with arbitrary dynamic channels, taking inspiration from the existing patterns we have for static virtual channels.

No dynamic channels are hooked up yet for any of the IronRDP clients, however these changes were all tested manually via the isaiah/resize branch in Teleport.

Depends on #418

Isaiah Becker-Mayer added 24 commits March 19, 2024 20:51
…receiving a ServerDeactivateAll on the I/O channel
sequences into a separate connection_activation module. This allows us
to reuse them when we receive a Server Deactivate All.
`single_connect_step_read` and `single_connect_step_write`.

Also adds a `ConnectionActivationSequence::reset_clone` method to aid
in reusing the sequence for Deactivate All PDU handling.

Passes `ConnectionActivationSequence` to `x224::Processor` to hand back
upon receiving a Deactivate All PDU.
…:Windows::RDS::DisplayControl DVC to work.
…to us from the server via the BlockType in the BlockHeader, so it needn't be rigidly tracked explicitly in DecodingContext. This makes the RFX pipeline more flexible, which in turn allows us to seamlessly use it when we get another sync sequence mid-stream due to having fielded a Server Deactivate PDU.
client codebase. This uses `encode_dvc_data` to encode the DVC data
responses.

Currently no dynamic channels are added so the code is untested (WIP).
in the form of `DisplayControlClient`
I attempted to transfer everything in ironrdp_pdu::dvc::display
into it, but it required more refactoring that I'd expected to get it to
play nicely with the no_std possibility.

This also removes the requirement of keeping track of the channel_id
in each `DynamicVirtualChannel`, and instead lets the `DynamicChannelSet`
take care of returning that as needed.
`DvcPduEncode` trait simply inherits `PduEncode`. It is a marker trait
to indicate that the implementor, when encoded, is ready to be wrapped
in DataFirst and/or Data PDU(s).

`MonitorLayoutPdu` is a copy of the pdu by the same name in the
`ironrdp-pdu` crate. It is moved (well, copied for now) to the `ironrdp-dvc`
crate and implements `DvcPduEncode`.
added previously. Also creates a `DrdynvcPdu` enum that implements
`SvcPduEncode`, and begins migrating `DataFirstPdu` and `DataPdu` to
the new `PduEncode` paradigm.

In the process, also:
- Moved `DynamicVirtualChannel` and `DynamicChannelSet` from `client.rs`
to `lib.rs` since they can ultimately be used by both client and server.
… This has been tested to work for screen resize. The refactored server side code has not been tested.
…bles testing in ironrdp-dvc by eliminating the test = false line in Cargo.toml
Copy link

github-actions bot commented Mar 20, 2024

Coverage Report 🤖 ⚙️

Past:
Total lines: 29276
Covered lines: 17115 (58.46%)

New:
Total lines: 29091
Covered lines: 16849 (57.92%)

Diff: -0.54%

[this comment will be updated automatically]

@elmarco
Copy link
Contributor

elmarco commented Mar 20, 2024

This is likely going to conflict badly with #348

@pacmancoder
Copy link
Contributor

Thanks for the PR, I'll review it today!

@pacmancoder pacmancoder self-requested a review March 20, 2024 08:49
@pacmancoder
Copy link
Contributor

I feel like the right merge order will be the following:

cc @CBenoit

@CBenoit CBenoit changed the title Adds support for dydynvc feat: add support for dydynvc Mar 25, 2024
use core::fmt;
use slab::Slab;

use ironrdp_pdu as pdu;
Copy link
Member

@CBenoit CBenoit Mar 25, 2024

Choose a reason for hiding this comment

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

thought: We should avoid this use ironrdp_pdu as pdu. It’s a bit confusing. (In follow-up PR.)

Isaiah Becker-Mayer added 4 commits March 25, 2024 11:46
…e framed module. These should be able to be used in both ironrdp-async/src/connector.rs and ironrdp-acceptor/src/lib.rs (however the latter has not been manually tested as of this commit).
@ibeckermayer
Copy link
Collaborator Author

  • Once we squash and merge, make note of the commit where gfx.rs was removed in an issue for tracking a future implementation of GFX.

Base automatically changed from feat/deactivate-all to master March 27, 2024 06:39
@CBenoit
Copy link
Member

CBenoit commented Mar 27, 2024

@ibeckermayer can you rebase on top of master?

Copy link
Contributor

@pacmancoder pacmancoder left a comment

Choose a reason for hiding this comment

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

LGTM!

@CBenoit CBenoit self-assigned this Mar 28, 2024
crates/ironrdp-pdu/src/utils.rs Show resolved Hide resolved
crates/ironrdp-pdu/src/utils.rs Show resolved Hide resolved
crates/ironrdp-pdu/src/utils.rs Show resolved Hide resolved
crates/ironrdp-svc/src/lib.rs Show resolved Hide resolved
crates/ironrdp-server/src/server.rs Show resolved Hide resolved
crates/ironrdp-displaycontrol/src/client.rs Show resolved Hide resolved
crates/ironrdp-displaycontrol/src/client.rs Show resolved Hide resolved
crates/ironrdp-dvc/src/complete_data.rs Show resolved Hide resolved
crates/ironrdp-dvc/src/lib.rs Show resolved Hide resolved
crates/ironrdp-dvc/src/lib.rs Show resolved Hide resolved
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM! New comments can be addressed in a follow up PR.

Thank you for the PR @ibeckermayer
Thank you for helping with reviewing @pacmancoder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants