| Message ID | pull.766.v2.git.1612208747.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Simple IPC Mechanism | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > Here is version 2 of my "Simple IPC" series and addresses the following > review comments: > ... > Junio C Hamano (1): > ci/install-depends: attempt to fix "brew cask" stuff Huh?
On 2/1/21 5:20 PM, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> Here is version 2 of my "Simple IPC" series and addresses the following >> review comments: >> ... >> Junio C Hamano (1): >> ci/install-depends: attempt to fix "brew cask" stuff > > Huh? > Sorry. I had to prepend that one to the patch series to get the CI builds to run. I've been working rebased against "v2.30.0" and GitGitGadget references "master". Jeff
Hi Junio & Jeff, On Mon, 1 Feb 2021, Jeff Hostetler wrote: > On 2/1/21 5:20 PM, Junio C Hamano wrote: > > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > > > Here is version 2 of my "Simple IPC" series and addresses the following > > > review comments: > > > ... > > > Junio C Hamano (1): > > > ci/install-depends: attempt to fix "brew cask" stuff > > > > Huh? > > > > Sorry. I had to prepend that one to the patch series to get the > CI builds to run. I've been working rebased against "v2.30.0" and > GitGitGadget references "master". The idea being that we want to be able to merge this branch as-is into Git for Windows (and also into microsoft/git), and therefore do not want to base it on a later commit that is not reachable from git-for-windows/git's `main` branch. Maybe it is time to merge `jc/macos-install-dependencies-fix` down to `maint`? Then we could base Simple IPC/FSMonitor on `maint` instead, and would still have the benefit we want. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > The idea being that we want to be able to merge this branch as-is into Git > for Windows (and also into microsoft/git), and therefore do not want to > base it on a later commit that is not reachable from git-for-windows/git's > `main` branch. > > Maybe it is time to merge `jc/macos-install-dependencies-fix` down to > `maint`? Then we could base Simple IPC/FSMonitor on `maint` instead, and > would still have the benefit we want. Now you mention it, if we ever have an update to the current 'maint' branch, we'd trigger this known failure in macOS build without a good reason, even though we have a fix that has been in use on the 'master' and above. I definitely qshould merge the jc/macos-install-dependencies-fix topic down to 'maint' now. Are there other topics that deserve to be in 'maint' that are "obviously correct" people can think of? Thanks.
Hi Junio, On Thu, 4 Feb 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > The idea being that we want to be able to merge this branch as-is into Git > > for Windows (and also into microsoft/git), and therefore do not want to > > base it on a later commit that is not reachable from git-for-windows/git's > > `main` branch. > > > > Maybe it is time to merge `jc/macos-install-dependencies-fix` down to > > `maint`? Then we could base Simple IPC/FSMonitor on `maint` instead, and > > would still have the benefit we want. > > Now you mention it, if we ever have an update to the current 'maint' > branch, we'd trigger this known failure in macOS build without a > good reason, even though we have a fix that has been in use on the > 'master' and above. > > I definitely qshould merge the jc/macos-install-dependencies-fix > topic down to 'maint' now. > > Are there other topics that deserve to be in 'maint' that are > "obviously correct" people can think of? I looked over the branches merged into `master` that are not in `maint`, and from a cursory look, these seem to be good candidates: - pk/subsub-fetch-fix-take-2 - rs/rebase-commit-validation - en/stash-apply-sparse-checkout - ds/for-each-repo-noopfix - pb/mergetool-tool-help-fix - jk/t5516-deflake - jc/macos-install-dependencies-fix - jk/forbid-lf-in-git-url - jk/log-cherry-pick-duplicate-patches - js/skip-dashed-built-ins-from-config-mak From a cursory look, it might seem as if ds/maintenance-prefetch-cleanup would also be a good candidate, but it is not based on `maint`, although it _does_ appear to fix an issue introduced in v2.30.0-rc0~23^2~8. There is another candidate that I am not _quite_ sure about: dl/p4-encode-after-kw-expansion. It _seems_ as if it would be good to apply on the maintenance train, but I am uncertain how important a bug fix it is. There are also a couple test updates that might be nice to have in `maint`: - nk/perf-fsmonitor-cleanup - mt/t4129-with-setgid-dir - ad/t4129-setfacl-target-fix Finally, there are documentation updates that I would probably merge, if I was tasked with updating `maint`: - ta/doc-typofix - pb/doc-modules-git-work-tree-typofix - jc/sign-off - vv/send-email-with-less-secure-apps-access - ug/doc-lose-dircache - ab/gettext-charset-comment-fix - bc/doc-status-short - tb/local-clone-race-doc - ab/fsck-doc-fix - jt/packfile-as-uri-doc Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Are there other topics that deserve to be in 'maint' that are >> "obviously correct" people can think of? > > I looked over the branches merged into `master` that are not in `maint`, > and from a cursory look, these seem to be good candidates: > ... > There are also a couple test updates that might be nice to have in > `maint`: > ... > Finally, there are documentation updates that I would probably merge, if I > was tasked with updating `maint`: > ... Your list more or less matches what the ML (merge later) script on the todo/ branch produces when it is fed the RelNotes (the script just greps for "merge laster to maint" comments and shows the result in way a bit easier to use for me). The ones that are in RelNotes but not in your list are ab/branch-sort # 7 (11 days ago) ar/t6016-modernise # 1 (3 weeks ago) dl/p4-encode-after-kw-expansion # 1 (3 weeks ago) fc/t6030-bisect-reset-removes-auxiliary-files # 1 (4 weeks ago) ma/doc-pack-format-varint-for-sizes # 1 (3 weeks ago) ma/more-opaque-lock-file # 5 (11 days ago) ma/t1300-cleanup # 3 (3 weeks ago) zh/arg-help-format # 2 (3 weeks ago) and I think all of them are safe to merge down. Thanks for being an independent source I can rely on to sanity check what is in RelNotes. Very much appreciated.
Here is version 2 of my "Simple IPC" series and addresses the following review comments: [1] Redo packet_write_gently() to take a scratch buffer argument and fixup callers to avoid potential thread-stack problems caused by a very large stack buffer when used multi-threaded callers. This turned out to be a little more involved than anticipated because the pkt-line code doesn't know about its thread state nor an opportunity to initialize thread-state. [2] Deleted the unix_stream_socket() helper function and inline it in the few callers and then let those call sites decide whether to call die() or not. [3] Refactor unix_stream_listen() to take an "options" structure and to incorporate the changes I described in my earlier unix_stream_listen_gently(). [4] Update unix_stream_connect() to return errors rather than calling die(). [5] Update the simple-ipc server startup to detect dead and/or in-use Unix domain sockets and/or create a new socket in a race-friendly way. I now use a variation of the atomic lock-rename-trick when creating the socket (details are in a large comment in the code). Jeff Hostetler (10): pkt-line: promote static buffer in packet_write_gently() to callers pkt-line: add write_packetized_from_buf2() that takes scratch buffer simple-ipc: design documentation for new IPC mechanism simple-ipc: add win32 implementation simple-ipc: add t/helper/test-simple-ipc and t0052 unix-socket: elimiate static unix_stream_socket() helper function unix-socket: add options to unix_stream_listen() unix-socket: add no-chdir option to unix_stream_listen() unix-socket: do not call die in unix_stream_connect() simple-ipc: add Unix domain socket implementation Johannes Schindelin (3): pkt-line: optionally skip the flush packet in write_packetized_from_buf() pkt-line: (optionally) libify the packet readers pkt-line: accept additional options in read_packetized_to_strbuf() Junio C Hamano (1): ci/install-depends: attempt to fix "brew cask" stuff Documentation/technical/api-simple-ipc.txt | 34 + Makefile | 8 + builtin/credential-cache--daemon.c | 3 +- ci/install-dependencies.sh | 8 +- compat/simple-ipc/ipc-shared.c | 28 + compat/simple-ipc/ipc-unix-socket.c | 1127 ++++++++++++++++++++ compat/simple-ipc/ipc-win32.c | 751 +++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 6 + convert.c | 4 +- pkt-line.c | 70 +- pkt-line.h | 26 +- simple-ipc.h | 230 ++++ t/helper/test-simple-ipc.c | 485 +++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 129 +++ unix-socket.c | 67 +- unix-socket.h | 16 +- 19 files changed, 2949 insertions(+), 47 deletions(-) create mode 100644 Documentation/technical/api-simple-ipc.txt create mode 100644 compat/simple-ipc/ipc-shared.c create mode 100644 compat/simple-ipc/ipc-unix-socket.c create mode 100644 compat/simple-ipc/ipc-win32.c create mode 100644 simple-ipc.h create mode 100644 t/helper/test-simple-ipc.c create mode 100755 t/t0052-simple-ipc.sh base-commit: 71ca53e8125e36efbda17293c50027d31681a41f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/766 Range-diff vs v1: 1: 1155a45cf64 < -: ----------- pkt-line: use stack rather than static buffer in packet_write_gently() -: ----------- > 1: 4c6766d4183 ci/install-depends: attempt to fix "brew cask" stuff -: ----------- > 2: 3b03a8ff7a7 pkt-line: promote static buffer in packet_write_gently() to callers -: ----------- > 3: e671894b4c0 pkt-line: add write_packetized_from_buf2() that takes scratch buffer 3: edf5ac95d66 ! 4: 0832f7d324d pkt-line: optionally skip the flush packet in write_packetized_from_buf() @@ Commit message packets before a final flush packet, so let's extend this function to prepare for that scenario. + Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## convert.c ## @@ pkt-line.c: int write_packetized_from_fd(int fd_in, int fd_out) -int write_packetized_from_buf(const char *src_in, size_t len, int fd_out) +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out, + int flush_at_end) + { + static struct packet_scratch_space scratch; + +- return write_packetized_from_buf2(src_in, len, fd_out, &scratch); ++ return write_packetized_from_buf2(src_in, len, fd_out, ++ flush_at_end, &scratch); + } + + int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out, ++ int flush_at_end, + struct packet_scratch_space *scratch) { int err = 0; - size_t bytes_written = 0; -@@ pkt-line.c: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out) - err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write); +@@ pkt-line.c: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out, + err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch); bytes_written += bytes_to_write; } - if (!err) @@ pkt-line.h: void packet_buf_write_len(struct strbuf *buf, const char *data, size -int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out, + int flush_at_end); + int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out, ++ int flush_at_end, + struct packet_scratch_space *scratch); /* - * Read a packetized line into the buffer, which must be at least size bytes 2: b7d678bc918 ! 5: 43bc4a26b79 pkt-line: (optionally) libify the packet readers @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b *pktlen = -1; ## pkt-line.h ## -@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); +@@ pkt-line.h: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out, * * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an * ERR packet. 4: 2f399ac107c = 6: 6a389a35335 pkt-line: accept additional options in read_packetized_to_strbuf() 5: 7064c5e9ffa ! 7: a7275b4bdc2 simple-ipc: design documentation for new IPC mechanism @@ Documentation/technical/api-simple-ipc.txt (new) + +This IPC mechanism differs from the existing `sub-process.c` model +(Documentation/technical/long-running-process-protocol.txt) and used -+by applications like Git-LFS because the server is assumed to be very -+long running system service. In contrast, a "sub-process model process" -+is started with the foreground process and exits when the foreground -+process terminates. How the server is started is also outside the -+scope of the IPC mechanism. ++by applications like Git-LFS. In the simple-ipc model the server is ++assumed to be a very long-running system service. In contrast, in the ++LFS-style sub-process model the helper is started with the foreground ++process and exits when the foreground process terminates. ++ ++How the simple-ipc server is started is also outside the scope of the ++IPC mechanism. For example, the server might be started during ++maintenance operations. + +The IPC protocol consists of a single request message from the client and +an optional request message from the server. For simplicity, pkt-line 6: 9e27c07d785 ! 8: 388366913d4 simple-ipc: add win32 implementation @@ compat/simple-ipc/ipc-win32.c (new) +enum ipc_active_state ipc_client_try_connect( + const char *path, + const struct ipc_client_connect_options *options, -+ int *pfd) ++ struct ipc_client_connection **p_connection) +{ + wchar_t wpath[MAX_PATH]; + enum ipc_active_state state = IPC_STATE__OTHER_ERROR; ++ int fd = -1; + -+ *pfd = -1; ++ *p_connection = NULL; + + trace2_region_enter("ipc-client", "try-connect", NULL); + trace2_data_string("ipc-client", NULL, "try-connect/path", path); @@ compat/simple-ipc/ipc-win32.c (new) + state = IPC_STATE__INVALID_PATH; + else + state = connect_to_server(wpath, WINDOWS_CONNECTION_TIMEOUT_MS, -+ options, pfd); ++ options, &fd); + + trace2_data_intmax("ipc-client", NULL, "try-connect/state", + (intmax_t)state); + trace2_region_leave("ipc-client", "try-connect", NULL); ++ ++ if (state == IPC_STATE__LISTENING) { ++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection)); ++ (*p_connection)->fd = fd; ++ } ++ + return state; +} + -+int ipc_client_send_command_to_fd(int fd, const char *message, -+ struct strbuf *answer) ++void ipc_client_close_connection(struct ipc_client_connection *connection) ++{ ++ if (!connection) ++ return; ++ ++ if (connection->fd != -1) ++ close(connection->fd); ++ ++ free(connection); ++} ++ ++int ipc_client_send_command_to_connection( ++ struct ipc_client_connection *connection, ++ const char *message, struct strbuf *answer) +{ + int ret = 0; + @@ compat/simple-ipc/ipc-win32.c (new) + + trace2_region_enter("ipc-client", "send-command", NULL); + -+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) { ++ if (write_packetized_from_buf2(message, strlen(message), ++ connection->fd, 1, ++ &connection->scratch_write_buffer) < 0) { + ret = error(_("could not send IPC command")); + goto done; + } + -+ FlushFileBuffers((HANDLE)_get_osfhandle(fd)); ++ FlushFileBuffers((HANDLE)_get_osfhandle(connection->fd)); + -+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) { ++ if (read_packetized_to_strbuf(connection->fd, answer, ++ PACKET_READ_NEVER_DIE) < 0) { + ret = error(_("could not read IPC response")); + goto done; + } @@ compat/simple-ipc/ipc-win32.c (new) + const struct ipc_client_connect_options *options, + const char *message, struct strbuf *response) +{ -+ int fd; + int ret = -1; + enum ipc_active_state state; ++ struct ipc_client_connection *connection = NULL; + -+ state = ipc_client_try_connect(path, options, &fd); ++ state = ipc_client_try_connect(path, options, &connection); + + if (state != IPC_STATE__LISTENING) + return ret; + -+ ret = ipc_client_send_command_to_fd(fd, message, response); -+ close(fd); ++ ret = ipc_client_send_command_to_connection(connection, message, response); ++ ++ ipc_client_close_connection(connection); ++ + return ret; +} + @@ compat/simple-ipc/ipc-win32.c (new) + struct ipc_server_data *server_data; + pthread_t pthread_id; + HANDLE hPipe; ++ struct packet_scratch_space scratch_write_buffer; +}; + +/* @@ compat/simple-ipc/ipc-win32.c (new) +static int do_io_reply_callback(struct ipc_server_reply_data *reply_data, + const char *response, size_t response_len) +{ ++ struct packet_scratch_space *scratch = ++ &reply_data->server_thread_data->scratch_write_buffer; ++ + if (reply_data->magic != MAGIC_SERVER_REPLY_DATA) + BUG("reply_cb called with wrong instance data"); + -+ return write_packetized_from_buf(response, response_len, -+ reply_data->fd, 0); ++ return write_packetized_from_buf2(response, response_len, ++ reply_data->fd, 0, scratch); +} + +/* @@ simple-ipc.h (new) +#endif + +#ifdef SUPPORTS_SIMPLE_IPC ++#include "pkt-line.h" + +/* + * Simple IPC Client Side API. @@ simple-ipc.h (new) + */ +enum ipc_active_state ipc_get_active_state(const char *path); + ++struct ipc_client_connection { ++ int fd; ++ struct packet_scratch_space scratch_write_buffer; ++}; ++ +/* + * Try to connect to the daemon on the named pipe or socket. + * -+ * Returns IPC_STATE__LISTENING (and an fd) when connected. ++ * Returns IPC_STATE__LISTENING and a connection handle. + * + * Otherwise, returns info to help decide whether to retry or to + * spawn/respawn the server. @@ simple-ipc.h (new) +enum ipc_active_state ipc_client_try_connect( + const char *path, + const struct ipc_client_connect_options *options, -+ int *pfd); ++ struct ipc_client_connection **p_connection); ++ ++void ipc_client_close_connection(struct ipc_client_connection *connection); + +/* + * Used by the client to synchronously send and receive a message with -+ * the server on the provided fd. ++ * the server on the provided client connection. + * + * Returns 0 when successful. + * + * Calls error() and returns non-zero otherwise. + */ -+int ipc_client_send_command_to_fd(int fd, const char *message, -+ struct strbuf *answer); ++int ipc_client_send_command_to_connection( ++ struct ipc_client_connection *connection, ++ const char *message, struct strbuf *answer); + +/* + * Used by the client to synchronously connect and send and receive a 9: 69969c2b8d3 = 9: f0bebf1cdb3 simple-ipc: add t/helper/test-simple-ipc and t0052 -: ----------- > 10: f5d5445cf42 unix-socket: elimiate static unix_stream_socket() helper function 7: 96268351ac6 ! 11: 7a6a69dfc20 unix-socket: create gentle version of unix_stream_listen() @@ Metadata Author: Jeff Hostetler <jeffhost@microsoft.com> ## Commit message ## - unix-socket: create gentle version of unix_stream_listen() + unix-socket: add options to unix_stream_listen() - Create a gentle version of `unix_stream_listen()`. This version does - not call `die()` if a socket-fd cannot be created and does not assume - that it is safe to `unlink()` an existing socket-inode. + Update `unix_stream_listen()` to take an options structure to override + default behaviors. This includes the size of the `listen()` backlog + and whether it should always unlink the socket file before trying to + create a new one. Also eliminate calls to `die()` if it cannot create + a socket. - `unix_stream_listen()` uses `unix_stream_socket()` helper function to - create the socket-fd. Avoid that helper because it calls `die()` on - errors. - - `unix_stream_listen()` always tries to `unlink()` the socket-path before - calling `bind()`. If there is an existing server/daemon already bound - and listening on that socket-path, our `unlink()` would have the effect - of disassociating the existing server's bound-socket-fd from the socket-path - without notifying the existing server. The existing server could continue - to service existing connections (accepted-socket-fd's), but would not - receive any futher new connections (since clients rendezvous via the - socket-path). The existing server would effectively be offline but yet - appear to be active. + Normally, `unix_stream_listen()` always tries to `unlink()` the + socket-path before calling `bind()`. If there is an existing + server/daemon already bound and listening on that socket-path, our + `unlink()` would have the effect of disassociating the existing + server's bound-socket-fd from the socket-path without notifying the + existing server. The existing server could continue to service + existing connections (accepted-socket-fd's), but would not receive any + futher new connections (since clients rendezvous via the socket-path). + The existing server would effectively be offline but yet appear to be + active. Furthermore, `unix_stream_listen()` creates an opportunity for a brief race condition for connecting clients if they try to connect in the @@ Commit message Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> + ## builtin/credential-cache--daemon.c ## +@@ builtin/credential-cache--daemon.c: static int serve_cache_loop(int fd) + + static void serve_cache(const char *socket_path, int debug) + { ++ struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT; + int fd; + +- fd = unix_stream_listen(socket_path); ++ fd = unix_stream_listen(socket_path, &opts); + if (fd < 0) + die_errno("unable to bind to '%s'", socket_path); + + ## unix-socket.c ## -@@ unix-socket.c: int unix_stream_listen(const char *path) - errno = saved_errno; +@@ unix-socket.c: int unix_stream_connect(const char *path) return -1; } -+ -+int unix_stream_listen_gently(const char *path, -+ const struct unix_stream_listen_opts *opts) -+{ + +-int unix_stream_listen(const char *path) ++int unix_stream_listen(const char *path, ++ const struct unix_stream_listen_opts *opts) + { +- int fd, saved_errno; + int fd = -1; -+ int bind_successful = 0; + int saved_errno; -+ struct sockaddr_un sa; -+ struct unix_sockaddr_context ctx; -+ -+ if (unix_sockaddr_init(&sa, path, &ctx) < 0) -+ goto fail; ++ int bind_successful = 0; ++ int backlog; + struct sockaddr_un sa; + struct unix_sockaddr_context ctx; + +- unlink(path); +- + if (unix_sockaddr_init(&sa, path, &ctx) < 0) + return -1; + -+ fd = socket(AF_UNIX, SOCK_STREAM, 0); -+ if (fd < 0) + fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) +- die_errno("unable to create socket"); + goto fail; + + if (opts->force_unlink_before_bind) + unlink(path); -+ -+ if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) -+ goto fail; + + if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) + goto fail; + bind_successful = 1; -+ -+ if (listen(fd, opts->listen_backlog_size) < 0) -+ goto fail; -+ -+ unix_sockaddr_cleanup(&ctx); -+ return fd; -+ -+fail: -+ saved_errno = errno; -+ unix_sockaddr_cleanup(&ctx); -+ close(fd); + +- if (listen(fd, 5) < 0) ++ if (opts->listen_backlog_size > 0) ++ backlog = opts->listen_backlog_size; ++ else ++ backlog = 5; ++ if (listen(fd, backlog) < 0) + goto fail; + + unix_sockaddr_cleanup(&ctx); +@@ unix-socket.c: int unix_stream_listen(const char *path) + fail: + saved_errno = errno; + unix_sockaddr_cleanup(&ctx); +- close(fd); ++ if (fd != -1) ++ close(fd); + if (bind_successful) + unlink(path); -+ errno = saved_errno; -+ return -1; -+} + errno = saved_errno; + return -1; + } ## unix-socket.h ## @@ - int unix_stream_connect(const char *path); - int unix_stream_listen(const char *path); + #ifndef UNIX_SOCKET_H + #define UNIX_SOCKET_H +struct unix_stream_listen_opts { + int listen_backlog_size; + unsigned int force_unlink_before_bind:1; +}; + -+int unix_stream_listen_gently(const char *path, -+ const struct unix_stream_listen_opts *opts); ++#define UNIX_STREAM_LISTEN_OPTS_INIT \ ++{ \ ++ .listen_backlog_size = 5, \ ++ .force_unlink_before_bind = 1, \ ++} + + int unix_stream_connect(const char *path); +-int unix_stream_listen(const char *path); ++int unix_stream_listen(const char *path, ++ const struct unix_stream_listen_opts *opts); + #endif /* UNIX_SOCKET_H */ 8: 383a9755669 ! 12: 745b6d5fb74 unix-socket: add no-chdir option to unix_stream_listen_gently() @@ Metadata Author: Jeff Hostetler <jeffhost@microsoft.com> ## Commit message ## - unix-socket: add no-chdir option to unix_stream_listen_gently() + unix-socket: add no-chdir option to unix_stream_listen() Calls to `chdir()` are dangerous in a multi-threaded context. If `unix_stream_listen()` is given a socket pathname that is too big to @@ Commit message Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag is set. - Extend the public interface to `unix_stream_listen_gently()` to also - expose this new flag. - Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> ## unix-socket.c ## @@ unix-socket.c: int unix_stream_connect(const char *path) if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1; -@@ unix-socket.c: int unix_stream_listen(const char *path) - { - int fd, saved_errno; - struct sockaddr_un sa; -- struct unix_sockaddr_context ctx; -+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT; - - unlink(path); - -@@ unix-socket.c: int unix_stream_listen_gently(const char *path, +@@ unix-socket.c: int unix_stream_listen(const char *path, int bind_successful = 0; - int saved_errno; + int backlog; struct sockaddr_un sa; - struct unix_sockaddr_context ctx; + struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT; @@ unix-socket.c: int unix_stream_listen_gently(const char *path, + ctx.disallow_chdir = opts->disallow_chdir; if (unix_sockaddr_init(&sa, path, &ctx) < 0) - goto fail; + return -1; ## unix-socket.h ## -@@ unix-socket.h: int unix_stream_listen(const char *path); +@@ struct unix_stream_listen_opts { int listen_backlog_size; unsigned int force_unlink_before_bind:1; + unsigned int disallow_chdir:1; }; - int unix_stream_listen_gently(const char *path, + #define UNIX_STREAM_LISTEN_OPTS_INIT \ + { \ + .listen_backlog_size = 5, \ + .force_unlink_before_bind = 1, \ ++ .disallow_chdir = 0, \ + } + + int unix_stream_connect(const char *path); -: ----------- > 13: 2cca15a10ec unix-socket: do not call die in unix_stream_connect() 10: a1b15fb5cb0 ! 14: 72c1c209c38 simple-ipc: add Unix domain socket implementation @@ Commit message Create Unix domain socket based implementation of "simple-ipc". + A set of `ipc_client` routines implement a client library to connect + to an `ipc_server` over a Unix domain socket, send a simple request, + and receive a single response. Clients use blocking IO on the socket. + + A set of `ipc_server` routines implement a thread pool to listen for + and concurrently service client connections. + + The server creates a new Unix domain socket at a known location. If a + socket already exists with that name, the server tries to determine if + another server is already listening on the socket or if the socket is + dead. If socket is busy, the server exits with an error rather than + stealing the socket. If the socket is dead, the server creates a new + one and starts up. + + If while running, the server detects that its socket has been stolen + by another server, it automatically exits. + Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> ## Makefile ## @@ compat/simple-ipc/ipc-unix-socket.c (new) + struct ipc_client_connect_options options + = IPC_CLIENT_CONNECT_OPTIONS_INIT; + struct stat st; -+ int fd_test = -1; ++ struct ipc_client_connection *connection_test = NULL; + + options.wait_if_busy = 0; + options.wait_if_not_found = 0; @@ compat/simple-ipc/ipc-unix-socket.c (new) + * at `path`, doesn't mean it that there is a server listening. + * Ping it to be sure. + */ -+ state = ipc_client_try_connect(path, &options, &fd_test); -+ close(fd_test); ++ state = ipc_client_try_connect(path, &options, &connection_test); ++ ipc_client_close_connection(connection_test); + + return state; +} @@ compat/simple-ipc/ipc-unix-socket.c (new) +enum ipc_active_state ipc_client_try_connect( + const char *path, + const struct ipc_client_connect_options *options, -+ int *pfd) ++ struct ipc_client_connection **p_connection) +{ + enum ipc_active_state state = IPC_STATE__OTHER_ERROR; ++ int fd = -1; + -+ *pfd = -1; ++ *p_connection = NULL; + + trace2_region_enter("ipc-client", "try-connect", NULL); + trace2_data_string("ipc-client", NULL, "try-connect/path", path); + + state = connect_to_server(path, MY_CONNECTION_TIMEOUT_MS, -+ options, pfd); ++ options, &fd); + + trace2_data_intmax("ipc-client", NULL, "try-connect/state", + (intmax_t)state); + trace2_region_leave("ipc-client", "try-connect", NULL); ++ ++ if (state == IPC_STATE__LISTENING) { ++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection)); ++ (*p_connection)->fd = fd; ++ } ++ + return state; +} + -+int ipc_client_send_command_to_fd(int fd, const char *message, -+ struct strbuf *answer) ++void ipc_client_close_connection(struct ipc_client_connection *connection) ++{ ++ if (!connection) ++ return; ++ ++ if (connection->fd != -1) ++ close(connection->fd); ++ ++ free(connection); ++} ++ ++int ipc_client_send_command_to_connection( ++ struct ipc_client_connection *connection, ++ const char *message, struct strbuf *answer) +{ + int ret = 0; + @@ compat/simple-ipc/ipc-unix-socket.c (new) + + trace2_region_enter("ipc-client", "send-command", NULL); + -+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) { ++ if (write_packetized_from_buf2(message, strlen(message), ++ connection->fd, 1, ++ &connection->scratch_write_buffer) < 0) { + ret = error(_("could not send IPC command")); + goto done; + } + -+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) { ++ if (read_packetized_to_strbuf(connection->fd, answer, ++ PACKET_READ_NEVER_DIE) < 0) { + ret = error(_("could not read IPC response")); + goto done; + } @@ compat/simple-ipc/ipc-unix-socket.c (new) + const struct ipc_client_connect_options *options, + const char *message, struct strbuf *answer) +{ -+ int fd; + int ret = -1; + enum ipc_active_state state; ++ struct ipc_client_connection *connection = NULL; + -+ state = ipc_client_try_connect(path, options, &fd); ++ state = ipc_client_try_connect(path, options, &connection); + + if (state != IPC_STATE__LISTENING) + return ret; + -+ ret = ipc_client_send_command_to_fd(fd, message, answer); -+ close(fd); ++ ret = ipc_client_send_command_to_connection(connection, message, answer); ++ ++ ipc_client_close_connection(connection); ++ + return ret; +} + @@ compat/simple-ipc/ipc-unix-socket.c (new) + struct ipc_worker_thread_data *next_thread; + struct ipc_server_data *server_data; + pthread_t pthread_id; ++ struct packet_scratch_space scratch_write_buffer; +}; + +struct ipc_accept_thread_data { + enum magic magic; + struct ipc_server_data *server_data; ++ + int fd_listen; -+ ino_t inode_listen; ++ struct stat st_listen; ++ + int fd_send_shutdown; + int fd_wait_shutdown; + pthread_t pthread_id; @@ compat/simple-ipc/ipc-unix-socket.c (new) +static int do_io_reply_callback(struct ipc_server_reply_data *reply_data, + const char *response, size_t response_len) +{ ++ struct packet_scratch_space *scratch = ++ &reply_data->worker_thread_data->scratch_write_buffer; ++ + if (reply_data->magic != MAGIC_SERVER_REPLY_DATA) + BUG("reply_cb called with wrong instance data"); + -+ return write_packetized_from_buf(response, response_len, -+ reply_data->fd, 0); ++ return write_packetized_from_buf2(response, response_len, ++ reply_data->fd, 0, scratch); +} + +/* A randomly chosen value. */ @@ compat/simple-ipc/ipc-unix-socket.c (new) +static int socket_was_stolen(struct ipc_accept_thread_data *accept_thread_data) +{ + struct stat st; ++ struct stat *ref_st = &accept_thread_data->st_listen; + + if (lstat(accept_thread_data->server_data->buf_path.buf, &st) == -1) + return 1; + -+ if (st.st_ino != accept_thread_data->inode_listen) ++ if (st.st_ino != ref_st->st_ino) + return 1; + ++ /* We might also consider the creation time on some platforms. */ ++ + return 0; +} + @@ compat/simple-ipc/ipc-unix-socket.c (new) + * open/connect using the "socket-inode" pathname. + * + * Unix domain sockets have a fundamental design flaw because the -+ * "socket-inode" persists until the pathname is deleted; closing the listening -+ * "socket-fd" only closes the socket handle/descriptor, it does not delete -+ * the inode/pathname. ++ * "socket-inode" persists until the pathname is deleted; closing the ++ * listening "socket-fd" only closes the socket handle/descriptor, it ++ * does not delete the inode/pathname. + * + * Well-behaving service daemons are expected to also delete the inode + * before shutdown. If a service crashes (or forgets) it can leave @@ compat/simple-ipc/ipc-unix-socket.c (new) + * inode *or* another service instance is already running. + * + * One possible solution is to blindly unlink the inode before -+ * attempting to bind a new socket-fd (and thus create) a new ++ * attempting to bind a new socket-fd and thus create a new + * socket-inode. Then `bind(2)` should always succeed. However, if -+ * there is an existing service instance, it would be orphaned -- -+ * it would still be listening on a socket-fd that is still bound -+ * to an (unlinked) socket-inode, but that socket-inode is no longer ++ * there is an existing service instance, it would be orphaned -- it ++ * would still be listening on a socket-fd that is still bound to an ++ * (unlinked) socket-inode, but that socket-inode is no longer + * associated with the pathname. New client connections will arrive -+ * at our new socket-inode and not the existing server's. (It is upto -+ * the existing server to detect that its socket-inode has been -+ * stolen and shutdown.) ++ * at OUR new socket-inode -- rather than the existing server's ++ * socket. (I suppose it is up to the existing server to detect that ++ * its socket-inode has been stolen and shutdown.) ++ * ++ * Another possible solution is to try to use the ".lock" trick, but ++ * bind() does not have a exclusive-create use bit like open() does, ++ * so we cannot have multiple servers fighting/racing to create the ++ * same file name without having losers lose without knowing that they ++ * lost. ++ * ++ * We try to avoid such stealing and would rather fail to run than ++ * steal an existing socket-inode (because we assume that the ++ * existing server has more context and value to the clients than a ++ * freshly started server). However, if multiple servers are racing ++ * to start, we don't care which one wins -- none of them have any ++ * state information yet worth fighting for. ++ * ++ * Create a "unique" socket-inode (with our PID in it (and assume that ++ * we can force-delete an existing socket with that name)). Stat it ++ * to get the inode number and ctime -- so that we can identify it as ++ * the one we created. Then use the atomic-rename trick to install it ++ * in the real location. (This will unlink an existing socket with ++ * that pathname -- and thereby steal the real socket-inode from an ++ * existing server.) + * -+ * Since this is rather obscure and infrequent, we try to "gently" -+ * create the socket-inode without disturbing an existing service. ++ * Elsewhere, our thread will periodically poll the socket-inode to ++ * see if someone else steals ours. + */ +static int create_listener_socket(const char *path, -+ const struct ipc_server_opts *ipc_opts) ++ const struct ipc_server_opts *ipc_opts, ++ struct stat *st_socket) +{ ++ struct stat st; ++ struct strbuf buf_uniq = STRBUF_INIT; + int fd_listen; -+ int fd_client; -+ struct unix_stream_listen_opts uslg_opts = { -+ .listen_backlog_size = LISTEN_BACKLOG, -+ .force_unlink_before_bind = 0, -+ .disallow_chdir = ipc_opts->uds_disallow_chdir -+ }; -+ -+ trace2_data_string("ipc-server", NULL, "try-listen-gently", path); -+ -+ /* -+ * Assume socket-inode does not exist and try to (gently) -+ * create a new socket-inode on disk at pathname and bind -+ * socket-fd to it. -+ */ -+ fd_listen = unix_stream_listen_gently(path, &uslg_opts); -+ if (fd_listen >= 0) -+ return fd_listen; ++ struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT; + -+ if (errno != EADDRINUSE) -+ return error_errno(_("could not create socket '%s'"), -+ path); -+ -+ trace2_data_string("ipc-server", NULL, "try-detect-server", path); -+ -+ /* -+ * A socket-inode at pathname exists on disk, but we don't -+ * know if it a server is using it or if it is a stale inode. -+ * -+ * poke it with a trivial connection to try to find out. -+ */ -+ fd_client = unix_stream_connect(path); -+ if (fd_client >= 0) { ++ if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) { ++ int fd_client; + /* -+ * An existing service process is alive and accepted our -+ * connection. ++ * A socket-inode at `path` exists on disk, but we ++ * don't know whether it belongs to an active server ++ * or if the last server died without cleaning up. ++ * ++ * Poke it with a trivial connection to try to find out. + */ -+ close(fd_client); -+ -+ /* -+ * We cannot create a new socket-inode here, so we cannot -+ * startup a new server on this pathname. -+ */ -+ errno = EADDRINUSE; -+ return error_errno(_("socket already in use '%s'"), ++ trace2_data_string("ipc-server", NULL, "try-detect-server", + path); ++ fd_client = unix_stream_connect(path); ++ if (fd_client >= 0) { ++ close(fd_client); ++ errno = EADDRINUSE; ++ return error_errno(_("socket already in use '%s'"), ++ path); ++ } + } + -+ trace2_data_string("ipc-server", NULL, "try-listen-force", path); -+ + /* -+ * A socket-inode at pathname exists on disk, but we were not -+ * able to connect to it, so we believe that this is a stale -+ * socket-inode that a previous server forgot to delete. Use -+ * the tradional solution: force unlink it and create a new -+ * one. -+ * -+ * TODO Note that it is possible that another server is -+ * listening, but is either just starting up and not yet -+ * responsive or is stuck somehow. For now, I'm OK with -+ * stealing the socket-inode from it in this case. ++ * Create pathname to our "unique" socket and set it up for ++ * business. + */ -+ uslg_opts.force_unlink_before_bind = 1; -+ fd_listen = unix_stream_listen_gently(path, &uslg_opts); -+ if (fd_listen >= 0) -+ return fd_listen; -+ -+ return error_errno(_("could not force create socket '%s'"), path); -+} -+ -+static int setup_listener_socket(const char *path, ino_t *inode, -+ const struct ipc_server_opts *ipc_opts) -+{ -+ int fd_listen; -+ struct stat st; -+ -+ trace2_region_enter("ipc-server", "create-listener_socket", NULL); -+ fd_listen = create_listener_socket(path, ipc_opts); -+ trace2_region_leave("ipc-server", "create-listener_socket", NULL); ++ strbuf_addf(&buf_uniq, "%s.%d", path, getpid()); + -+ if (fd_listen < 0) -+ return fd_listen; -+ -+ /* -+ * We just bound a socket (descriptor) to a newly created unix -+ * domain socket in the filesystem. Capture the inode number -+ * so we can later detect if/when someone else force-creates a -+ * new socket and effectively steals the path from us. (Which -+ * would leave us listening to a socket that no client could -+ * reach.) -+ */ -+ if (lstat(path, &st) < 0) { ++ uslg_opts.listen_backlog_size = LISTEN_BACKLOG; ++ uslg_opts.force_unlink_before_bind = 1; ++ uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir; ++ fd_listen = unix_stream_listen(buf_uniq.buf, &uslg_opts); ++ if (fd_listen < 0) { + int saved_errno = errno; ++ error_errno(_("could not create listener socket '%s'"), ++ buf_uniq.buf); ++ strbuf_release(&buf_uniq); ++ errno = saved_errno; ++ return -1; ++ } + ++ if (lstat(buf_uniq.buf, st_socket)) { ++ int saved_errno = errno; ++ error_errno(_("could not stat listener socket '%s'"), ++ buf_uniq.buf); + close(fd_listen); -+ unlink(path); -+ ++ unlink(buf_uniq.buf); ++ strbuf_release(&buf_uniq); + errno = saved_errno; -+ return error_errno(_("could not lstat listener socket '%s'"), -+ path); ++ return -1; + } + + if (set_socket_blocking_flag(fd_listen, 1)) { + int saved_errno = errno; -+ ++ error_errno(_("could not set listener socket nonblocking '%s'"), ++ buf_uniq.buf); + close(fd_listen); -+ unlink(path); ++ unlink(buf_uniq.buf); ++ strbuf_release(&buf_uniq); ++ errno = saved_errno; ++ return -1; ++ } + ++ /* ++ * Install it as the "real" socket so that clients will starting ++ * connecting to our socket. ++ */ ++ if (rename(buf_uniq.buf, path)) { ++ int saved_errno = errno; ++ error_errno(_("could not create listener socket '%s'"), path); ++ close(fd_listen); ++ unlink(buf_uniq.buf); ++ strbuf_release(&buf_uniq); + errno = saved_errno; -+ return error_errno(_("making listener socket nonblocking '%s'"), -+ path); ++ return -1; + } + -+ *inode = st.st_ino; ++ strbuf_release(&buf_uniq); ++ trace2_data_string("ipc-server", NULL, "try-listen", path); ++ return fd_listen; ++} ++ ++static int setup_listener_socket(const char *path, struct stat *st_socket, ++ const struct ipc_server_opts *ipc_opts) ++{ ++ int fd_listen; ++ ++ trace2_region_enter("ipc-server", "create-listener_socket", NULL); ++ fd_listen = create_listener_socket(path, ipc_opts, st_socket); ++ trace2_region_leave("ipc-server", "create-listener_socket", NULL); + + return fd_listen; +} @@ compat/simple-ipc/ipc-unix-socket.c (new) +{ + struct ipc_server_data *server_data; + int fd_listen; -+ ino_t inode_listen; ++ struct stat st_listen; + int sv[2]; + int k; + int nr_threads = opts->nr_threads; @@ compat/simple-ipc/ipc-unix-socket.c (new) + path); + } + -+ fd_listen = setup_listener_socket(path, &inode_listen, opts); ++ fd_listen = setup_listener_socket(path, &st_listen, opts); + if (fd_listen < 0) { + int saved_errno = errno; + close(sv[0]); @@ compat/simple-ipc/ipc-unix-socket.c (new) + server_data->accept_thread->magic = MAGIC_ACCEPT_THREAD_DATA; + server_data->accept_thread->server_data = server_data; + server_data->accept_thread->fd_listen = fd_listen; -+ server_data->accept_thread->inode_listen = inode_listen; ++ server_data->accept_thread->st_listen = st_listen; + server_data->accept_thread->fd_send_shutdown = sv[0]; + server_data->accept_thread->fd_wait_shutdown = sv[1]; +