-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
WIP: #6664 fixes for server side external io api #6668
base: master
Are you sure you want to change the base?
WIP: #6664 fixes for server side external io api #6668
Conversation
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patch.
Could you address the suggested changes?
Thank you.
@@ -819,8 +819,11 @@ 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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
i am just started, this is not ready to be merged yet, it's work in progress as suggested by prefix in PR name. |
to disable internal handling of file descriptor
aead5ca
to
e451b6e
Compare
this will contain necessary fixes in order server side part of external io api to work properly.