util: report back child errors to parent and throw

[ViniciusCestarii]

Report execvp() and close() failures to the parent via a socket and throw from SpawnProcess.

Previously the parent didn't have a way of knowing whether the child process successfully execute or not without polling for it or waiting for it.

Also make post-fork child not rely on on perror(), which is not async-signal-safe.

This addresses the TODO on commit 69652f0

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky, xyzconstant

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #311 (bugfix: clear FD_CLOEXEC in child instead of parent before fork by Sjors)
• #231 (Add windows support by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

Concept ACK dee4383. I plan to review more and left a suggestion below.

I think PR description might be underselling the change, because it describes the main advantage as avoiding a perror call that is not signal safe (so could hang). But I think a bigger improvement is translating the execvp failure in the child into an exception in the parent. Previously the parent didn't have a way of knowing whether the child process successfully execute or not without polling for it or waiting for it.

[ryanofsky]

In commit "util: report back child execvp error to parent and throw" (dee4383)

Could be good to extend the PR to cover this error as well and translate it to an exception in the parent. Might be good to introduce a struct like:

  struct ChildError {
      int which;  // e.g. 1=close, 2=fcntl, 3=execvp
      int err;    // errno
  };

and write that to the pipe, then have the parent throw

throw std::system_error(error.err, std::system_category(), name_for(error.which))

[ViniciusCestarii]

I agree, I have added ChildError to cover execv and close errors on d417706 and then added a helper function to read the error on a761aca and renamed it to SpawnError to cover the parent error on reading from socket.

[ViniciusCestarii]

Thanks for reviewing @ryanofsky! I updated the PR description to sell this PR more and I will address your comment.

[xyzconstant]

Concept ACK dee4383

Left some comments about descriptor leaks on the error paths. Planning to review more once you update on ryanofsky's feedback.

[Sjors]

Note that this change happens very close to the code touched by #311, but does seem orthogonal at first glance.

[ViniciusCestarii]

Thanks @xyzconstant for reviewing! I have force-pushed d417706 and pushed a761aca and updated PR title and description.

I have addresed:

• @xyzconstant comment about leaking fds and throwing wrong errno and @ryanofsky comment for reporting a struct ChildError to cover execv and close errors on d417706.
• @xyzconstant comment that this PR could benefit with a function for reading the error from child so I added ReadSpawnResult on a761aca and also renamed ChildError to SpawnError to cover the parent error on reading from socket.

I believe this now conflicts with #311 but it should be a simple rebase for whichever PR merges second.

I also noticed that there are several throws after fork that could benefit from reaping the child to prevent zombie process, but I believe this could be a follow up.