API to offload io operations to external implementation #6324
Conversation
This file contains 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
|
Can one of the admins verify this patch? |
akallabeth
reviewed
Jun 29, 2020
rozhuk-im
reviewed
Jun 29, 2020
rozhuk-im
reviewed
Jun 29, 2020
|
A general comment: I'd rather like that there would be no difference between the current transport and the external one from the core freerdp point of view. All these |
|
probably i am lacking knowledge of libfreerdp internals, more detailed suggestions are welcome. |
Well it would be much more elegant, if FreeRDP core would just use a "transport", without knowing if it's the one that is set by default by FreeRDP, or one that has been set externally. So the code in FreeRDP core would be clean and nice without |
|
to achieve this, design must be changed. current behaviour is : correct me if i wrong. |
|
as for write, it is more simple, write can just be switchable and i guess no significant design/logic changes required. |
Write polling is also needed especially for server-side. |
|
in current implementation it is not used, i am not able to find any polling implementation for write in current code, if it is already used, can you please tell me where it is ? |
sss123next
changed the title
akallabeth
requested changes
Jul 9, 2020
There was a problem hiding this comment.
I´d suggest changing the API the following way:
- Add a functon registration function
freerdp_set_transport_callbacksor similar that allows setting read and write functions for the transport layer - Initialize these with the currently implemented read/write functions (or wrap them so that this is possible) at creation time
- Applications that want to use a different layer can then implement their own read/write functions externally and simply call
freerdp_set_transport_callbacksright after context creation
This way the changes to FreeRDP should stay minimal and for your use case you should have all degrees of freedom required to do what ever you like
libfreerdp/core/peer.c
Outdated
| @@ -811,6 +811,7 @@ BOOL freerdp_peer_context_new(freerdp_peer* client) | |||
| client->autodetect->context = context; | |||
| update_register_server_callbacks(client->update); | |||
| autodetect_register_server_callbacks(client->autodetect); | |||
| update_register_io_callbacks(rdp->update); | |||
There was a problem hiding this comment.
This should not be necessary with the new API
There was a problem hiding this comment.
why ?, is it not same initialization sequence as for client ?
|
i have one more question. |
|
ah, looks like i also need to make switchable rdp_client_connect, rdp_client_disconnect. |
akallabeth
requested changes
Jul 10, 2020
sss123next
force-pushed
the
external_io
branch
2 times, most recently
from
71c8be4
to
eab26b6
Compare
|
rebased on master, squashed |
or maybe just completely avoid usage of this, but i guess i need at least to handle disconnect initiated by freerdp somehow... |
|
looks like my changes does not work properly with some servers, investigating... crashing like this: |
|
let's look at it with little asan ) |
|
it was my logic error, fixed. |
akallabeth
reviewed
Jul 23, 2020
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
if i properly understand, server side api already switchable and defined in |
akallabeth
previously approved these changes
Aug 10, 2020
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
i am still not sure what this is enough to implement everything we need, |
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
akallabeth
reviewed
Aug 10, 2020
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
|
@sss123next ok, did some connection testing and in depth review.
so, long story short it is currently unusable as most connections abort due to invalid data (size) read |
|
can you provide more details about events ? |
|
@sss123next have not found the source yet. Problem is that the data decoded does not start/end where it is supposed to and the connection aborts quite fast (client wise) with a protocol error (parsing something invalid) |
still not get it, sorry, but i will appreciate if you provide even more details... existence of this event handles look strange for me, still do not get this part of design...
yes it is implemented just to be able to completely disable freerdp internal related code. |
|
@sss123next is this pr still relevant? |
|
we still missing few features from this, but you said you do not want to alter internal logic this much, also i have not finished my implementation in our project yet (new api from master still not completely tested), api from master looks ok, i just not tested it yet ( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.