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

Add low-level process support to eio_linux #435

Closed
wants to merge 13 commits into from

Conversation

patricoferris
Copy link
Collaborator

The next instalment for #330. This adds initial low-level process support to Eio_linux using the spawn library which has no dependencies. I started implementing it and realised they already had, and it supports windows and has a lot of signal and error handling built-in that I otherwise would have ended up copying. It is also released under MIT so presumably we could reuse their code to avoid the dependency.

I put it under Eio_unix because I envisaged reusing spawn with other backends like IOCP/kqueue I think that means we have to add optional or something to the eio_unix library?

It doesn't do program resolution so I tried to do that in OCaml, I also default to the paths [ "/usr/bin"; "/usr/local/bin" ] if we can't find PATH -- I don't know if that's reasonable. Also I'm not sure on what error we should raise if we cannot find the program.

@patricoferris
Copy link
Collaborator Author

Just as a confirmation that using Spawn might not be such a bad idea (and adding it to Eio_unix), I added a backend to kqueue using it + an Eio.Process module here: patricoferris@9875511

lib_eio/unix/eio_unix.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Comments in-line.

I'd still like proper clone3 support at some point, but using spawn is fine for now (just not in eio.unix).

Note that I'm changing the way FDs work a bit in #440, so you might want to rebase on that before changing anything.

lib_eio/unix/eio_unix.ml Outdated Show resolved Hide resolved
lib_eio/unix/eio_unix.ml Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.ml Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.ml Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.mli Outdated Show resolved Hide resolved
lib_eio_linux/eio_stubs.c Show resolved Hide resolved
lib_eio/unix/eio_unix.ml Outdated Show resolved Hide resolved
@talex5
Copy link
Collaborator

talex5 commented Feb 10, 2023

Regarding PATH, I'd leave that out of the low-level API (which should just export what the OS provides).

lib_eio_linux/eio_stubs.c Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.mli Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.mli Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.mli Outdated Show resolved Hide resolved
lib_eio_linux/eio_linux.ml Outdated Show resolved Hide resolved
@patricoferris
Copy link
Collaborator Author

W.r.t this comment:

Regarding PATH, I'd leave that out of the low-level API (which should just export what the OS provides).

We now vendor spawn so have a little more control over what we can do on the C-side of things, did you mean to just mark it as calling execve and let the user worry about things like PATH ?

@talex5
Copy link
Collaborator

talex5 commented Mar 1, 2023

We now vendor spawn so have a little more control over what we can do on the C-side of things, did you mean to just mark it as calling execve and let the user worry about things like PATH ?

Yes.

string ->
string list ->
t
(** Spawns a subprocess. If the process has not finished when the switch is released, the process
Copy link
Contributor

Choose a reason for hiding this comment

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

This diff is looking really good! Just one thing about the documentation though: there is quite a lot of useful information in the underlying spawn ocamldoc that is worth propagating here, now that the underlying library is no longer exposed. For example, it answers: "what happens if [env] is None" in that documentation, which is quite important to know.

has finished. *)

