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

[WIP] use g_fork_execvp instead of g_fork #2598

Draft
wants to merge 15 commits into
base: devel
Choose a base branch
from

Conversation

unstabler
Copy link

@unstabler unstabler commented Mar 18, 2023

DESCRIPTION

  • This PR moved from [WIP] use fork+execvp instead of fork team-unstablers/xrdp-tumod#1
  • On macOS, some System APIs (including VideoToolbox) which communicates with Mach/XPC services stops working after fork().
    • This PR fixes problem that initialization of VTCompressionSession fails after fork().
  • However, I think there is only one of the above items that will be improved, even though quite a bit of core code has been modified, so you may feel free to close this PR if you don't think it will be necessary. machine-translated

CHANGES

  • g_fork_execvp(exec, argv) has added
  • g_get_executable_path(exectype, buf, bufsize) has added
  • xrdp now accepts --child-process and --child-fd [fd], entrypoint for child process also added
  • xrdp will use g_fork_execvp() instead of g_fork() for accepting new connections

TODO

  • rollback 4d1b20a : should i reconstruct logger by passing logger->fd to child process (via argv, or ...), as i reconstructed struct trans?
  • remove unreachable / unused code: xrdp_listen.c:814 ...
  • clean-up struct xrdp_process *, struct trans * after xrdp_process_main_loop

translated from: ですが、かなりコアなコードが修正されたのに、改善される事項は上記の一つしかいないと思うので、もし必要にならないと思われた場合は気楽にこのPRをクローズしても結構です。

@matt335672
Copy link
Member

Hi @unstabler

This is going along the right lines, and for the reasons you state regarding MacOS, we need to get this functionality in one way or another.

There's a couple of things you haven't considered which will need to be included in a complete solution:-

  • The sub-process will need to re-read the config file and re-open the log file.
  • The config file may be overridden with the -c switch passed to xrdp.
  • There's some stuff around setting FD_CLOEXEC on existing file descriptors which isn't in the main product yet, but will be added soon.

I'm currently focussed on getting sesman split into two for xrdp v1.x. This will mean I'm going to need to cover all of the above in sesman anyway, so I suggest we hold off on this one until I've got that done. I've probably missed a few things, and we'll find those during testing.

Regarding your existing code, you can simplify things by ditching the --child-process switch and simply using --child-fd. Unless I'm missing something, I can't see a use-case for having one without the other.

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

Successfully merging this pull request may close these issues.

2 participants