-
Notifications
You must be signed in to change notification settings - Fork 49
Add nonunix platform support #274
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
Open
ryanofsky
wants to merge
20
commits into
bitcoin-core:master
Choose a base branch
from
ryanofsky:pr/wins
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
b15d63e
doc: Bump version 11 > 12
ryanofsky 36c91a0
util, refactor: Add ProcessId type alias and use it
ryanofsky 94af41b
util, refactor: Add SocketId type alias and use it
ryanofsky beaa50a
util, refactor: Add ConnectInfo type alias and use it
ryanofsky b16f8c4
util, refactor: Handle forking inside ExecProcess
ryanofsky 022b29b
util, refactor: Add SocketPair() and use it in SpawnProcess
ryanofsky 24c5e57
util: Clear FD_CLOEXEC on child socket before exec
ryanofsky 3c81cf2
proxy, refactor: Replace EventLoop wakeup fd integers with KJ stream …
ryanofsky 17a1952
cmake: Bump minimum required Cap'n Proto version to 0.9
ryanofsky 091f5e1
proxy, refactor: Change ConnectStream and ServeStream to accept strea…
ryanofsky bfc2db7
proxy: Call shutdownWrite() in Connection destructor
ryanofsky 1060a95
util, refactor: Fix PtrOrValue constructor for move-only types on MSVC
ryanofsky 362d416
proxy, refactor: Fix C4305 truncation warning in Accessor on MSVC
ryanofsky 3fd227c
type-interface, refactor: Fix typename decltype() SFINAE in CustomBui…
ryanofsky 926ae35
ci: Check out bitcoin/bitcoin PR #35084 instead of master
ryanofsky 28e4c7f
proxy: Fix shutdownWrite() exception handling on macOS with dynamic l…
ryanofsky f6aa627
ipc: Wrap mpgen main() in try-catch to print errors
ryanofsky 7f513a4
doc: Remove trailing whitespace
ryanofsky c9aa806
cmake: Replace capnp_PREFIX path construction with cmake-provided sym…
ryanofsky 7cb83a5
cmake: Fix CapnProto tool paths broken by Ubuntu Noble packaging bug
ryanofsky File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include <filesystem> | ||
| #include <iostream> | ||
| #include <kj/common.h> | ||
| #include <kj/debug.h> | ||
| #include <kj/string-tree.h> | ||
| #include <pthread.h> | ||
| #include <sstream> | ||
|
|
@@ -116,9 +117,9 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size) | |
| return result; | ||
| } | ||
|
|
||
| int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args) | ||
| std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_to_args) | ||
| { | ||
| int fds[2]; | ||
| SocketId fds[2]; | ||
| if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) { | ||
| throw std::system_error(errno, std::system_category(), "socketpair"); | ||
| } | ||
|
|
@@ -129,10 +130,10 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args) | |
| // locks at fork time. In that case, running code that allocates memory or | ||
| // takes locks in the child between fork() and exec() can deadlock | ||
| // indefinitely. Precomputing arguments in the parent avoids this. | ||
| const std::vector<std::string> args{fd_to_args(fds[0])}; | ||
| const std::vector<std::string> args{connect_info_to_args(std::to_string(fds[0]))}; | ||
| const std::vector<char*> argv{MakeArgv(args)}; | ||
|
|
||
| pid = fork(); | ||
| ProcessId pid = fork(); | ||
| if (pid == -1) { | ||
| throw std::system_error(errno, std::system_category(), "fork"); | ||
| } | ||
|
|
@@ -168,22 +169,31 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args) | |
| perror("execvp failed"); | ||
| _exit(127); | ||
| } | ||
| return fds[1]; | ||
| return {pid, fds[1]}; | ||
| } | ||
|
|
||
| void ExecProcess(const std::vector<std::string>& args) | ||
| SocketId StartSpawned(const ConnectInfo& connect_info) | ||
| { | ||
| return std::stoi(connect_info); | ||
| } | ||
|
|
||
| ProcessId ExecProcess(const std::vector<std::string>& args) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that this function does fork() + execvp() rather than just execvp, is ExecProcess still the intended name? |
||
| { | ||
| const std::vector<char*> argv{MakeArgv(args)}; | ||
| ProcessId pid; | ||
| KJ_SYSCALL(pid = fork()); | ||
| if (pid) return pid; | ||
| if (execvp(argv[0], argv.data()) != 0) { | ||
| perror("execvp failed"); | ||
| if (errno == ENOENT && !args.empty()) { | ||
| std::cerr << "Missing executable: " << fs::weakly_canonical(args.front()) << '\n'; | ||
| } | ||
| _exit(1); | ||
| } | ||
| KJ_UNREACHABLE; | ||
| } | ||
|
|
||
| int WaitProcess(int pid) | ||
| int WaitProcess(ProcessId pid) | ||
| { | ||
| int status; | ||
| if (::waitpid(pid, &status, /*options=*/0) != pid) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
StartSpawned reads as imperative, but the body just parses an int out of the connect-info string (and the header comment seems to describe more work than the function actually does). Would something like ParseConnectInfo fit better?
I might be missing the full picture here.
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.
Yes, I was missing the full picture. Just looked PR 231