Simple IPC Mechanism
Subsystems (2)
PATCH v5 9/12 unix-socket: disallow chdir() when creating unix domain sockets
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() or unix_stream_connect() is given a socket pathname that is too long to fit in a sockaddr_un structure, it will chdir() to the parent directory of the requested socket pathname, create the socket using a relative pathname, and then chdir() back. This is not thread-safe.
Teach unix_sockaddr_init() to not allow calls to chdir() when this flag is set.
--- builtin/credential-cache.c | 2 +- unix-socket.c | 17 ++++++++++++----- unix-socket.h | 3 ++- 3 files changed, 15 insertions(+), 7 deletions(-)
builtin/credential-cache.c +1 -1
@@ -14,7 +14,7 @@static int send_request (const char *socket,const struct strbuf *out) {int got_data =0 ; -int fd =unix_stream_connect (socket); +int fd =unix_stream_connect (socket,0 );if (fd <0 )return -1 ;
unix-socket.c +12 -5
@@ -30,16 +30,23 @@ static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) }static int unix_sockaddr_init (struct sockaddr_un *sa,const char *path, -struct unix_sockaddr_context *ctx) +struct unix_sockaddr_context *ctx, +int disallow_chdir) {int size =strlen (path) +1 ; ctx->orig_dir =NULL ;if (size >sizeof (sa->sun_path)) { -const char *slash =find_last_dir_sep (path); +const char *slash;const char *dir;struct strbuf cwd =STRBUF_INIT ; +if (disallow_chdir) { + errno =ENAMETOOLONG ; +return -1 ; + } + + slash =find_last_dir_sep (path);if (!slash) { errno =ENAMETOOLONG ;return -1 ;
@@ -65,13 +72,13 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,return 0 ; } -int unix_stream_connect (const char *path) +int unix_stream_connect (const char *path,int disallow_chdir) {int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx; -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
@@ -101,7 +108,7 @@ int unix_stream_listen(const char *path,unlink (path); -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, opts->disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
unix-socket.h +2 -1
@@ -3,11 +3,12 @@struct unix_stream_listen_opts {int listen_backlog_size; +unsigned int disallow_chdir:1 ; };#define UNIX_STREAM_LISTEN_OPTS_INIT { 0 } -int unix_stream_connect (const char *path); +int unix_stream_connect (const char *path,int disallow_chdir);int unix_stream_listen (const char *path,const struct unix_stream_listen_opts *opts);
Signature · 1 line
gitgitgadget
PATCH v5 10/12 unix-stream-server: create unix domain socket under lock
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create a wrapper class for unix_stream_listen() that uses a ".lock" lockfile to create the unix domain socket in a race-free manner.
Unix domain sockets have a fundamental problem on Unix systems because they persist in the filesystem until they are deleted. This is independent of whether a server is actually listening for connections. Well-behaved servers are expected to delete the socket when they shutdown. A new server cannot easily tell if a found socket is attached to an active server or is leftover cruft from a dead server. The traditional solution used by unix_stream_listen() is to force delete the socket pathname and then create a new socket. This solves the latter (cruft) problem, but in the case of the former, it orphans the existing server (by stealing the pathname associated with the socket it is listening on).
We cannot directly use a .lock lockfile to create the socket because the socket is created by bind(2) rather than the open(2) mechanism used by tempfile.c.
As an alternative, we hold a plain lockfile ("<path>.lock") as a mutual exclusion device. Under the lock, we test if an existing socket ("<path>") is has an active server. If not, we create a new socket and begin listening. Then we use "rollback" to delete the lockfile in all cases.
This wrapper code conceptually exists at a higher-level than the core unix_stream_connect() and unix_stream_listen() routines that it consumes. It is isolated in a wrapper class for clarity.
--- Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 2 +- unix-stream-server.c | 128 ++++++++++++++++++++++++++++ unix-stream-server.h | 36 ++++++++ 4 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 unix-stream-server.c create mode 100644 unix-stream-server.h
Makefile +1 -0
@@ -1665,6 +1665,7 @@ ifdef NO_UNIX_SOCKETS BASIC_CFLAGS += -DNO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o + LIB_OBJS += unix-stream-server.o endif ifdef USE_WIN32_IPC
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -243,7 +243,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY ) - list(APPEND compat_SOURCES unix-socket.c) + list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c) endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
unix-stream-server.c new file +128 -0
128 lines not on this page · raw message
unix-stream-server.h new file +36 -0
@@ -0,0 +1,36 @@ +#ifndef UNIX_STREAM_SERVER_H +#define UNIX_STREAM_SERVER_H + +#include "unix-socket.h" + +struct unix_stream_server_socket { +char *path_socket; +struct stat st_socket; +int fd_socket; +}; + +/* +* Create a Unix Domain Socket at the given path under the protection +* of a '.lock' lockfile. +* +* Returns 0 on success, -1 on error, -2 if socket is in use. +*/ +int unix_stream_server__create ( +const char *path, +const struct unix_stream_listen_opts *opts, +long timeout_ms, +struct unix_stream_server_socket **server_socket); + +/* +* Close and delete the socket. +*/ +void unix_stream_server__free ( +struct unix_stream_server_socket *server_socket); + +/* +* Return 1 if the inode of the pathname to our socket changes. +*/ +int unix_stream_server__was_stolen ( +struct unix_stream_server_socket *server_socket); + +#endif /* UNIX_STREAM_SERVER_H */
Signature · 1 line
gitgitgadget
Re: [PATCH v5 10/12] unix-stream-server: create unix domain socket under lock
unix-stream-server.h line 33
Quoted 4 lines · Jeff Hostetler via GitGitGadget
unix-stream-server.h new file +4
-0+struct unix_stream_server_socket { +int unix_stream_server__create( +void unix_stream_server__free( +int unix_stream_server__was_stolen(
I think we reserve __ in our API for names of symbols that normal callers never have to write (both data like git_attr__true[] and functions like cmd_bisect__helper()).
It seems that list-objects-filter.h may have introduced the "name_space" followed by "__" followed by "name" convention, but I am not sure if that is a desirable convention to spread throughout our codebase.
Also "unix_stream_server" is quite a mouthful. Perhaps abbreviate it to uss_ or something? I dunno if that is too short and invite confusion with other kinds of uss.
PATCH v5 11/12 simple-ipc: add Unix domain socket implementation
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
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.
--- Makefile | 2 + compat/simple-ipc/ipc-unix-socket.c | 986 ++++++++++++++++++++++++++++ contrib/buildsystems/CMakeLists.txt | 2 + simple-ipc.h | 13 +- 4 files changed, 1002 insertions(+), 1 deletion(-) create mode 100644 compat/simple-ipc/ipc-unix-socket.c
Makefile +2 -0
@@ -1666,6 +1666,8 @@ ifdef NO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o LIB_OBJS += unix-stream-server.o + LIB_OBJS += compat/simple-ipc/ipc-shared.o + LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o endif ifdef USE_WIN32_IPC
compat/simple-ipc/ipc-unix-socket.c new file +986 -0
986 lines not on this page · raw message
contrib/buildsystems/CMakeLists.txt +2 -0
@@ -248,6 +248,8 @@ endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows") list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c) +else() + list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c) endif() set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})
simple-ipc.h +12 -1
@@ -5,7 +5,7 @@* See Documentation/technical/api-simple-ipc.txt */ -#if defined(GIT_WINDOWS_NATIVE ) +#if defined(GIT_WINDOWS_NATIVE ) || !defined(NO_UNIX_SOCKETS )#define SUPPORTS_SIMPLE_IPC #endif
@@ -62,11 +62,17 @@ struct ipc_client_connect_options {* the service and need to wait for it to become ready. */ unsigned int wait_if_not_found:1; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ + unsigned int uds_disallow_chdir:1; };#define IPC_CLIENT_CONNECT_OPTIONS_INIT { \ .wait_if_busy = 0, \ .wait_if_not_found = 0, \ + .uds_disallow_chdir = 0, \ }/*
@@ -159,6 +165,11 @@ struct ipc_server_data;struct ipc_server_opts {int nr_threads; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ +unsigned int uds_disallow_chdir:1 ; };/*
Signature · 1 line
gitgitgadget
Re: [PATCH v5 11/12] simple-ipc: add Unix domain socket implementation
compat/simple-ipc/ipc-unix-socket.c line 52
Quoted 4 lines · Jeff Hostetler via GitGitGadget
compat/simple-ipc/ipc-unix-socket.c new file +4
-0+/* +* This value was chosen at random. +*/ +#define WAIT_STEP_MS (50)
... and never used. Is this supposed to be used as the hardcoded value 50 below ...
compat/simple-ipc/ipc-unix-socket.c line 64
Quoted 12 lines
compat/simple-ipc/ipc-unix-socket.c new file +12
-0+ +/* +* Try to connect to the server. If the server is just starting up or +* is very busy, we may not get a connection the first time. +*/ +static enum ipc_active_state connect_to_server ( +const char *path, +int timeout_ms, +const struct ipc_client_connect_options *options, +int *pfd) +{ +int wait_ms =50 ;
... here?
Quoted 13 lines
compat/simple-ipc/ipc-unix-socket.c new file +13
-0+int k; + + *pfd =-1 ; + +for (k =0 ; k < timeout_ms; k += wait_ms) { +int fd =unix_stream_connect (path, options->uds_disallow_chdir); + +if (fd !=-1 ) { + *pfd = fd; +return IPC_STATE__LISTENING ; + } + +if (errno ==ENOENT ) {
compat/simple-ipc/ipc-unix-socket.c line 101
Quoted 10 lines
compat/simple-ipc/ipc-unix-socket.c new file +10
-0+if (!options->wait_if_not_found) +return IPC_STATE__PATH_NOT_FOUND ; + + goto sleep_and_try_again; + } + ... +return IPC_STATE__OTHER_ERROR ; + + sleep_and_try_again: +sleep_millisec (wait_ms);
Or, since there is nothing like exponential back-off implemented here which may want to modify wait_ms variable, perhaps use the constant directly here and where k is incremented?
compat/simple-ipc/ipc-unix-socket.c line 110
Quoted 4 lines
compat/simple-ipc/ipc-unix-socket.c new file +4
-0+/* +* A randomly chosen timeout value. +*/ +#define MY_CONNECTION_TIMEOUT_MS (1000)
Even if it may have been "randomly chosen", there should be some criteria to judge if the value is sensible, right? IOW, I have a suspicion that I would regret if I randomly chose 5 (or 3600000) instead of 1000. How would we figure that 1000 acceptable but not 5?
Perhaps explain that criterion here, e.g. "... value that ought to be long enough to establish connection locally as long as the box is not loaded unusably heavily" or something?
Re: [PATCH v5 11/12] simple-ipc: add Unix domain socket implementation
Quoted 10 lines · Junio C Hamano
compat/simple-ipc/ipc-unix-socket.c line 52
Quoted 4 lines · Jeff Hostetler via GitGitGadget
compat/simple-ipc/ipc-unix-socket.c new file +4
-0+/* +* This value was chosen at random. +*/ +#define WAIT_STEP_MS (50)... and never used. Is this supposed to be used as the hardcoded value 50 below ...
Let me fix that before you move this to "next".
Looks like I just missed that one.
Quoted 54 lines
compat/simple-ipc/ipc-unix-socket.c line 64
Quoted 12 lines
compat/simple-ipc/ipc-unix-socket.c new file +12
-0+ +/* +* Try to connect to the server. If the server is just starting up or +* is very busy, we may not get a connection the first time. +*/ +static enum ipc_active_state connect_to_server ( +const char *path, +int timeout_ms, +const struct ipc_client_connect_options *options, +int *pfd) +{ +int wait_ms =50 ;... here?
Quoted 13 lines
compat/simple-ipc/ipc-unix-socket.c new file +13
-0+int k; + + *pfd =-1 ; + +for (k =0 ; k < timeout_ms; k += wait_ms) { +int fd =unix_stream_connect (path, options->uds_disallow_chdir); + +if (fd !=-1 ) { + *pfd = fd; +return IPC_STATE__LISTENING ; + } + +if (errno ==ENOENT ) {compat/simple-ipc/ipc-unix-socket.c line 101
Quoted 10 lines
compat/simple-ipc/ipc-unix-socket.c new file +10
-0+if (!options->wait_if_not_found) +return IPC_STATE__PATH_NOT_FOUND ; + + goto sleep_and_try_again; + } + ... +return IPC_STATE__OTHER_ERROR ; + + sleep_and_try_again: +sleep_millisec (wait_ms);Or, since there is nothing like exponential back-off implemented here which may want to modify
wait_msvariable, perhaps use the constant directly here and where k is incremented?compat/simple-ipc/ipc-unix-socket.c line 110
Quoted 4 lines
compat/simple-ipc/ipc-unix-socket.c new file +4
-0+/* +* A randomly chosen timeout value. +*/ +#define MY_CONNECTION_TIMEOUT_MS (1000)Even if it may have been "randomly chosen", there should be some criteria to judge if the value is sensible, right? IOW, I have a suspicion that I would regret if I randomly chose 5 (or 3600000) instead of 1000. How would we figure that 1000 acceptable but not 5?
Quoted 4 lines
Perhaps explain that criterion here, e.g. "... value that ought to be long enough to establish connection locally as long as the box is not loaded unusably heavily" or something?
Will do. Thanks!
Jeff
PATCH v5 12/12 t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create t0052-simple-ipc.sh with unit tests for the "simple-ipc" mechanism.
Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc" functions.
When the tool is invoked with "run-daemon", it runs a server to listen for "simple-ipc" connections on a test socket or named pipe and responds to a set of commands to exercise/stress the communication setup.
When the tool is invoked with "start-daemon", it spawns a "run-daemon" command in the background and waits for the server to become ready before exiting. (This helps make unit tests in t0052 more predictable and avoids the need for arbitrary sleeps in the test script.)
The tool also has a series of client "send" commands to send commands and data to a server instance.
--- Makefile | 1 + t/helper/test-simple-ipc.c | 787 +++++++++++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 ++++++ 5 files changed, 912 insertions(+) create mode 100644 t/helper/test-simple-ipc.c create mode 100755 t/t0052-simple-ipc.sh
Makefile +1 -0
@@ -734,6 +734,7 @@ TEST_BUILTINS_OBJS += test-serve-v2.o TEST_BUILTINS_OBJS += test-sha1.o TEST_BUILTINS_OBJS += test-sha256.o TEST_BUILTINS_OBJS += test-sigchain.o +TEST_BUILTINS_OBJS += test-simple-ipc.o TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o
t/helper/test-simple-ipc.c new file +787 -0
787 lines not on this page · raw message
t/helper/test-tool.c +1 -0
@@ -65,6 +65,7 @@ static struct test_cmd cmds[] = { { "sha1", cmd__sha1 }, { "sha256", cmd__sha256 }, { "sigchain", cmd__sigchain }, + { "simple-ipc", cmd__simple_ipc }, { "strcmp-offset", cmd__strcmp_offset }, { "string-list", cmd__string_list }, { "submodule-config", cmd__submodule_config },
t/helper/test-tool.h +1 -0
@@ -55,6 +55,7 @@ int cmd__sha1(int argc, const char **argv);int cmd__oid_array (int argc,const char **argv);int cmd__sha256 (int argc,const char **argv);int cmd__sigchain (int argc,const char **argv); +int cmd__simple_ipc (int argc,const char **argv);int cmd__strcmp_offset (int argc,const char **argv);int cmd__string_list (int argc,const char **argv);int cmd__submodule_config (int argc,const char **argv);
t/t0052-simple-ipc.sh new file +122 -0
122 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 2 lines · Jeff Hostetler via GitGitGadget
.... I think the combined
result is better long term than preserving them as two sequential series.
Yup, I think that is a sensible thing to do, too. Just kick the one in 'next' out by reverting them, and queue a cleaned-up series to be merged to 'next' once the upcoming release is out.
Thanks.
v6 cover Simple IPC Mechanism
In reply to Jeff Hostetler via GitGitGadgetHere is V6 of my "Simple IPC" series. This version addresses comments from last week on V5. This includes:
1. Removing "perf and" in pkt-line.c
2. Better comments to describe the various timeout #define's.
3. Remove the double-underscore and shorten the "unix_stream_server" prefix.
Thanks Jeff
Jeff Hostetler (9):
pkt-line: eliminate the need for static buffer in
packet_write_gently()
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
unix-socket: eliminate static unix_stream_socket() helper function
unix-socket: add backlog size option to unix_stream_listen()
unix-socket: disallow chdir() when creating unix domain sockets
unix-stream-server: create unix domain socket under lock
simple-ipc: add Unix domain socket implementation
t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Johannes Schindelin (3):
pkt-line: do not issue flush packets in write_packetized_*()
pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
pkt-line: add options argument to read_packetized_to_strbuf()
Documentation/technical/api-simple-ipc.txt | 105 ++ Makefile | 9 + builtin/credential-cache--daemon.c | 3 +- builtin/credential-cache.c | 2 +- compat/simple-ipc/ipc-shared.c | 28 + compat/simple-ipc/ipc-unix-socket.c | 1000 ++++++++++++++++++++ compat/simple-ipc/ipc-win32.c | 751 +++++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 8 +- convert.c | 11 +- pkt-line.c | 59 +- pkt-line.h | 17 +- simple-ipc.h | 239 +++++ t/helper/test-simple-ipc.c | 787 +++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 +++ unix-socket.c | 53 +- unix-socket.h | 12 +- unix-stream-server.c | 125 +++ unix-stream-server.h | 33 + 21 files changed, 3316 insertions(+), 52 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 create mode 100644 unix-stream-server.c create mode 100644 unix-stream-server.h
base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/766
Range-diff vs v5:
1: 311ea4a5cd71 ! 1: fe35dc3d292d pkt-line: eliminate the need for static buffer in packet_write_gently()
@@ pkt-line.c: int packet_write_fmt_gently(int fd, const char *fmt, ...)
+ set_packet_header(header, packet_size);
+
+ /*
-+ * Write the header and the buffer in 2 parts so that we do not need
-+ * to allocate a buffer or rely on a static buffer. This avoids perf
-+ * and multi-threading issues.
++ * Write the header and the buffer in 2 parts so that we do
++ * not need to allocate a buffer or rely on a static buffer.
++ * This also avoids putting a large buffer on the stack which
++ * might have multi-threading issues.
+ */
+
+ if (write_in_full(fd_out, header, 4) < 0 ||
2: 25157c1f4873 = 2: de11b3036148 pkt-line: do not issue flush packets in write_packetized_*()
3: af3d13113bc9 = 3: 3718da39da30 pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
4: b73e66a69b61 = 4: b43df7ad0b7a pkt-line: add options argument to read_packetized_to_strbuf()
5: 1ae99d824a21 = 5: f829feb2aa93 simple-ipc: design documentation for new IPC mechanism
6: 8b3ce40e4538 = 6: 58c3fb7cd776 simple-ipc: add win32 implementation
7: 34df1af98e5b = 7: 4e8c352fb366 unix-socket: eliminate static unix_stream_socket() helper function
8: d6ff6e0e050a = 8: 3b71f52d8628 unix-socket: add backlog size option to unix_stream_listen()
9: 21b8d3c63dbf = 9: 5972a198361c unix-socket: disallow chdir() when creating unix domain sockets
10: 1ee9de55a106 ! 10: 02c885fd623d unix-stream-server: create unix domain socket under lock
@@ unix-stream-server.c (new)
+ return 0;
+}
+
-+int unix_stream_server__create(
-+ const char *path,
-+ const struct unix_stream_listen_opts *opts,
-+ long timeout_ms,
-+ struct unix_stream_server_socket **new_server_socket)
++int unix_ss_create(const char *path,
++ const struct unix_stream_listen_opts *opts,
++ long timeout_ms,
++ struct unix_ss_socket **new_server_socket)
+{
+ struct lock_file lock = LOCK_INIT;
+ int fd_socket;
-+ struct unix_stream_server_socket *server_socket;
++ struct unix_ss_socket *server_socket;
+
+ *new_server_socket = NULL;
+
@@ unix-stream-server.c (new)
+ return 0;
+}
+
-+void unix_stream_server__free(
-+ struct unix_stream_server_socket *server_socket)
++void unix_ss_free(struct unix_ss_socket *server_socket)
+{
+ if (!server_socket)
+ return;
+
+ if (server_socket->fd_socket >= 0) {
-+ if (!unix_stream_server__was_stolen(server_socket))
++ if (!unix_ss_was_stolen(server_socket))
+ unlink(server_socket->path_socket);
+ close(server_socket->fd_socket);
+ }
@@ unix-stream-server.c (new)
+ free(server_socket);
+}
+
-+int unix_stream_server__was_stolen(
-+ struct unix_stream_server_socket *server_socket)
++int unix_ss_was_stolen(struct unix_ss_socket *server_socket)
+{
+ struct stat st_now;
+
@@ unix-stream-server.h (new)
+
+#include "unix-socket.h"
+
-+struct unix_stream_server_socket {
++struct unix_ss_socket {
+ char *path_socket;
+ struct stat st_socket;
+ int fd_socket;
@@ unix-stream-server.h (new)
+ *
+ * Returns 0 on success, -1 on error, -2 if socket is in use.
+ */
-+int unix_stream_server__create(
-+ const char *path,
-+ const struct unix_stream_listen_opts *opts,
-+ long timeout_ms,
-+ struct unix_stream_server_socket **server_socket);
++int unix_ss_create(const char *path,
++ const struct unix_stream_listen_opts *opts,
++ long timeout_ms,
++ struct unix_ss_socket **server_socket);
+
+/*
+ * Close and delete the socket.
+ */
-+void unix_stream_server__free(
-+ struct unix_stream_server_socket *server_socket);
++void unix_ss_free(struct unix_ss_socket *server_socket);
+
+/*
+ * Return 1 if the inode of the pathname to our socket changes.
+ */
-+int unix_stream_server__was_stolen(
-+ struct unix_stream_server_socket *server_socket);
++int unix_ss_was_stolen(struct unix_ss_socket *server_socket);
+
+#endif /* UNIX_STREAM_SERVER_H */
11: f2e3b046cc8f ! 11: 4c2199231d05 simple-ipc: add Unix domain socket implementation
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+}
+
+/*
-+ * This value was chosen at random.
++ * Retry frequency when trying to connect to a server.
++ *
++ * This value should be short enough that we don't seriously delay our
++ * caller, but not fast enough that our spinning puts pressure on the
++ * system.
+ */
+#define WAIT_STEP_MS (50)
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ const struct ipc_client_connect_options *options,
+ int *pfd)
+{
-+ int wait_ms = 50;
+ int k;
+
+ *pfd = -1;
+
-+ for (k = 0; k < timeout_ms; k += wait_ms) {
++ for (k = 0; k < timeout_ms; k += WAIT_STEP_MS) {
+ int fd = unix_stream_connect(path, options->uds_disallow_chdir);
+
+ if (fd != -1) {
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ return IPC_STATE__OTHER_ERROR;
+
+ sleep_and_try_again:
-+ sleep_millisec(wait_ms);
++ sleep_millisec(WAIT_STEP_MS);
+ }
+
+ return IPC_STATE__NOT_LISTENING;
+}
+
+/*
-+ * A randomly chosen timeout value.
++ * The total amount of time that we are willing to wait when trying to
++ * connect to a server.
++ *
++ * When the server is first started, it might take a little while for
++ * it to become ready to service requests. Likewise, the server may
++ * be very (temporarily) busy and not respond to our connections.
++ *
++ * We should gracefully and silently handle those conditions and try
++ * again for a reasonable time period.
++ *
++ * The value chosen here should be long enough for the server
++ * to reliably heal from the above conditions.
+ */
+#define MY_CONNECTION_TIMEOUT_MS (1000)
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ enum magic magic;
+ struct ipc_server_data *server_data;
+
-+ struct unix_stream_server_socket *server_socket;
++ struct unix_ss_socket *server_socket;
+
+ int fd_send_shutdown;
+ int fd_wait_shutdown;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * will be routed elsewhere and we silently starve.
+ * If that happens, just queue a shutdown.
+ */
-+ if (unix_stream_server__was_stolen(
++ if (unix_ss_was_stolen(
+ accept_thread_data->server_socket)) {
+ trace2_data_string("ipc-accept", NULL,
+ "queue_stop_async",
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int create_listener_socket(
+ const char *path,
+ const struct ipc_server_opts *ipc_opts,
-+ struct unix_stream_server_socket **new_server_socket)
++ struct unix_ss_socket **new_server_socket)
+{
-+ struct unix_stream_server_socket *server_socket = NULL;
++ struct unix_ss_socket *server_socket = NULL;
+ struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+ int ret;
+
+ uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
+ uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
+
-+ ret = unix_stream_server__create(path, &uslg_opts, -1, &server_socket);
++ ret = unix_ss_create(path, &uslg_opts, -1, &server_socket);
+ if (ret)
+ return ret;
+
+ if (set_socket_blocking_flag(server_socket->fd_socket, 1)) {
+ int saved_errno = errno;
-+ unix_stream_server__free(server_socket);
++ unix_ss_free(server_socket);
+ errno = saved_errno;
+ return -1;
+ }
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int setup_listener_socket(
+ const char *path,
+ const struct ipc_server_opts *ipc_opts,
-+ struct unix_stream_server_socket **new_server_socket)
++ struct unix_ss_socket **new_server_socket)
+{
+ int ret, saved_errno;
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ ipc_server_application_cb *application_cb,
+ void *application_data)
+{
-+ struct unix_stream_server_socket *server_socket = NULL;
++ struct unix_ss_socket *server_socket = NULL;
+ struct ipc_server_data *server_data;
+ int sv[2];
+ int k;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+
+ accept_thread_data = server_data->accept_thread;
+ if (accept_thread_data) {
-+ unix_stream_server__free(accept_thread_data->server_socket);
++ unix_ss_free(accept_thread_data->server_socket);
+
+ if (accept_thread_data->fd_send_shutdown != -1)
+ close(accept_thread_data->fd_send_shutdown); 12: 6ccc7472096f = 12: 132b6f3271be t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Signature · 1 line
gitgitgadget
PATCH v6 1/12 pkt-line: eliminate the need for static buffer in packet_write_gently()
From: Jeff Hostetler <[email protected]>
Teach packet_write_gently() to write the pkt-line header and the actual buffer in 2 separate calls to write_in_full() and avoid the need for a static buffer, thread-safe scratch space, or an excessively large stack buffer.
Change write_packetized_from_fd() to allocate a temporary buffer rather than using a static buffer to avoid similar issues here.
These changes are intended to make it easier to use pkt-line routines in a multi-threaded context with multiple concurrent writers writing to different streams.
--- pkt-line.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
pkt-line.c +20 -8
@@ -196,17 +196,26 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)static int packet_write_gently (const int fd_out,const char *buf,size_t size) { -static char packet_write_buffer[LARGE_PACKET_MAX ]; +char header[4 ];size_t packet_size; -if (size >sizeof (packet_write_buffer) -4 ) +if (size >LARGE_PACKET_DATA_MAX )return error (_ ("packet write failed - data exceeds max packet size" ));packet_trace (buf, size,1 ); packet_size = size +4 ; -set_packet_header (packet_write_buffer, packet_size); -memcpy (packet_write_buffer +4 , buf, size); -if (write_in_full (fd_out, packet_write_buffer, packet_size) <0 ) + +set_packet_header (header, packet_size); + +/* +* Write the header and the buffer in 2 parts so that we do +* not need to allocate a buffer or rely on a static buffer. +* This also avoids putting a large buffer on the stack which +* might have multi-threading issues. +*/ + +if (write_in_full (fd_out, header,4 ) <0 || +write_in_full (fd_out, buf, size) <0 )return error (_ ("packet write failed" ));return 0 ; }
@@ -244,20 +253,23 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)int write_packetized_from_fd (int fd_in,int fd_out) { -static char buf[LARGE_PACKET_DATA_MAX ]; +char *buf =xmalloc (LARGE_PACKET_DATA_MAX );int err =0 ;ssize_t bytes_to_write;while (!err) { - bytes_to_write =xread (fd_in, buf,sizeof (buf)); -if (bytes_to_write <0 ) + bytes_to_write =xread (fd_in, buf,LARGE_PACKET_DATA_MAX ); +if (bytes_to_write <0 ) { +free (buf);return COPY_READ_ERROR ; + }if (bytes_to_write ==0 )break ; err =packet_write_gently (fd_out, buf, bytes_to_write); }if (!err) err =packet_flush_gently (fd_out); +free (buf);return err; }
Signature · 1 line
gitgitgadget
PATCH v6 2/12 pkt-line: do not issue flush packets in write_packetized_*()
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Remove the packet_flush_gently() call in write_packetized_from_buf() and write_packetized_from_fd() and require the caller to call it if desired. Rename both functions to write_packetized_from_*_no_flush()` to prevent later merge accidents.
write_packetized_from_buf() currently only has one caller:apply_multi_file_filter() in convert.c. It always wants a flush packet to be written after writing the payload.
However, we are about to introduce a caller that wants to write many packets before a final flush packet, so let's make the caller responsible for emitting the flush packet.
--- convert.c | 8 ++++++-- pkt-line.c | 8 ++------ pkt-line.h | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-)
convert.c +6 -2
@@ -884,9 +884,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len goto done;if (fd >=0 ) - err =write_packetized_from_fd (fd, process->in); + err =write_packetized_from_fd_no_flush (fd, process->in);else - err =write_packetized_from_buf (src, len, process->in); + err =write_packetized_from_buf_no_flush (src, len, process->in); +if (err) + goto done; + + err =packet_flush_gently (process->in);if (err) goto done;
pkt-line.c +2 -6
@@ -251,7 +251,7 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)packet_trace (data, len,1 ); } -int write_packetized_from_fd (int fd_in,int fd_out) +int write_packetized_from_fd_no_flush (int fd_in,int fd_out) {char *buf =xmalloc (LARGE_PACKET_DATA_MAX );int err =0 ;
@@ -267,13 +267,11 @@ int write_packetized_from_fd(int fd_in, int fd_out)break ; err =packet_write_gently (fd_out, buf, bytes_to_write); } -if (!err) - err =packet_flush_gently (fd_out);free (buf);return err; } -int write_packetized_from_buf (const char *src_in,size_t len,int fd_out) +int write_packetized_from_buf_no_flush (const char *src_in,size_t len,int fd_out) {int err =0 ;size_t bytes_written =0 ;
@@ -289,8 +287,6 @@ 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); bytes_written += bytes_to_write; } -if (!err) - err =packet_flush_gently (fd_out);return err; }
pkt-line.h +2 -2
@@ -32,8 +32,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((fvoid packet_buf_write_len (struct strbuf *buf,const char *data,size_t len);int packet_flush_gently (int fd);int packet_write_fmt_gently (int fd,const char *fmt, ...) __attribute__((format (printf,2 ,3 ))); -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_fd_no_flush (int fd_in,int fd_out); +int write_packetized_from_buf_no_flush (const char *src_in,size_t len,int fd_out);/* * Read a packetized line into the buffer, which must be at least size bytes
Signature · 1 line
gitgitgadget
PATCH v6 3/12 pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Introduce PACKET_READ_GENTLE_ON_READ_ERROR option to help libify the packet readers.
So far, the (possibly indirect) callers of get_packet_data() can ask that function to return an error instead of die()ing upon end-of-file. However, random read errors will still cause the process to die.
So let's introduce an explicit option to tell the packet reader machinery to please be nice and only return an error on read errors.
This change prepares pkt-line for use by long-running daemon processes. Such processes should be able to serve multiple concurrent clients and and survive random IO errors. If there is an error on one connection, a daemon should be able to drop that connection and continue serving existing and future connections.
This ability will be used by a Git-aware "Builtin FSMonitor" feature in a later patch series.
--- pkt-line.c | 19 +++++++++++++++++-- pkt-line.h | 11 ++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)
pkt-line.c +17 -2
@@ -306,8 +306,11 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size, *src_size -= ret; }else { ret =read_in_full (fd, dst, size); -if (ret <0 ) +if (ret <0 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error_errno (_ ("read error" ));die_errno (_ ("read error" )); + } }/* And complain if we didn't get enough bytes to satisfy the read. */
@@ -315,6 +318,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,if (options &PACKET_READ_GENTLE_ON_EOF )return -1 ; +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("the remote end hung up unexpectedly" ));die (_ ("the remote end hung up unexpectedly" )); }
@@ -343,6 +348,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, len =packet_length (linelen);if (len <0 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length " +"character: %.4s" ), linelen);die (_ ("protocol error: bad line length character: %.4s" ), linelen); }else if (!len) {packet_trace ("0000" ,4 ,0 );
@@ -357,12 +365,19 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, *pktlen =0 ;return PACKET_READ_RESPONSE_END ; }else if (len <4 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length %d" ), + len);die (_ ("protocol error: bad line length %d" ), len); } len -=4 ; -if ((unsigned )len >= size) +if ((unsigned )len >= size) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length %d" ), + len);die (_ ("protocol error: bad line length %d" ), len); + }if (get_packet_data (fd, src_buffer, src_len, buffer, len, options) <0 ) { *pktlen =-1 ;
pkt-line.h +8 -3
@@ -68,10 +68,15 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou* * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an * ERR packet. +* +* If options contains PACKET_READ_GENTLE_ON_READ_ERROR, we will not die +* on read errors, but instead return -1. However, we may still die on an +* ERR packet (if requested). */ -#define PACKET_READ_GENTLE_ON_EOF (1u<<0) -#define PACKET_READ_CHOMP_NEWLINE (1u<<1) -#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2) +#define PACKET_READ_GENTLE_ON_EOF (1u<<0) +#define PACKET_READ_CHOMP_NEWLINE (1u<<1) +#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2) +#define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)int packet_read (int fd,char **src_buffer,size_t *src_len,char *buffer,unsigned size,int options);
Signature · 1 line
gitgitgadget
PATCH v6 4/12 pkt-line: add options argument to read_packetized_to_strbuf()
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Update the calling sequence of read_packetized_to_strbuf() to take an options argument and not assume a fixed set of options. Update the only existing caller accordingly to explicitly pass the formerly-assumed flags.
The read_packetized_to_strbuf() function calls packet_read() with a fixed set of assumed options (PACKET_READ_GENTLE_ON_EOF). This assumption has been fine for the single existing caller apply_multi_file_filter() in convert.c.
In a later commit we would like to add other callers to read_packetized_to_strbuf() that need a different set of options.
--- convert.c | 3 ++- pkt-line.c | 4 ++-- pkt-line.h | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)
convert.c +2 -1
@@ -907,7 +907,8 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t lenif (err) goto done; - err =read_packetized_to_strbuf (process->out, &nbuf) <0 ; + err =read_packetized_to_strbuf (process->out, &nbuf, +PACKET_READ_GENTLE_ON_EOF ) <0 ;if (err) goto done;
pkt-line.c +2 -2
@@ -444,7 +444,7 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)return packet_read_line_generic (-1 , src, src_len, dst_len); } -ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out) +ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out,int options) {int packet_len;
@@ -460,7 +460,7 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)* that there is already room for the extra byte. */ sb_out->buf + sb_out->len,LARGE_PACKET_DATA_MAX +1 , - PACKET_READ_GENTLE_ON_EOF); + options);if (packet_len <=0 )break ; sb_out->len += packet_len;
pkt-line.h +1 -1
@@ -136,7 +136,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);/* * Reads a stream of variable sized packets until a flush packet is detected. */ -ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out); +ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out,int options);/* * Receive multiplexed output stream over git native protocol.
Signature · 1 line
gitgitgadget
PATCH v6 5/12 simple-ipc: design documentation for new IPC mechanism
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Brief design documentation for new IPC mechanism allowing foreground Git client to talk with an existing daemon process at a known location using a named pipe or unix domain socket.
--- Documentation/technical/api-simple-ipc.txt | 105 +++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 Documentation/technical/api-simple-ipc.txt
Documentation/technical/api-simple-ipc.txt new file +105 -0
@@ -0,0 +1,105 @@ +Simple-IPC API +============== + +The Simple-IPC API is a collection of `ipc_` prefixed library routines +and a basic communication protocol that allow an IPC-client process to +send an application-specific IPC-request message to an IPC-server +process and receive an application-specific IPC-response message. + +Communication occurs over a named pipe on Windows and a Unix domain +socket on other platforms. IPC-clients and IPC-servers rendezvous at +a previously agreed-to application-specific pathname (which is outside +the scope of this design) that is local to the computer system. + +The IPC-server routines within the server application process create a +thread pool to listen for connections and receive request messages +from multiple concurrent IPC-clients. When received, these messages +are dispatched up to the server application callbacks for handling. +IPC-server routines then incrementally relay responses back to the +IPC-client. + +The IPC-client routines within a client application process connect +to the IPC-server and send a request message and wait for a response. +When received, the response is returned back the caller. + +For example, the `fsmonitor--daemon` feature will be built as a server +application on top of the IPC-server library routines. It will have +threads watching for file system events and a thread pool waiting for +client connections. Clients, such as `git status` will request a list +of file system events since a point in time and the server will +respond with a list of changed files and directories. The formats of +the request and response are application-specific; the IPC-client and +IPC-server routines treat them as opaque byte streams. + + +Comparison with sub-process model +--------------------------------- + +The Simple-IPC mechanism differs from the existing `sub-process.c` +model (Documentation/technical/long-running-process-protocol.txt) and +used by applications like Git-LFS. In the LFS-style sub-process model +the helper is started by the foreground process, communication happens +via a pair of file descriptors bound to the stdin/stdout of the +sub-process, the sub-process only serves the current foreground +process, and the sub-process exits when the foreground process +terminates. + +In the Simple-IPC model the server is a very long-running service. It +can service many clients at the same time and has a private socket or +named pipe connection to each active client. It might be started +(on-demand) by the current client process or it might have been +started by a previous client or by the OS at boot time. The server +process is not associated with a terminal and it persists after +clients terminate. Clients do not have access to the stdin/stdout of +the server process and therefore must communicate over sockets or +named pipes. + + +Server startup and shutdown +--------------------------- + +How an application server based upon IPC-server is started is also +outside the scope of the Simple-IPC design and is a property of the +application using it. For example, the server might be started or +restarted during routine maintenance operations, or it might be +started as a system service during the system boot-up sequence, or it +might be started on-demand by a foreground Git command when needed. + +Similarly, server shutdown is a property of the application using +the simple-ipc routines. For example, the server might decide to +shutdown when idle or only upon explicit request. + + +Simple-IPC protocol +------------------- + +The Simple-IPC protocol consists of a single request message from the +client and an optional response message from the server. Both the +client and server messages are unlimited in length and are terminated +with a flush packet. + +The pkt-line routines (Documentation/technical/protocol-common.txt) +are used to simplify buffer management during message generation, +transmission, and reception. A flush packet is used to mark the end +of the message. This allows the sender to incrementally generate and +transmit the message. It allows the receiver to incrementally receive +the message in chunks and to know when they have received the entire +message. + +The actual byte format of the client request and server response +messages are application specific. The IPC layer transmits and +receives them as opaque byte buffers without any concern for the +content within. It is the job of the calling application layer to +understand the contents of the request and response messages. + + +Summary +------- + +Conceptually, the Simple-IPC protocol is similar to an HTTP REST +request. Clients connect, make an application-specific and +stateless request, receive an application-specific +response, and disconnect. It is a one round trip facility for +querying the server. The Simple-IPC routines hide the socket, +named pipe, and thread pool details and allow the application +layer to focus on the application at hand.
Signature · 1 line
gitgitgadget
PATCH v6 6/12 simple-ipc: add win32 implementation
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create Windows implementation of "simple-ipc" using named pipes.
--- Makefile | 5 + compat/simple-ipc/ipc-shared.c | 28 ++ compat/simple-ipc/ipc-win32.c | 751 ++++++++++++++++++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 4 + simple-ipc.h | 228 +++++++++ 6 files changed, 1018 insertions(+) create mode 100644 compat/simple-ipc/ipc-shared.c create mode 100644 compat/simple-ipc/ipc-win32.c create mode 100644 simple-ipc.h
Makefile +5 -0
@@ -1667,6 +1667,11 @@ else LIB_OBJS += unix-socket.o endif +ifdef USE_WIN32_IPC + LIB_OBJS += compat/simple-ipc/ipc-shared.o + LIB_OBJS += compat/simple-ipc/ipc-win32.o +endif + ifdef NO_ICONV BASIC_CFLAGS += -DNO_ICONV endif
compat/simple-ipc/ipc-shared.c new file +28 -0
@@ -0,0 +1,28 @@ +#include "cache.h" +#include "simple-ipc.h" +#include "strbuf.h" +#include "pkt-line.h" +#include "thread-utils.h" + +#ifdef SUPPORTS_SIMPLE_IPC + +int ipc_server_run (const char *path,const struct ipc_server_opts *opts, +ipc_server_application_cb *application_cb, +void *application_data) +{ +struct ipc_server_data *server_data =NULL ; +int ret; + + ret =ipc_server_run_async (&server_data, path, opts, + application_cb, application_data); +if (ret) +return ret; + + ret =ipc_server_await (server_data); + +ipc_server_free (server_data); + +return ret; +} + +#endif /* SUPPORTS_SIMPLE_IPC */
compat/simple-ipc/ipc-win32.c new file +751 -0
751 lines not on this page · raw message
config.mak.uname +2 -0
@@ -421,6 +421,7 @@ ifeq ($(uname_S),Windows) RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease + USE_WIN32_IPC = YesPlease USE_WIN32_MMAP = YesPlease MMAP_PREVENTS_DELETE = UnfortunatelyYes # USE_NED_ALLOCATOR = YesPlease
@@ -597,6 +598,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease + USE_WIN32_IPC = YesPlease USE_WIN32_MMAP = YesPlease MMAP_PREVENTS_DELETE = UnfortunatelyYes USE_NED_ALLOCATOR = YesPlease
contrib/buildsystems/CMakeLists.txt +4 -0
@@ -246,6 +246,10 @@ elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") list(APPEND compat_SOURCES unix-socket.c) endif() +if(CMAKE_SYSTEM_NAME STREQUAL "Windows") + list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c) +endif() + set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX}) #header checks
simple-ipc.h new file +228 -0
228 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v6 7/12 unix-socket: eliminate static unix_stream_socket() helper function
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
The static helper function unix_stream_socket() calls die(). This is not appropriate for all callers. Eliminate the wrapper function and make the callers propagate the error.
--- unix-socket.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
unix-socket.c +13 -14
@@ -1,14 +1,6 @@#include "cache.h" #include "unix-socket.h" -static int unix_stream_socket (void ) -{ -int fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); -if (fd <0 ) -die_errno ("unable to create socket" ); -return fd; -} -static int chdir_len (const char *orig,int len) {char *path =xmemdupz (orig, len);
@@ -73,13 +65,16 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,int unix_stream_connect (const char *path) { -int fd, saved_errno; +int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx;if (unix_sockaddr_init (&sa, path, &ctx) <0 )return -1 ; - fd =unix_stream_socket (); + fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); +if (fd <0 ) + goto fail; +if (connect (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail;unix_sockaddr_cleanup (&ctx);
@@ -87,15 +82,16 @@ int unix_stream_connect(const char *path) fail: saved_errno = errno; +if (fd !=-1 ) +close (fd);unix_sockaddr_cleanup (&ctx); -close (fd); errno = saved_errno;return -1 ; }int unix_stream_listen (const char *path) { -int fd, saved_errno; +int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx;
@@ -103,7 +99,9 @@ int unix_stream_listen(const char *path)if (unix_sockaddr_init (&sa, path, &ctx) <0 )return -1 ; - fd =unix_stream_socket (); + fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); +if (fd <0 ) + goto fail;if (bind (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail;
@@ -116,8 +114,9 @@ int unix_stream_listen(const char *path) fail: saved_errno = errno; +if (fd !=-1 ) +close (fd);unix_sockaddr_cleanup (&ctx); -close (fd); errno = saved_errno;return -1 ; }
Signature · 1 line
gitgitgadget
PATCH v6 8/12 unix-socket: add backlog size option to unix_stream_listen()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Update unix_stream_listen() to take an options structure to override default behaviors. This commit includes the size of the listen() backlog.
--- builtin/credential-cache--daemon.c | 3 ++- unix-socket.c | 11 +++++++++-- unix-socket.h | 9 ++++++++- 3 files changed, 19 insertions(+), 4 deletions(-)
builtin/credential-cache--daemon.c +2 -1
@@ -203,9 +203,10 @@ 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 +9 -2
@@ -1,6 +1,8 @@#include "cache.h" #include "unix-socket.h" +#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5) +static int chdir_len (const char *orig,int len) {char *path =xmemdupz (orig, len);
@@ -89,9 +91,11 @@ int unix_stream_connect(const char *path)return -1 ; } -int unix_stream_listen (const char *path) +int unix_stream_listen (const char *path, +const struct unix_stream_listen_opts *opts) {int fd =-1 , saved_errno; +int backlog;struct sockaddr_un sa;struct unix_sockaddr_context ctx;
@@ -106,7 +110,10 @@ int unix_stream_listen(const char *path)if (bind (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail; -if (listen (fd,5 ) <0 ) + backlog = opts->listen_backlog_size; +if (backlog <=0 ) + backlog =DEFAULT_UNIX_STREAM_LISTEN_BACKLOG ; +if (listen (fd, backlog) <0 ) goto fail;unix_sockaddr_cleanup (&ctx);
unix-socket.h +8 -1
@@ -1,7 +1,14 @@#ifndef UNIX_SOCKET_H #define UNIX_SOCKET_H +struct unix_stream_listen_opts { +int listen_backlog_size; +}; + +#define UNIX_STREAM_LISTEN_OPTS_INIT { 0 } +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 */
Signature · 1 line
gitgitgadget
PATCH v6 9/12 unix-socket: disallow chdir() when creating unix domain sockets
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() or unix_stream_connect() is given a socket pathname that is too long to fit in a sockaddr_un structure, it will chdir() to the parent directory of the requested socket pathname, create the socket using a relative pathname, and then chdir() back. This is not thread-safe.
Teach unix_sockaddr_init() to not allow calls to chdir() when this flag is set.
--- builtin/credential-cache.c | 2 +- unix-socket.c | 17 ++++++++++++----- unix-socket.h | 3 ++- 3 files changed, 15 insertions(+), 7 deletions(-)
builtin/credential-cache.c +1 -1
@@ -14,7 +14,7 @@static int send_request (const char *socket,const struct strbuf *out) {int got_data =0 ; -int fd =unix_stream_connect (socket); +int fd =unix_stream_connect (socket,0 );if (fd <0 )return -1 ;
unix-socket.c +12 -5
@@ -30,16 +30,23 @@ static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) }static int unix_sockaddr_init (struct sockaddr_un *sa,const char *path, -struct unix_sockaddr_context *ctx) +struct unix_sockaddr_context *ctx, +int disallow_chdir) {int size =strlen (path) +1 ; ctx->orig_dir =NULL ;if (size >sizeof (sa->sun_path)) { -const char *slash =find_last_dir_sep (path); +const char *slash;const char *dir;struct strbuf cwd =STRBUF_INIT ; +if (disallow_chdir) { + errno =ENAMETOOLONG ; +return -1 ; + } + + slash =find_last_dir_sep (path);if (!slash) { errno =ENAMETOOLONG ;return -1 ;
@@ -65,13 +72,13 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,return 0 ; } -int unix_stream_connect (const char *path) +int unix_stream_connect (const char *path,int disallow_chdir) {int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx; -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
@@ -101,7 +108,7 @@ int unix_stream_listen(const char *path,unlink (path); -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, opts->disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
unix-socket.h +2 -1
@@ -3,11 +3,12 @@struct unix_stream_listen_opts {int listen_backlog_size; +unsigned int disallow_chdir:1 ; };#define UNIX_STREAM_LISTEN_OPTS_INIT { 0 } -int unix_stream_connect (const char *path); +int unix_stream_connect (const char *path,int disallow_chdir);int unix_stream_listen (const char *path,const struct unix_stream_listen_opts *opts);
Signature · 1 line
gitgitgadget
PATCH v6 10/12 unix-stream-server: create unix domain socket under lock
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create a wrapper class for unix_stream_listen() that uses a ".lock" lockfile to create the unix domain socket in a race-free manner.
Unix domain sockets have a fundamental problem on Unix systems because they persist in the filesystem until they are deleted. This is independent of whether a server is actually listening for connections. Well-behaved servers are expected to delete the socket when they shutdown. A new server cannot easily tell if a found socket is attached to an active server or is leftover cruft from a dead server. The traditional solution used by unix_stream_listen() is to force delete the socket pathname and then create a new socket. This solves the latter (cruft) problem, but in the case of the former, it orphans the existing server (by stealing the pathname associated with the socket it is listening on).
We cannot directly use a .lock lockfile to create the socket because the socket is created by bind(2) rather than the open(2) mechanism used by tempfile.c.
As an alternative, we hold a plain lockfile ("<path>.lock") as a mutual exclusion device. Under the lock, we test if an existing socket ("<path>") is has an active server. If not, we create a new socket and begin listening. Then we use "rollback" to delete the lockfile in all cases.
This wrapper code conceptually exists at a higher-level than the core unix_stream_connect() and unix_stream_listen() routines that it consumes. It is isolated in a wrapper class for clarity.
--- Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 2 +- unix-stream-server.c | 125 ++++++++++++++++++++++++++++ unix-stream-server.h | 33 ++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 unix-stream-server.c create mode 100644 unix-stream-server.h
Makefile +1 -0
@@ -1665,6 +1665,7 @@ ifdef NO_UNIX_SOCKETS BASIC_CFLAGS += -DNO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o + LIB_OBJS += unix-stream-server.o endif ifdef USE_WIN32_IPC
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -243,7 +243,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY ) - list(APPEND compat_SOURCES unix-socket.c) + list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c) endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
unix-stream-server.c new file +125 -0
125 lines not on this page · raw message
unix-stream-server.h new file +33 -0
@@ -0,0 +1,33 @@ +#ifndef UNIX_STREAM_SERVER_H +#define UNIX_STREAM_SERVER_H + +#include "unix-socket.h" + +struct unix_ss_socket { +char *path_socket; +struct stat st_socket; +int fd_socket; +}; + +/* +* Create a Unix Domain Socket at the given path under the protection +* of a '.lock' lockfile. +* +* Returns 0 on success, -1 on error, -2 if socket is in use. +*/ +int unix_ss_create (const char *path, +const struct unix_stream_listen_opts *opts, +long timeout_ms, +struct unix_ss_socket **server_socket); + +/* +* Close and delete the socket. +*/ +void unix_ss_free (struct unix_ss_socket *server_socket); + +/* +* Return 1 if the inode of the pathname to our socket changes. +*/ +int unix_ss_was_stolen (struct unix_ss_socket *server_socket); + +#endif /* UNIX_STREAM_SERVER_H */
Signature · 1 line
gitgitgadget
PATCH v6 11/12 simple-ipc: add Unix domain socket implementation
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
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.
--- Makefile | 2 + compat/simple-ipc/ipc-unix-socket.c | 1000 +++++++++++++++++++++++++++ contrib/buildsystems/CMakeLists.txt | 2 + simple-ipc.h | 13 +- 4 files changed, 1016 insertions(+), 1 deletion(-) create mode 100644 compat/simple-ipc/ipc-unix-socket.c
Makefile +2 -0
@@ -1666,6 +1666,8 @@ ifdef NO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o LIB_OBJS += unix-stream-server.o + LIB_OBJS += compat/simple-ipc/ipc-shared.o + LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o endif ifdef USE_WIN32_IPC
compat/simple-ipc/ipc-unix-socket.c new file +1000 -0
1,000 lines not on this page · raw message
contrib/buildsystems/CMakeLists.txt +2 -0
@@ -248,6 +248,8 @@ endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows") list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c) +else() + list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c) endif() set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})
simple-ipc.h +12 -1
@@ -5,7 +5,7 @@* See Documentation/technical/api-simple-ipc.txt */ -#if defined(GIT_WINDOWS_NATIVE ) +#if defined(GIT_WINDOWS_NATIVE ) || !defined(NO_UNIX_SOCKETS )#define SUPPORTS_SIMPLE_IPC #endif
@@ -62,11 +62,17 @@ struct ipc_client_connect_options {* the service and need to wait for it to become ready. */ unsigned int wait_if_not_found:1; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ + unsigned int uds_disallow_chdir:1; };#define IPC_CLIENT_CONNECT_OPTIONS_INIT { \ .wait_if_busy = 0, \ .wait_if_not_found = 0, \ + .uds_disallow_chdir = 0, \ }/*
@@ -159,6 +165,11 @@ struct ipc_server_data;struct ipc_server_opts {int nr_threads; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ +unsigned int uds_disallow_chdir:1 ; };/*
Signature · 1 line
gitgitgadget
PATCH v6 12/12 t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create t0052-simple-ipc.sh with unit tests for the "simple-ipc" mechanism.
Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc" functions.
When the tool is invoked with "run-daemon", it runs a server to listen for "simple-ipc" connections on a test socket or named pipe and responds to a set of commands to exercise/stress the communication setup.
When the tool is invoked with "start-daemon", it spawns a "run-daemon" command in the background and waits for the server to become ready before exiting. (This helps make unit tests in t0052 more predictable and avoids the need for arbitrary sleeps in the test script.)
The tool also has a series of client "send" commands to send commands and data to a server instance.
--- Makefile | 1 + t/helper/test-simple-ipc.c | 787 +++++++++++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 ++++++ 5 files changed, 912 insertions(+) create mode 100644 t/helper/test-simple-ipc.c create mode 100755 t/t0052-simple-ipc.sh
Makefile +1 -0
@@ -734,6 +734,7 @@ TEST_BUILTINS_OBJS += test-serve-v2.o TEST_BUILTINS_OBJS += test-sha1.o TEST_BUILTINS_OBJS += test-sha256.o TEST_BUILTINS_OBJS += test-sigchain.o +TEST_BUILTINS_OBJS += test-simple-ipc.o TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o
t/helper/test-simple-ipc.c new file +787 -0
787 lines not on this page · raw message
t/helper/test-tool.c +1 -0
@@ -65,6 +65,7 @@ static struct test_cmd cmds[] = { { "sha1", cmd__sha1 }, { "sha256", cmd__sha256 }, { "sigchain", cmd__sigchain }, + { "simple-ipc", cmd__simple_ipc }, { "strcmp-offset", cmd__strcmp_offset }, { "string-list", cmd__string_list }, { "submodule-config", cmd__submodule_config },
t/helper/test-tool.h +1 -0
@@ -55,6 +55,7 @@ int cmd__sha1(int argc, const char **argv);int cmd__oid_array (int argc,const char **argv);int cmd__sha256 (int argc,const char **argv);int cmd__sigchain (int argc,const char **argv); +int cmd__simple_ipc (int argc,const char **argv);int cmd__strcmp_offset (int argc,const char **argv);int cmd__string_list (int argc,const char **argv);int cmd__submodule_config (int argc,const char **argv);
t/t0052-simple-ipc.sh new file +122 -0
122 lines not on this page · raw message
Signature · 1 line
gitgitgadget
v7 cover Simple IPC Mechanism
In reply to Jeff Hostetler via GitGitGadgetHere is version V7 of my simple-ipc series. The only change from V6 is to squash in the CALLOC_ARRAY() suggestion.
$ git range-diff v2.31.0-rc1..pr-766/jeffhostetler/simple-ipc-v6 v2.31.0-rc1..HEAD 1: fe35dc3d29 = 1: fe35dc3d29 pkt-line: eliminate the need for static buffer in packet_write_gently() 2: de11b30361 = 2: de11b30361 pkt-line: do not issue flush packets in write_packetized_*() 3: 3718da39da = 3: 3718da39da pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option 4: b43df7ad0b = 4: b43df7ad0b pkt-line: add options argument to read_packetized_to_strbuf() 5: f829feb2aa = 5: f829feb2aa simple-ipc: design documentation for new IPC mechanism 6: 58c3fb7cd7 = 6: 58c3fb7cd7 simple-ipc: add win32 implementation 7: 4e8c352fb3 = 7: 4e8c352fb3 unix-socket: eliminate static unix_stream_socket() helper function 8: 3b71f52d86 = 8: 3b71f52d86 unix-socket: add backlog size option to unix_stream_listen() 9: 5972a19836 = 9: 5972a19836 unix-socket: disallow chdir() when creating unix domain sockets 10: 02c885fd62 = 10: 02c885fd62 unix-stream-server: create unix domain socket under lock 11: 4c2199231d ! 11: eee5f4796d simple-ipc: add Unix domain socket implementation @@ compat/simple-ipc/ipc-unix-socket.c (new) + pthread_cond_init(&server_data->work_available_cond, NULL); + + server_data->queue_size = nr_threads * FIFO_SCALE; -+ server_data->fifo_fds = xcalloc(server_data->queue_size, -+ sizeof(*server_data->fifo_fds)); ++ CALLOC_ARRAY(server_data->fifo_fds, server_data->queue_size); + + server_data->accept_thread = + xcalloc(1, sizeof(*server_data->accept_thread)); 12: 132b6f3271 = 12: 8b5dcca684 t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Jeff
Jeff Hostetler (9):
pkt-line: eliminate the need for static buffer in
packet_write_gently()
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
unix-socket: eliminate static unix_stream_socket() helper function
unix-socket: add backlog size option to unix_stream_listen()
unix-socket: disallow chdir() when creating unix domain sockets
unix-stream-server: create unix domain socket under lock
simple-ipc: add Unix domain socket implementation
t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Johannes Schindelin (3):
pkt-line: do not issue flush packets in write_packetized_*()
pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
pkt-line: add options argument to read_packetized_to_strbuf()
Documentation/technical/api-simple-ipc.txt | 105 +++ Makefile | 9 + builtin/credential-cache--daemon.c | 3 +- builtin/credential-cache.c | 2 +- compat/simple-ipc/ipc-shared.c | 28 + compat/simple-ipc/ipc-unix-socket.c | 999 +++++++++++++++++++++ compat/simple-ipc/ipc-win32.c | 751 ++++++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 8 +- convert.c | 11 +- pkt-line.c | 59 +- pkt-line.h | 17 +- simple-ipc.h | 239 +++++ t/helper/test-simple-ipc.c | 787 ++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 +++ unix-socket.c | 53 +- unix-socket.h | 12 +- unix-stream-server.c | 125 +++ unix-stream-server.h | 33 + 21 files changed, 3315 insertions(+), 52 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 create mode 100644 unix-stream-server.c create mode 100644 unix-stream-server.h
base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/766
Range-diff vs v6:
1: fe35dc3d292d = 1: fe35dc3d292d pkt-line: eliminate the need for static buffer in packet_write_gently()
2: de11b3036148 = 2: de11b3036148 pkt-line: do not issue flush packets in write_packetized_*()
3: 3718da39da30 = 3: 3718da39da30 pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
4: b43df7ad0b7a = 4: b43df7ad0b7a pkt-line: add options argument to read_packetized_to_strbuf()
5: f829feb2aa93 = 5: f829feb2aa93 simple-ipc: design documentation for new IPC mechanism
6: 58c3fb7cd776 = 6: 58c3fb7cd776 simple-ipc: add win32 implementation
7: 4e8c352fb366 = 7: 4e8c352fb366 unix-socket: eliminate static unix_stream_socket() helper function
8: 3b71f52d8628 = 8: 3b71f52d8628 unix-socket: add backlog size option to unix_stream_listen()
9: 5972a198361c = 9: 5972a198361c unix-socket: disallow chdir() when creating unix domain sockets
10: 02c885fd623d = 10: 02c885fd623d unix-stream-server: create unix domain socket under lock
11: 4c2199231d05 ! 11: eee5f4796d37 simple-ipc: add Unix domain socket implementation
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ pthread_cond_init(&server_data->work_available_cond, NULL);
+
+ server_data->queue_size = nr_threads * FIFO_SCALE;
-+ server_data->fifo_fds = xcalloc(server_data->queue_size,
-+ sizeof(*server_data->fifo_fds));
++ CALLOC_ARRAY(server_data->fifo_fds, server_data->queue_size);
+
+ server_data->accept_thread =
+ xcalloc(1, sizeof(*server_data->accept_thread)); 12: 132b6f3271be = 12: 8b5dcca68440 t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Signature · 1 line
gitgitgadget
PATCH v7 1/12 pkt-line: eliminate the need for static buffer in packet_write_gently()
From: Jeff Hostetler <[email protected]>
Teach packet_write_gently() to write the pkt-line header and the actual buffer in 2 separate calls to write_in_full() and avoid the need for a static buffer, thread-safe scratch space, or an excessively large stack buffer.
Change write_packetized_from_fd() to allocate a temporary buffer rather than using a static buffer to avoid similar issues here.
These changes are intended to make it easier to use pkt-line routines in a multi-threaded context with multiple concurrent writers writing to different streams.
--- pkt-line.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
pkt-line.c +20 -8
@@ -196,17 +196,26 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)static int packet_write_gently (const int fd_out,const char *buf,size_t size) { -static char packet_write_buffer[LARGE_PACKET_MAX ]; +char header[4 ];size_t packet_size; -if (size >sizeof (packet_write_buffer) -4 ) +if (size >LARGE_PACKET_DATA_MAX )return error (_ ("packet write failed - data exceeds max packet size" ));packet_trace (buf, size,1 ); packet_size = size +4 ; -set_packet_header (packet_write_buffer, packet_size); -memcpy (packet_write_buffer +4 , buf, size); -if (write_in_full (fd_out, packet_write_buffer, packet_size) <0 ) + +set_packet_header (header, packet_size); + +/* +* Write the header and the buffer in 2 parts so that we do +* not need to allocate a buffer or rely on a static buffer. +* This also avoids putting a large buffer on the stack which +* might have multi-threading issues. +*/ + +if (write_in_full (fd_out, header,4 ) <0 || +write_in_full (fd_out, buf, size) <0 )return error (_ ("packet write failed" ));return 0 ; }
@@ -244,20 +253,23 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)int write_packetized_from_fd (int fd_in,int fd_out) { -static char buf[LARGE_PACKET_DATA_MAX ]; +char *buf =xmalloc (LARGE_PACKET_DATA_MAX );int err =0 ;ssize_t bytes_to_write;while (!err) { - bytes_to_write =xread (fd_in, buf,sizeof (buf)); -if (bytes_to_write <0 ) + bytes_to_write =xread (fd_in, buf,LARGE_PACKET_DATA_MAX ); +if (bytes_to_write <0 ) { +free (buf);return COPY_READ_ERROR ; + }if (bytes_to_write ==0 )break ; err =packet_write_gently (fd_out, buf, bytes_to_write); }if (!err) err =packet_flush_gently (fd_out); +free (buf);return err; }
Signature · 1 line
gitgitgadget
PATCH v7 2/12 pkt-line: do not issue flush packets in write_packetized_*()
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Remove the packet_flush_gently() call in write_packetized_from_buf() and write_packetized_from_fd() and require the caller to call it if desired. Rename both functions to write_packetized_from_*_no_flush()` to prevent later merge accidents.
write_packetized_from_buf() currently only has one caller:apply_multi_file_filter() in convert.c. It always wants a flush packet to be written after writing the payload.
However, we are about to introduce a caller that wants to write many packets before a final flush packet, so let's make the caller responsible for emitting the flush packet.
--- convert.c | 8 ++++++-- pkt-line.c | 8 ++------ pkt-line.h | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-)
convert.c +6 -2
@@ -884,9 +884,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len goto done;if (fd >=0 ) - err =write_packetized_from_fd (fd, process->in); + err =write_packetized_from_fd_no_flush (fd, process->in);else - err =write_packetized_from_buf (src, len, process->in); + err =write_packetized_from_buf_no_flush (src, len, process->in); +if (err) + goto done; + + err =packet_flush_gently (process->in);if (err) goto done;
pkt-line.c +2 -6
@@ -251,7 +251,7 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)packet_trace (data, len,1 ); } -int write_packetized_from_fd (int fd_in,int fd_out) +int write_packetized_from_fd_no_flush (int fd_in,int fd_out) {char *buf =xmalloc (LARGE_PACKET_DATA_MAX );int err =0 ;
@@ -267,13 +267,11 @@ int write_packetized_from_fd(int fd_in, int fd_out)break ; err =packet_write_gently (fd_out, buf, bytes_to_write); } -if (!err) - err =packet_flush_gently (fd_out);free (buf);return err; } -int write_packetized_from_buf (const char *src_in,size_t len,int fd_out) +int write_packetized_from_buf_no_flush (const char *src_in,size_t len,int fd_out) {int err =0 ;size_t bytes_written =0 ;
@@ -289,8 +287,6 @@ 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); bytes_written += bytes_to_write; } -if (!err) - err =packet_flush_gently (fd_out);return err; }
pkt-line.h +2 -2
@@ -32,8 +32,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((fvoid packet_buf_write_len (struct strbuf *buf,const char *data,size_t len);int packet_flush_gently (int fd);int packet_write_fmt_gently (int fd,const char *fmt, ...) __attribute__((format (printf,2 ,3 ))); -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_fd_no_flush (int fd_in,int fd_out); +int write_packetized_from_buf_no_flush (const char *src_in,size_t len,int fd_out);/* * Read a packetized line into the buffer, which must be at least size bytes
Signature · 1 line
gitgitgadget
PATCH v7 3/12 pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Introduce PACKET_READ_GENTLE_ON_READ_ERROR option to help libify the packet readers.
So far, the (possibly indirect) callers of get_packet_data() can ask that function to return an error instead of die()ing upon end-of-file. However, random read errors will still cause the process to die.
So let's introduce an explicit option to tell the packet reader machinery to please be nice and only return an error on read errors.
This change prepares pkt-line for use by long-running daemon processes. Such processes should be able to serve multiple concurrent clients and and survive random IO errors. If there is an error on one connection, a daemon should be able to drop that connection and continue serving existing and future connections.
This ability will be used by a Git-aware "Builtin FSMonitor" feature in a later patch series.
--- pkt-line.c | 19 +++++++++++++++++-- pkt-line.h | 11 ++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)
pkt-line.c +17 -2
@@ -306,8 +306,11 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size, *src_size -= ret; }else { ret =read_in_full (fd, dst, size); -if (ret <0 ) +if (ret <0 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error_errno (_ ("read error" ));die_errno (_ ("read error" )); + } }/* And complain if we didn't get enough bytes to satisfy the read. */
@@ -315,6 +318,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,if (options &PACKET_READ_GENTLE_ON_EOF )return -1 ; +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("the remote end hung up unexpectedly" ));die (_ ("the remote end hung up unexpectedly" )); }
@@ -343,6 +348,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, len =packet_length (linelen);if (len <0 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length " +"character: %.4s" ), linelen);die (_ ("protocol error: bad line length character: %.4s" ), linelen); }else if (!len) {packet_trace ("0000" ,4 ,0 );
@@ -357,12 +365,19 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, *pktlen =0 ;return PACKET_READ_RESPONSE_END ; }else if (len <4 ) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length %d" ), + len);die (_ ("protocol error: bad line length %d" ), len); } len -=4 ; -if ((unsigned )len >= size) +if ((unsigned )len >= size) { +if (options &PACKET_READ_GENTLE_ON_READ_ERROR ) +return error (_ ("protocol error: bad line length %d" ), + len);die (_ ("protocol error: bad line length %d" ), len); + }if (get_packet_data (fd, src_buffer, src_len, buffer, len, options) <0 ) { *pktlen =-1 ;
pkt-line.h +8 -3
@@ -68,10 +68,15 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou* * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an * ERR packet. +* +* If options contains PACKET_READ_GENTLE_ON_READ_ERROR, we will not die +* on read errors, but instead return -1. However, we may still die on an +* ERR packet (if requested). */ -#define PACKET_READ_GENTLE_ON_EOF (1u<<0) -#define PACKET_READ_CHOMP_NEWLINE (1u<<1) -#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2) +#define PACKET_READ_GENTLE_ON_EOF (1u<<0) +#define PACKET_READ_CHOMP_NEWLINE (1u<<1) +#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2) +#define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)int packet_read (int fd,char **src_buffer,size_t *src_len,char *buffer,unsigned size,int options);
Signature · 1 line
gitgitgadget
PATCH v7 4/12 pkt-line: add options argument to read_packetized_to_strbuf()
In reply to Jeff Hostetler via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Update the calling sequence of read_packetized_to_strbuf() to take an options argument and not assume a fixed set of options. Update the only existing caller accordingly to explicitly pass the formerly-assumed flags.
The read_packetized_to_strbuf() function calls packet_read() with a fixed set of assumed options (PACKET_READ_GENTLE_ON_EOF). This assumption has been fine for the single existing caller apply_multi_file_filter() in convert.c.
In a later commit we would like to add other callers to read_packetized_to_strbuf() that need a different set of options.
--- convert.c | 3 ++- pkt-line.c | 4 ++-- pkt-line.h | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)
convert.c +2 -1
@@ -907,7 +907,8 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t lenif (err) goto done; - err =read_packetized_to_strbuf (process->out, &nbuf) <0 ; + err =read_packetized_to_strbuf (process->out, &nbuf, +PACKET_READ_GENTLE_ON_EOF ) <0 ;if (err) goto done;
pkt-line.c +2 -2
@@ -444,7 +444,7 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)return packet_read_line_generic (-1 , src, src_len, dst_len); } -ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out) +ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out,int options) {int packet_len;
@@ -460,7 +460,7 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)* that there is already room for the extra byte. */ sb_out->buf + sb_out->len,LARGE_PACKET_DATA_MAX +1 , - PACKET_READ_GENTLE_ON_EOF); + options);if (packet_len <=0 )break ; sb_out->len += packet_len;
pkt-line.h +1 -1
@@ -136,7 +136,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);/* * Reads a stream of variable sized packets until a flush packet is detected. */ -ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out); +ssize_t read_packetized_to_strbuf (int fd_in,struct strbuf *sb_out,int options);/* * Receive multiplexed output stream over git native protocol.
Signature · 1 line
gitgitgadget
PATCH v7 5/12 simple-ipc: design documentation for new IPC mechanism
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Brief design documentation for new IPC mechanism allowing foreground Git client to talk with an existing daemon process at a known location using a named pipe or unix domain socket.
--- Documentation/technical/api-simple-ipc.txt | 105 +++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 Documentation/technical/api-simple-ipc.txt
Documentation/technical/api-simple-ipc.txt new file +105 -0
@@ -0,0 +1,105 @@ +Simple-IPC API +============== + +The Simple-IPC API is a collection of `ipc_` prefixed library routines +and a basic communication protocol that allow an IPC-client process to +send an application-specific IPC-request message to an IPC-server +process and receive an application-specific IPC-response message. + +Communication occurs over a named pipe on Windows and a Unix domain +socket on other platforms. IPC-clients and IPC-servers rendezvous at +a previously agreed-to application-specific pathname (which is outside +the scope of this design) that is local to the computer system. + +The IPC-server routines within the server application process create a +thread pool to listen for connections and receive request messages +from multiple concurrent IPC-clients. When received, these messages +are dispatched up to the server application callbacks for handling. +IPC-server routines then incrementally relay responses back to the +IPC-client. + +The IPC-client routines within a client application process connect +to the IPC-server and send a request message and wait for a response. +When received, the response is returned back the caller. + +For example, the `fsmonitor--daemon` feature will be built as a server +application on top of the IPC-server library routines. It will have +threads watching for file system events and a thread pool waiting for +client connections. Clients, such as `git status` will request a list +of file system events since a point in time and the server will +respond with a list of changed files and directories. The formats of +the request and response are application-specific; the IPC-client and +IPC-server routines treat them as opaque byte streams. + + +Comparison with sub-process model +--------------------------------- + +The Simple-IPC mechanism differs from the existing `sub-process.c` +model (Documentation/technical/long-running-process-protocol.txt) and +used by applications like Git-LFS. In the LFS-style sub-process model +the helper is started by the foreground process, communication happens +via a pair of file descriptors bound to the stdin/stdout of the +sub-process, the sub-process only serves the current foreground +process, and the sub-process exits when the foreground process +terminates. + +In the Simple-IPC model the server is a very long-running service. It +can service many clients at the same time and has a private socket or +named pipe connection to each active client. It might be started +(on-demand) by the current client process or it might have been +started by a previous client or by the OS at boot time. The server +process is not associated with a terminal and it persists after +clients terminate. Clients do not have access to the stdin/stdout of +the server process and therefore must communicate over sockets or +named pipes. + + +Server startup and shutdown +--------------------------- + +How an application server based upon IPC-server is started is also +outside the scope of the Simple-IPC design and is a property of the +application using it. For example, the server might be started or +restarted during routine maintenance operations, or it might be +started as a system service during the system boot-up sequence, or it +might be started on-demand by a foreground Git command when needed. + +Similarly, server shutdown is a property of the application using +the simple-ipc routines. For example, the server might decide to +shutdown when idle or only upon explicit request. + + +Simple-IPC protocol +------------------- + +The Simple-IPC protocol consists of a single request message from the +client and an optional response message from the server. Both the +client and server messages are unlimited in length and are terminated +with a flush packet. + +The pkt-line routines (Documentation/technical/protocol-common.txt) +are used to simplify buffer management during message generation, +transmission, and reception. A flush packet is used to mark the end +of the message. This allows the sender to incrementally generate and +transmit the message. It allows the receiver to incrementally receive +the message in chunks and to know when they have received the entire +message. + +The actual byte format of the client request and server response +messages are application specific. The IPC layer transmits and +receives them as opaque byte buffers without any concern for the +content within. It is the job of the calling application layer to +understand the contents of the request and response messages. + + +Summary +------- + +Conceptually, the Simple-IPC protocol is similar to an HTTP REST +request. Clients connect, make an application-specific and +stateless request, receive an application-specific +response, and disconnect. It is a one round trip facility for +querying the server. The Simple-IPC routines hide the socket, +named pipe, and thread pool details and allow the application +layer to focus on the application at hand.
Signature · 1 line
gitgitgadget
PATCH v7 6/12 simple-ipc: add win32 implementation
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create Windows implementation of "simple-ipc" using named pipes.
--- Makefile | 5 + compat/simple-ipc/ipc-shared.c | 28 ++ compat/simple-ipc/ipc-win32.c | 751 ++++++++++++++++++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 4 + simple-ipc.h | 228 +++++++++ 6 files changed, 1018 insertions(+) create mode 100644 compat/simple-ipc/ipc-shared.c create mode 100644 compat/simple-ipc/ipc-win32.c create mode 100644 simple-ipc.h
Makefile +5 -0
@@ -1667,6 +1667,11 @@ else LIB_OBJS += unix-socket.o endif +ifdef USE_WIN32_IPC + LIB_OBJS += compat/simple-ipc/ipc-shared.o + LIB_OBJS += compat/simple-ipc/ipc-win32.o +endif + ifdef NO_ICONV BASIC_CFLAGS += -DNO_ICONV endif
compat/simple-ipc/ipc-shared.c new file +28 -0
@@ -0,0 +1,28 @@ +#include "cache.h" +#include "simple-ipc.h" +#include "strbuf.h" +#include "pkt-line.h" +#include "thread-utils.h" + +#ifdef SUPPORTS_SIMPLE_IPC + +int ipc_server_run (const char *path,const struct ipc_server_opts *opts, +ipc_server_application_cb *application_cb, +void *application_data) +{ +struct ipc_server_data *server_data =NULL ; +int ret; + + ret =ipc_server_run_async (&server_data, path, opts, + application_cb, application_data); +if (ret) +return ret; + + ret =ipc_server_await (server_data); + +ipc_server_free (server_data); + +return ret; +} + +#endif /* SUPPORTS_SIMPLE_IPC */
compat/simple-ipc/ipc-win32.c new file +751 -0
751 lines not on this page · raw message
config.mak.uname +2 -0
@@ -421,6 +421,7 @@ ifeq ($(uname_S),Windows) RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease + USE_WIN32_IPC = YesPlease USE_WIN32_MMAP = YesPlease MMAP_PREVENTS_DELETE = UnfortunatelyYes # USE_NED_ALLOCATOR = YesPlease
@@ -597,6 +598,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease + USE_WIN32_IPC = YesPlease USE_WIN32_MMAP = YesPlease MMAP_PREVENTS_DELETE = UnfortunatelyYes USE_NED_ALLOCATOR = YesPlease
contrib/buildsystems/CMakeLists.txt +4 -0
@@ -246,6 +246,10 @@ elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") list(APPEND compat_SOURCES unix-socket.c) endif() +if(CMAKE_SYSTEM_NAME STREQUAL "Windows") + list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c) +endif() + set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX}) #header checks
simple-ipc.h new file +228 -0
228 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v7 7/12 unix-socket: eliminate static unix_stream_socket() helper function
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
The static helper function unix_stream_socket() calls die(). This is not appropriate for all callers. Eliminate the wrapper function and make the callers propagate the error.
--- unix-socket.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
unix-socket.c +13 -14
@@ -1,14 +1,6 @@#include "cache.h" #include "unix-socket.h" -static int unix_stream_socket (void ) -{ -int fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); -if (fd <0 ) -die_errno ("unable to create socket" ); -return fd; -} -static int chdir_len (const char *orig,int len) {char *path =xmemdupz (orig, len);
@@ -73,13 +65,16 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,int unix_stream_connect (const char *path) { -int fd, saved_errno; +int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx;if (unix_sockaddr_init (&sa, path, &ctx) <0 )return -1 ; - fd =unix_stream_socket (); + fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); +if (fd <0 ) + goto fail; +if (connect (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail;unix_sockaddr_cleanup (&ctx);
@@ -87,15 +82,16 @@ int unix_stream_connect(const char *path) fail: saved_errno = errno; +if (fd !=-1 ) +close (fd);unix_sockaddr_cleanup (&ctx); -close (fd); errno = saved_errno;return -1 ; }int unix_stream_listen (const char *path) { -int fd, saved_errno; +int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx;
@@ -103,7 +99,9 @@ int unix_stream_listen(const char *path)if (unix_sockaddr_init (&sa, path, &ctx) <0 )return -1 ; - fd =unix_stream_socket (); + fd =socket (AF_UNIX ,SOCK_STREAM ,0 ); +if (fd <0 ) + goto fail;if (bind (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail;
@@ -116,8 +114,9 @@ int unix_stream_listen(const char *path) fail: saved_errno = errno; +if (fd !=-1 ) +close (fd);unix_sockaddr_cleanup (&ctx); -close (fd); errno = saved_errno;return -1 ; }
Signature · 1 line
gitgitgadget
PATCH v7 8/12 unix-socket: add backlog size option to unix_stream_listen()
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Update unix_stream_listen() to take an options structure to override default behaviors. This commit includes the size of the listen() backlog.
--- builtin/credential-cache--daemon.c | 3 ++- unix-socket.c | 11 +++++++++-- unix-socket.h | 9 ++++++++- 3 files changed, 19 insertions(+), 4 deletions(-)
builtin/credential-cache--daemon.c +2 -1
@@ -203,9 +203,10 @@ 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 +9 -2
@@ -1,6 +1,8 @@#include "cache.h" #include "unix-socket.h" +#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5) +static int chdir_len (const char *orig,int len) {char *path =xmemdupz (orig, len);
@@ -89,9 +91,11 @@ int unix_stream_connect(const char *path)return -1 ; } -int unix_stream_listen (const char *path) +int unix_stream_listen (const char *path, +const struct unix_stream_listen_opts *opts) {int fd =-1 , saved_errno; +int backlog;struct sockaddr_un sa;struct unix_sockaddr_context ctx;
@@ -106,7 +110,10 @@ int unix_stream_listen(const char *path)if (bind (fd, (struct sockaddr *)&sa,sizeof (sa)) <0 ) goto fail; -if (listen (fd,5 ) <0 ) + backlog = opts->listen_backlog_size; +if (backlog <=0 ) + backlog =DEFAULT_UNIX_STREAM_LISTEN_BACKLOG ; +if (listen (fd, backlog) <0 ) goto fail;unix_sockaddr_cleanup (&ctx);
unix-socket.h +8 -1
@@ -1,7 +1,14 @@#ifndef UNIX_SOCKET_H #define UNIX_SOCKET_H +struct unix_stream_listen_opts { +int listen_backlog_size; +}; + +#define UNIX_STREAM_LISTEN_OPTS_INIT { 0 } +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 */
Signature · 1 line
gitgitgadget
PATCH v7 9/12 unix-socket: disallow chdir() when creating unix domain sockets
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() or unix_stream_connect() is given a socket pathname that is too long to fit in a sockaddr_un structure, it will chdir() to the parent directory of the requested socket pathname, create the socket using a relative pathname, and then chdir() back. This is not thread-safe.
Teach unix_sockaddr_init() to not allow calls to chdir() when this flag is set.
--- builtin/credential-cache.c | 2 +- unix-socket.c | 17 ++++++++++++----- unix-socket.h | 3 ++- 3 files changed, 15 insertions(+), 7 deletions(-)
builtin/credential-cache.c +1 -1
@@ -14,7 +14,7 @@static int send_request (const char *socket,const struct strbuf *out) {int got_data =0 ; -int fd =unix_stream_connect (socket); +int fd =unix_stream_connect (socket,0 );if (fd <0 )return -1 ;
unix-socket.c +12 -5
@@ -30,16 +30,23 @@ static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) }static int unix_sockaddr_init (struct sockaddr_un *sa,const char *path, -struct unix_sockaddr_context *ctx) +struct unix_sockaddr_context *ctx, +int disallow_chdir) {int size =strlen (path) +1 ; ctx->orig_dir =NULL ;if (size >sizeof (sa->sun_path)) { -const char *slash =find_last_dir_sep (path); +const char *slash;const char *dir;struct strbuf cwd =STRBUF_INIT ; +if (disallow_chdir) { + errno =ENAMETOOLONG ; +return -1 ; + } + + slash =find_last_dir_sep (path);if (!slash) { errno =ENAMETOOLONG ;return -1 ;
@@ -65,13 +72,13 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,return 0 ; } -int unix_stream_connect (const char *path) +int unix_stream_connect (const char *path,int disallow_chdir) {int fd =-1 , saved_errno;struct sockaddr_un sa;struct unix_sockaddr_context ctx; -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
@@ -101,7 +108,7 @@ int unix_stream_listen(const char *path,unlink (path); -if (unix_sockaddr_init (&sa, path, &ctx) <0 ) +if (unix_sockaddr_init (&sa, path, &ctx, opts->disallow_chdir) <0 )return -1 ; fd =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fd <0 )
unix-socket.h +2 -1
@@ -3,11 +3,12 @@struct unix_stream_listen_opts {int listen_backlog_size; +unsigned int disallow_chdir:1 ; };#define UNIX_STREAM_LISTEN_OPTS_INIT { 0 } -int unix_stream_connect (const char *path); +int unix_stream_connect (const char *path,int disallow_chdir);int unix_stream_listen (const char *path,const struct unix_stream_listen_opts *opts);
Signature · 1 line
gitgitgadget
PATCH v7 10/12 unix-stream-server: create unix domain socket under lock
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create a wrapper class for unix_stream_listen() that uses a ".lock" lockfile to create the unix domain socket in a race-free manner.
Unix domain sockets have a fundamental problem on Unix systems because they persist in the filesystem until they are deleted. This is independent of whether a server is actually listening for connections. Well-behaved servers are expected to delete the socket when they shutdown. A new server cannot easily tell if a found socket is attached to an active server or is leftover cruft from a dead server. The traditional solution used by unix_stream_listen() is to force delete the socket pathname and then create a new socket. This solves the latter (cruft) problem, but in the case of the former, it orphans the existing server (by stealing the pathname associated with the socket it is listening on).
We cannot directly use a .lock lockfile to create the socket because the socket is created by bind(2) rather than the open(2) mechanism used by tempfile.c.
As an alternative, we hold a plain lockfile ("<path>.lock") as a mutual exclusion device. Under the lock, we test if an existing socket ("<path>") is has an active server. If not, we create a new socket and begin listening. Then we use "rollback" to delete the lockfile in all cases.
This wrapper code conceptually exists at a higher-level than the core unix_stream_connect() and unix_stream_listen() routines that it consumes. It is isolated in a wrapper class for clarity.
--- Makefile | 1 + contrib/buildsystems/CMakeLists.txt | 2 +- unix-stream-server.c | 125 ++++++++++++++++++++++++++++ unix-stream-server.h | 33 ++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 unix-stream-server.c create mode 100644 unix-stream-server.h
Makefile +1 -0
@@ -1665,6 +1665,7 @@ ifdef NO_UNIX_SOCKETS BASIC_CFLAGS += -DNO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o + LIB_OBJS += unix-stream-server.o endif ifdef USE_WIN32_IPC
contrib/buildsystems/CMakeLists.txt +1 -1
@@ -243,7 +243,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY ) - list(APPEND compat_SOURCES unix-socket.c) + list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c) endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
unix-stream-server.c new file +125 -0
125 lines not on this page · raw message
unix-stream-server.h new file +33 -0
@@ -0,0 +1,33 @@ +#ifndef UNIX_STREAM_SERVER_H +#define UNIX_STREAM_SERVER_H + +#include "unix-socket.h" + +struct unix_ss_socket { +char *path_socket; +struct stat st_socket; +int fd_socket; +}; + +/* +* Create a Unix Domain Socket at the given path under the protection +* of a '.lock' lockfile. +* +* Returns 0 on success, -1 on error, -2 if socket is in use. +*/ +int unix_ss_create (const char *path, +const struct unix_stream_listen_opts *opts, +long timeout_ms, +struct unix_ss_socket **server_socket); + +/* +* Close and delete the socket. +*/ +void unix_ss_free (struct unix_ss_socket *server_socket); + +/* +* Return 1 if the inode of the pathname to our socket changes. +*/ +int unix_ss_was_stolen (struct unix_ss_socket *server_socket); + +#endif /* UNIX_STREAM_SERVER_H */
Signature · 1 line
gitgitgadget
PATCH v7 11/12 simple-ipc: add Unix domain socket implementation
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
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.
--- Makefile | 2 + compat/simple-ipc/ipc-unix-socket.c | 999 ++++++++++++++++++++++++++++ contrib/buildsystems/CMakeLists.txt | 2 + simple-ipc.h | 13 +- 4 files changed, 1015 insertions(+), 1 deletion(-) create mode 100644 compat/simple-ipc/ipc-unix-socket.c
Makefile +2 -0
@@ -1666,6 +1666,8 @@ ifdef NO_UNIX_SOCKETS else LIB_OBJS += unix-socket.o LIB_OBJS += unix-stream-server.o + LIB_OBJS += compat/simple-ipc/ipc-shared.o + LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o endif ifdef USE_WIN32_IPC
compat/simple-ipc/ipc-unix-socket.c new file +999 -0
999 lines not on this page · raw message
contrib/buildsystems/CMakeLists.txt +2 -0
@@ -248,6 +248,8 @@ endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows") list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c) +else() + list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c) endif() set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})
simple-ipc.h +12 -1
@@ -5,7 +5,7 @@* See Documentation/technical/api-simple-ipc.txt */ -#if defined(GIT_WINDOWS_NATIVE ) +#if defined(GIT_WINDOWS_NATIVE ) || !defined(NO_UNIX_SOCKETS )#define SUPPORTS_SIMPLE_IPC #endif
@@ -62,11 +62,17 @@ struct ipc_client_connect_options {* the service and need to wait for it to become ready. */ unsigned int wait_if_not_found:1; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ + unsigned int uds_disallow_chdir:1; };#define IPC_CLIENT_CONNECT_OPTIONS_INIT { \ .wait_if_busy = 0, \ .wait_if_not_found = 0, \ + .uds_disallow_chdir = 0, \ }/*
@@ -159,6 +165,11 @@ struct ipc_server_data;struct ipc_server_opts {int nr_threads; + +/* +* Disallow chdir() when creating a Unix domain socket. +*/ +unsigned int uds_disallow_chdir:1 ; };/*
Signature · 1 line
gitgitgadget
PATCH v7 12/12 t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
In reply to Jeff Hostetler via GitGitGadgetFrom: Jeff Hostetler <[email protected]>
Create t0052-simple-ipc.sh with unit tests for the "simple-ipc" mechanism.
Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc" functions.
When the tool is invoked with "run-daemon", it runs a server to listen for "simple-ipc" connections on a test socket or named pipe and responds to a set of commands to exercise/stress the communication setup.
When the tool is invoked with "start-daemon", it spawns a "run-daemon" command in the background and waits for the server to become ready before exiting. (This helps make unit tests in t0052 more predictable and avoids the need for arbitrary sleeps in the test script.)
The tool also has a series of client "send" commands to send commands and data to a server instance.
--- Makefile | 1 + t/helper/test-simple-ipc.c | 787 +++++++++++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 ++++++ 5 files changed, 912 insertions(+) create mode 100644 t/helper/test-simple-ipc.c create mode 100755 t/t0052-simple-ipc.sh
Makefile +1 -0
@@ -734,6 +734,7 @@ TEST_BUILTINS_OBJS += test-serve-v2.o TEST_BUILTINS_OBJS += test-sha1.o TEST_BUILTINS_OBJS += test-sha256.o TEST_BUILTINS_OBJS += test-sigchain.o +TEST_BUILTINS_OBJS += test-simple-ipc.o TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o
t/helper/test-simple-ipc.c new file +787 -0
787 lines not on this page · raw message
t/helper/test-tool.c +1 -0
@@ -65,6 +65,7 @@ static struct test_cmd cmds[] = { { "sha1", cmd__sha1 }, { "sha256", cmd__sha256 }, { "sigchain", cmd__sigchain }, + { "simple-ipc", cmd__simple_ipc }, { "strcmp-offset", cmd__strcmp_offset }, { "string-list", cmd__string_list }, { "submodule-config", cmd__submodule_config },
t/helper/test-tool.h +1 -0
@@ -55,6 +55,7 @@ int cmd__sha1(int argc, const char **argv);int cmd__oid_array (int argc,const char **argv);int cmd__sha256 (int argc,const char **argv);int cmd__sigchain (int argc,const char **argv); +int cmd__simple_ipc (int argc,const char **argv);int cmd__strcmp_offset (int argc,const char **argv);int cmd__string_list (int argc,const char **argv);int cmd__submodule_config (int argc,const char **argv);
t/t0052-simple-ipc.sh new file +122 -0
122 lines not on this page · raw message
Signature · 1 line
gitgitgadget