val signal : t -> int -> unit
(** [signal t s] send a signal [s] to process [t]. For example to stop a process you could use
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(** [signal t s] send a signal [s] to process [t]. For example to stop a process you could use
(** [signal t s] sends a signal [s] to process [t]. For example, to terminate a process use

confusingly, SIGSTOPing a process is different from SIGTERMing a process :-)

Comment on lines +286 to +287
(** [wait t] waits for the process [t] to exit. This blocks the fiber until the process
has finished. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(** [wait t] waits for the process [t] to exit. This blocks the fiber until the process
has finished. *)
(** [wait t] blocks the fiber until process [t] has terminated, either by
exiting or receiving a signal. *)

@talex5
Copy link
Collaborator

talex5 commented Mar 13, 2023

The spawn library has a single function that does a lot of stuff, and different platforms will want to provide different features. I'm wondering if it would be better to make it more modular.

The basic problem we have is that we can't call OCaml code after forking. However, we can make a list of C functions to call. Here's an experiment that defines a Fork_action.t and has spawn take a list of those:

https://github.com/ocaml-multicore/eio/compare/main...talex5:eio:spawn?expand=1

e.g. (I put this in eio_posix, but it's demonstrating the Linux-specific execveat and fchdir actions here):

open Eio.Std

module L = Eio_posix.Low_level

let () =
  Eio_posix.run @@ fun _env ->
  Switch.run @@ fun sw ->
  let exe = L.openat ~sw ~mode:0 "/bin/ls" L.Open_flags.(rdonly + excl) in
  let dir = L.openat ~sw ~mode:0 "/etc" L.Open_flags.(rdonly + directory + excl) in
  let pid = L.spawn ~sw L.Fork_action.[
      fchdir dir;
      execat exe [| "ls"; "-l" |];
    ] in
  traceln "spawned PID %d" pid

Probably some of this could go in Eio_unix and be shared between the various backends.

@avsm
Copy link
Contributor

avsm commented Mar 13, 2023

Fork_actions could also be expanded to include privilege dropping syscalls for the subprocess too, which is rather neat. E.g. a call to Fork_action.pledge in a future Eio_openbsd :-)

RyanGibb added a commit to RyanGibb/ocaml-exec-shell that referenced this pull request Mar 13, 2023
@patricoferris
Copy link
Collaborator Author

Cool API. The idea here is that you can have arbitrarily complicated lists of actions right? The example is a simple case that I think (at least in my mind) is easier to understand/code with something like what we have i.e. spawn ~cwd:dir "ls" but we have no nice way to say chdir; exec; chdir; exec inside the same fork. Similarly with pledge you might want to only pledge after doing something else first and this gives you control over that ordering. Seems like a good idea, and you could certainly implement the current API on top of this! I don't know in practice how shareable the actions will be across OSes... my experience thus far is to be wary of subtleties but something like fchdir should work.

Do you think the Fork_action.t list is the right abstraction? Or should this be more abstract and more of a DSL to combine actions in some meaningful way? I'm really just thinking out loud at this point but it might be nice to be able to do something like pipe (exec foo) (exec bar) although again that could probably be built on top if this anyway...

Not quite sure where to go from here, experiment with this API first porting more of the spawn code to this modular approach?

@talex5
Copy link
Collaborator

talex5 commented Mar 13, 2023

The example is a simple case that I think (at least in my mind) is easier to understand/code with something like what we have i.e. spawn ~cwd:dir "ls"

Yes, I'd expect the high-level API to build this up for you.

but we have no nice way to say chdir; exec; chdir; exec inside the same fork.

You can't do anything after exec (well, unless it fails, I guess). But things that might be useful include:

  • Reassigning FDs.
  • Dropping privileges (pledge etc, as Anil mentioned), which each platform does differently.
  • Setting up container namespaces.
  • Changing directory (with chdir or fchdir).
  • Maybe progress group stuff? Signal masks?
  • Execing with execve or execveat.

And being able to do this with fork, vfork or clone3.

I'm really just thinking out loud at this point but it might be nice to be able to do something like pipe (exec foo) (exec bar) although again that could probably be built on top if this anyway...

I think that would be built on top. This API is just for stuff that can only happen after forking, where we can't use OCaml.

Not quite sure where to go from here, experiment with this API first porting more of the spawn code to this modular approach?

Yes, I think so. I can try to clean it up over the next couple of days. Do you want to try putting the higher-level API on top after that?

@RyanGibb
Copy link
Contributor

I currently have a problem where there are some syscalls I need to make after forking but before execing. This is to set up a PTY for the child process. I've got a branch using this PR at the moment, but without the PTY setup. While writing a C wrapper could work, I think @talex5's proposal would provide a good solution for this.

@patricoferris
Copy link
Collaborator Author

Yes, I think so. I can try to clean it up over the next couple of days. Do you want to try putting the higher-level API on top after that?

Sounds good to me :))

RyanGibb added a commit to RyanGibb/ocaml-exec-shell that referenced this pull request Mar 14, 2023
@patricoferris
Copy link
Collaborator Author

Closing in favour of #472 :))

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.

5 participants