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.

Sign up for GitHub

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

Jump to bottom

WIP: #6664 fixes for server side external io api #6668

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

WIP: #6664 fixes for server side external io api #6668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
121e81d
allow to use "freerdp_peer_new" with invalid file descriptor
Dec 15, 2020
e451b6e
do not touch socket descriptor if it is set to invalid value
Dec 15, 2020
99970c3
typo fix
Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions libfreerdp/core/connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,8 @@ BOOL rdp_server_accept_nego(rdpRdp* rdp, wStream* s)
BOOL status;
rdpSettings* settings = rdp->settings;
rdpNego* nego = rdp->nego;
transport_set_blocking_mode(rdp->transport, TRUE);
if (rdp->context->peer->sockfd != -1)
transport_set_blocking_mode(rdp->transport, TRUE);

if (!nego_read_request(nego, s))
return FALSE;
Expand Down Expand Up @@ -1293,7 +1294,8 @@ BOOL rdp_server_accept_nego(rdpRdp* rdp, wStream* s)
if (!status)
return FALSE;

transport_set_blocking_mode(rdp->transport, FALSE);
if (rdp->context->peer->sockfd != -1)
transport_set_blocking_mode(rdp->transport, FALSE);
rdp_server_transition_to_state(rdp, CONNECTION_STATE_NEGO);
return TRUE;
}
Expand Down
12 changes: 8 additions & 4 deletions libfreerdp/core/peer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,12 +819,15 @@ BOOL freerdp_peer_context_new(freerdp_peer* client)
goto fail_error_description;
}

if (!transport_attach(rdp->transport, client->sockfd))
goto fail_transport_attach;
if (client->sockfd != -1)
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply add a client->ContextConfigure callback to struct rdp_freerdp_peer and call it here?
If the backend desires to configure something, implement it, otherwise skip it.
(just add the new function pointer at the end of the struct)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because freerdp_peer_context_new is not internal only call and used by existing implementations, but i think suggested solution will also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

additionally it will require to implement duplicated freerdp_peer_context_new in implementation, i am not sure what this is a good idea, or maybe i misunderstand you ?

Copy link
Contributor Author

@sss123next sss123next Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

one more reason to not add one more callbacks, context creation api already have one callback for additional context initialization from user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this api can be changed to more convenient and flexible one, but this will break compatibility with existing implementations using this api's

Copy link
Member

Choose a reason for hiding this comment

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

Ok, the suggestion I tried to explain was add a simple call IFCALLRET(client->ContextConfigure, ret, client, client->context); or similar before any transport_attach et al are called.
That will allow you to set your own transport functions and you can remove these socket checks (that dereference deep into the lower layers)

Copy link
Contributor Author

@sss123next sss123next Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ok, but we need somehow to disable socket related calls anyway, i mean calls which is not in switchable api. about transport_attach - just avoiding call transport_attach maybe not most elegant solution, but it will do the job without requirement of additional changes

{
if (!transport_attach(rdp->transport, client->sockfd))
goto fail_transport_attach;
transport_set_blocking_mode(rdp->transport, FALSE);
}

rdp->transport->ReceiveCallback = peer_recv_callback;
rdp->transport->ReceiveExtra = client;
transport_set_blocking_mode(rdp->transport, FALSE);
client->IsWriteBlocked = freerdp_peer_is_write_blocked;
client->DrainOutputBuffer = freerdp_peer_drain_output_buffer;
client->HasMoreToRead = freerdp_peer_has_more_to_read;
Expand Down Expand Up @@ -878,7 +881,8 @@ freerdp_peer* freerdp_peer_new(int sockfd)

option_value = TRUE;
option_len = sizeof(option_value);
setsockopt(sockfd, IPPROTO_TCP, TCP_NODELAY, (void*)&option_value, option_len);
if (sockfd != -1)
setsockopt(sockfd, IPPROTO_TCP, TCP_NODELAY, (void*)&option_value, option_len);

if (client)
{
Expand Down