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

eio_linux: fallback to fork if clone3 is unavailable #524

Merged
merged 1 commit into from
May 26, 2023

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented May 24, 2023

clone3 is blocked by Docker's default security policy.

Also, use (uintptr_t) when storing a pointer in a uint64_t field, otherwise it doesn't work on 32-bit systems.

Fixes #514.

@talex5 talex5 added the bug Something isn't working label May 24, 2023
`clone3` is blocked by Docker's default security policy.

Also, use `(uintptr_t)` when storing a pointer in a `uint64_t` field,
otherwise it doesn't work on 32-bit systems.
@avsm
Copy link
Contributor

avsm commented May 25, 2023

This does leave us in a strange half-way house, since we don't derive any benefit from using clone3 if the interface always must be able to fallback to fork(). Any reason not to always just use fork?

@talex5
Copy link
Collaborator Author

talex5 commented May 25, 2023

Any reason not to always just use fork?

I was going to do that, but the fallback path requires some awkward code to handle the case where you spawn a process and then can't get an FD for it (e.g. due to FD limits), so I decided to use the new system when available. Also, I'm still planning to add support for namespaces at some point (it's OK if that doesn't work in a container).

Copy link
Contributor

@avsm avsm left a comment

Choose a reason for hiding this comment

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

A path towards supporting namespaces sounds like an excellent reason to keep trying to use clone3. LGTM

return child_pid; /* Success! */

if (errno != ENOSYS && errno != EPERM) {
uerror("clone3", Nothing); /* Unknown error */
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be fixed everywhere, not just here, but I believe the recommended function from OCaml 5 is caml_uerror so it's namespaced right. (see ocaml/ocaml@67a4d75).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, let's do that in a separate PR.

@talex5 talex5 merged commit 0b18c3c into ocaml-multicore:main May 26, 2023
@talex5 talex5 deleted the linux-fork branch May 26, 2023 15:46
@talex5 talex5 added this to the 0.10 milestone May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code with Eio.Process.spawn fails with “clone3 not implemented”
2 participants