Simple IPC Mechanism

This series introduces a multi-threaded IPC mechanism called "Simple IPC". This is a library-layer feature to make it easy to create very long running daemon/service applications and for unrelated Git commands to communicate with them. Communication uses pkt-line messaging over a Windows named pipe or Unix domain socket.

On the server side, Simple IPC implements a (platform-specific) connection listener and worker thread-pool to accept and handle a series of client connections. The server functionality is completely hidden behind the ipc_server_run() and ipc_server_run_async() APIs. The daemon/service application only needs to define an application-specific callback to handle client requests.

Note that Simple IPC is completely unrelated to the long running process feature (described in sub-process.h) where the lifetime of a "sub-process" child is bound to that of the invoking parent process and communication occurs over the child's stdin/stdout.

Simple IPC will serve as a basis for a future builtin FSMonitor daemon feature.

Jeff Hostetler (7):
pkt-line: use stack rather than static buffer in packet_write_gently()
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
unix-socket: create gentle version of unix_stream_listen()
unix-socket: add no-chdir option to unix_stream_listen_gently()
simple-ipc: add t/helper/test-simple-ipc and t0052
simple-ipc: add Unix domain socket implementation

Johannes Schindelin (3):
pkt-line: (optionally) libify the packet readers
pkt-line: optionally skip the flush packet in
write_packetized_from_buf()
pkt-line: accept additional options in read_packetized_to_strbuf()

 Documentation/technical/api-simple-ipc.txt |   31 +
 Makefile                                   |    8 +
 compat/simple-ipc/ipc-shared.c             |   28 +
 compat/simple-ipc/ipc-unix-socket.c        | 1093 ++++++++++++++++++++
 compat/simple-ipc/ipc-win32.c              |  723 +++++++++++++
 config.mak.uname                           |    2 +
 contrib/buildsystems/CMakeLists.txt        |    6 +
 convert.c                                  |    4 +-
 pkt-line.c                                 |   30 +-
 pkt-line.h                                 |   13 +-
 simple-ipc.h                               |  221 ++++
 t/helper/test-simple-ipc.c                 |  485 +++++++++
 t/helper/test-tool.c                       |    1 +
 t/helper/test-tool.h                       |    1 +
 t/t0052-simple-ipc.sh                      |  129 +++
 unix-socket.c                              |   58 +-
 unix-socket.h                              |    9 +
 17 files changed, 2828 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/technical/api-simple-ipc.txt
 create mode 100644 compat/simple-ipc/ipc-shared.c
 create mode 100644 compat/simple-ipc/ipc-unix-socket.c
 create mode 100644 compat/simple-ipc/ipc-win32.c
 create mode 100644 simple-ipc.h
 create mode 100644 t/helper/test-simple-ipc.c
 create mode 100755 t/t0052-simple-ipc.sh

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/766

gitgitgadget

From: Jeff Hostetler <[email protected]>

Teach packet_write_gently() to use a stack buffer rather than a static buffer when composing the packet line message. This helps get us ready for threaded operations.

---
 pkt-line.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d633005ef74..98439a2fed0 100644
@@ -196,7 +196,7 @@ 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 packet_write_buffer[LARGE_PACKET_MAX];
 	size_t packet_size;
 
 	if (size > sizeof(packet_write_buffer) - 4)

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

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 "Internal FSMonitor" feature in a later patch series.

---
 pkt-line.c | 19 +++++++++++++++++--
 pkt-line.h |  4 ++++
 2 files changed, 21 insertions(+), 2 deletions(-)

index 98439a2fed0..5c2d86a2f60 100644
@@ -298,8 +298,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_NEVER_DIE)
+				return error_errno(_("read error"));
 			die_errno(_("read error"));
+		}
 	}
 
 	/* And complain if we didn't get enough bytes to satisfy the read. */

@@ -307,6 +310,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_NEVER_DIE)
+			return error(_("the remote end hung up unexpectedly"));
 		die(_("the remote end hung up unexpectedly"));
 	}
 

@@ -335,6 +340,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_NEVER_DIE)
+			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);

@@ -349,12 +357,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_NEVER_DIE)
+			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_NEVER_DIE)
+			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;

index 8c90daa59ef..c1fa245faf8 100644
@@ -68,10 +68,14 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  *
  * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
  * ERR packet.
+ *
+ * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
+ * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
  */
 #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_NEVER_DIE         (1u<<3)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function currently has only one caller: apply_multi_file_filter() in convert.c. That caller wants a flush packet to be written after writing the payload.

However, we are about to introduce a user that wants to write many packets before a final flush packet, so let's extend this function to prepare for that scenario.

---
 convert.c  | 2 +-
 pkt-line.c | 5 +++--
 pkt-line.h | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

index ee360c2f07c..3f396a9b288 100644
@@ -886,7 +886,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (fd >= 0)
 		err = write_packetized_from_fd(fd, process->in);
 	else
-		err = write_packetized_from_buf(src, len, process->in);
+		err = write_packetized_from_buf(src, len, process->in, 1);
 	if (err)
 		goto done;
 

index 5c2d86a2f60..ef83439b9ee 100644
@@ -261,7 +261,8 @@ int write_packetized_from_fd(int fd_in, int fd_out)
 	return err;
 }
 
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end)
 {
 	int err = 0;
 	size_t bytes_written = 0;

@@ -277,7 +278,7 @@ 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)
+	if (!err && flush_at_end)
 		err = packet_flush_gently(fd_out);
 	return err;
 }

index c1fa245faf8..5b7a0fb8510 100644
@@ -33,7 +33,8 @@ void 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_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end);
 
 /*
  * Read a packetized line into the buffer, which must be at least size bytes

gitgitgadget

From: Johannes Schindelin <[email protected]>

The read_packetized_to_strbuf() function reads packets into a strbuf until a flush packet has been received. So far, it has only one caller:
apply_multi_file_filter() in convert.c. This caller really only needs the PACKET_READ_GENTLE_ON_EOF option to be passed to packet_read() (which makes sense in the scenario where packets should be read until a flush packet is received).

We are about to introduce a caller that wants to pass other options through to packet_read(), so let's extend the function signature accordingly.

---
 convert.c  | 2 +-
 pkt-line.c | 4 ++--
 pkt-line.h | 6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

index 3f396a9b288..175c5cd51d5 100644
@@ -903,7 +903,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 		if (err)
 			goto done;
 
-		err = read_packetized_to_strbuf(process->out, &nbuf) < 0;
+		err = read_packetized_to_strbuf(process->out, &nbuf, 0) < 0;
 		if (err)
 			goto done;
 

index ef83439b9ee..615211819cd 100644
@@ -437,7 +437,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;
 

@@ -453,7 +453,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 | PACKET_READ_GENTLE_ON_EOF);
 		if (packet_len <= 0)
 			break;
 		sb_out->len += packet_len;

index 5b7a0fb8510..02554a20a6c 100644
@@ -135,8 +135,12 @@ 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.
+ *
+ * The options are augmented by PACKET_READ_GENTLE_ON_EOF and passed to
+ * packet_read.
  */
-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.

gitgitgadget

From: 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 | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/technical/api-simple-ipc.txt

new file mode 100644
index 00000000000..920994a69d3
@@ -0,0 +1,31 @@
+simple-ipc API
+==============
+
+The simple-ipc API is used to send an IPC message and response between
+a (presumably) foreground Git client process to a background server or
+daemon process.  The server process must already be running.  Multiple
+client processes can simultaneously communicate with the server
+process.
+
+Communication occurs over a named pipe on Windows and a Unix domain
+socket on other platforms.  Clients and the server rendezvous at a
+previously agreed-to application-specific pathname (which is outside
+the scope of this design).
+
+This IPC mechanism differs from the existing `sub-process.c` model
+(Documentation/technical/long-running-process-protocol.txt) and used
+by applications like Git-LFS because the server is assumed to be very
+long running system service.  In contrast, a "sub-process model process"
+is started with the foreground process and exits when the foreground
+process terminates.  How the server is started is also outside the
+scope of the IPC mechanism.
+
+The IPC protocol consists of a single request message from the client and
+an optional request message from the server.  For simplicity, pkt-line
+routines are used to hide chunking and buffering concerns.  Each side
+terminates their message with a flush packet.
+(Documentation/technical/protocol-common.txt)
+
+The actual format of the client and server messages is application
+specific.  The IPC layer transmits and receives an opaque buffer without
+any concern for the content within.

gitgitgadget

From: Jeff Hostetler <[email protected]>

Create Unix domain socket based implementation of "simple-ipc".

---
 Makefile                            |    2 +
 compat/simple-ipc/ipc-unix-socket.c | 1093 +++++++++++++++++++++++++++
 contrib/buildsystems/CMakeLists.txt |    2 +
 simple-ipc.h                        |    7 +-
 4 files changed, 1103 insertions(+), 1 deletion(-)
 create mode 100644 compat/simple-ipc/ipc-unix-socket.c

index e7ba8853ea6..f2524c02ff0 100644
@@ -1681,6 +1681,8 @@ ifdef NO_UNIX_SOCKETS
 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
+	LIB_OBJS += compat/simple-ipc/ipc-shared.o
+	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
 endif
 
 ifdef USE_WIN32_IPC

index 4bd41054ee7..4c27a373414 100644
@@ -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})

index cd525e711bd..8a6dfc72c83 100644
@@ -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
 

@@ -151,6 +151,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;
 };
 
 /*

gitgitgadget

From: Jeff Hostetler <[email protected]>

Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() is given a socket pathname that is too big 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.

Add disallow_chdir flag to struct unix_sockaddr_context and change all callers to pass an initialized context structure.

Teach unix_sockaddr_init() to not allow calls to chdir() when flag is set.

Extend the public interface to unix_stream_listen_gently() to also expose this new flag.

---
 unix-socket.c | 21 +++++++++++++++++----
 unix-socket.h |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

index 3a9ffc32268..f66987261e6 100644
@@ -19,8 +19,15 @@ static int chdir_len(const char *orig, int len)
 
 struct unix_sockaddr_context {
 	char *orig_dir;
+	unsigned int disallow_chdir:1;
 };
 
+#define UNIX_SOCKADDR_CONTEXT_INIT \
+{ \
+	.orig_dir=NULL, \
+	.disallow_chdir=0, \
+}
+
 static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
 {
 	if (!ctx->orig_dir)

@@ -40,7 +47,11 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 {
 	int size = strlen(path) + 1;
 
-	ctx->orig_dir = NULL;
+	if (ctx->disallow_chdir && size > sizeof(sa->sun_path)) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;

@@ -75,7 +86,7 @@ int unix_stream_connect(const char *path)
 {
 	int fd, saved_errno;
 	struct sockaddr_un sa;
-	struct unix_sockaddr_context ctx;
+	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;

@@ -97,7 +108,7 @@ int unix_stream_listen(const char *path)
 {
 	int fd, saved_errno;
 	struct sockaddr_un sa;
-	struct unix_sockaddr_context ctx;
+	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
 
 	unlink(path);
 

@@ -129,7 +140,9 @@ int unix_stream_listen_gently(const char *path,
 	int bind_successful = 0;
 	int saved_errno;
 	struct sockaddr_un sa;
-	struct unix_sockaddr_context ctx;
+	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
+
+	ctx.disallow_chdir = opts->disallow_chdir;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		goto fail;

index 253f579f087..08d3d822111 100644
@@ -7,6 +7,7 @@ int unix_stream_listen(const char *path);
 struct unix_stream_listen_opts {
 	int listen_backlog_size;
 	unsigned int force_unlink_before_bind:1;
+	unsigned int disallow_chdir:1;
 };
 
 int unix_stream_listen_gently(const char *path,

gitgitgadget

From: Jeff Hostetler <[email protected]>

Create unit tests for "simple-ipc". These are currently only enabled on Windows.

---
 Makefile                   |   1 +
 t/helper/test-simple-ipc.c | 485 +++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |   1 +
 t/helper/test-tool.h       |   1 +
 t/t0052-simple-ipc.sh      | 129 ++++++++++
 5 files changed, 617 insertions(+)
 create mode 100644 t/helper/test-simple-ipc.c
 create mode 100755 t/t0052-simple-ipc.sh

index c94d5847919..e7ba8853ea6 100644
@@ -740,6 +740,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

index 9d6d14d9293..a409655f03b 100644
@@ -64,6 +64,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 },

index a6470ff62c4..564eb3c8e91 100644
@@ -54,6 +54,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);

gitgitgadget

From: 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       | 723 ++++++++++++++++++++++++++++
 config.mak.uname                    |   2 +
 contrib/buildsystems/CMakeLists.txt |   4 +
 simple-ipc.h                        | 216 +++++++++
 6 files changed, 978 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

index 7b64106930a..c94d5847919 100644
@@ -1682,6 +1682,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

new file mode 100644
index 00000000000..1edec815953
@@ -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 */

index 198ab1e58f8..76087cff678 100644
@@ -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

index c151dd7257f..4bd41054ee7 100644
@@ -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

gitgitgadget

From: Jeff Hostetler <[email protected]>

Create a gentle version of unix_stream_listen(). This version does not call die() if a socket-fd cannot be created and does not assume that it is safe to unlink() an existing socket-inode.

unix_stream_listen() uses unix_stream_socket() helper function to create the socket-fd. Avoid that helper because it calls die() on errors.

unix_stream_listen() always tries to unlink() the socket-path before calling bind(). If there is an existing server/daemon already bound and listening on that socket-path, our unlink() would have the effect of disassociating the existing server's bound-socket-fd from the socket-path without notifying the existing server. The existing server could continue to service existing connections (accepted-socket-fd's), but would not receive any futher new connections (since clients rendezvous via the socket-path). The existing server would effectively be offline but yet appear to be active.

Furthermore, unix_stream_listen() creates an opportunity for a brief race condition for connecting clients if they try to connect in the interval between the forced unlink() and the subsequent bind() (which recreates the socket-path that is bound to a new socket-fd in the current process).

---
 unix-socket.c | 39 +++++++++++++++++++++++++++++++++++++++
 unix-socket.h |  8 ++++++++
 2 files changed, 47 insertions(+)

index 19ed48be990..3a9ffc32268 100644
@@ -121,3 +121,42 @@ int unix_stream_listen(const char *path)
 	errno = saved_errno;
 	return -1;
 }
+
+int unix_stream_listen_gently(const char *path,
+			      const struct unix_stream_listen_opts *opts)
+{
+	int fd = -1;
+	int bind_successful = 0;
+	int saved_errno;
+	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
+
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		goto fail;
+
+	fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (fd < 0)
+		goto fail;
+
+	if (opts->force_unlink_before_bind)
+		unlink(path);
+
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+		goto fail;
+	bind_successful = 1;
+
+	if (listen(fd, opts->listen_backlog_size) < 0)
+		goto fail;
+
+	unix_sockaddr_cleanup(&ctx);
+	return fd;
+
+fail:
+	saved_errno = errno;
+	unix_sockaddr_cleanup(&ctx);
+	close(fd);
+	if (bind_successful)
+		unlink(path);
+	errno = saved_errno;
+	return -1;
+}

index e271aeec5a0..253f579f087 100644
@@ -4,4 +4,12 @@
 int unix_stream_connect(const char *path);
 int unix_stream_listen(const char *path);
 
+struct unix_stream_listen_opts {
+	int listen_backlog_size;
+	unsigned int force_unlink_before_bind:1;
+};
+
+int unix_stream_listen_gently(const char *path,
+			      const struct unix_stream_listen_opts *opts);
+
 #endif /* UNIX_SOCKET_H */

gitgitgadget

I only skimmed this so far. In the past we had a git-read-cache--daemon -> git-index-helper[1] -> watchman. The last iteration of that seems to be the [3] re-roll from Ben Peart in 2017. I used/tested that for a while and had some near-production use-cases of it.

How does this new series relate to that past work (if at all), and (not having re-read the old threads) were there reasons those old patch serieses weren't merged in that are addressed here, mitigated etc?

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
3. https://lore.kernel.org/git/[email protected]/
4. https://lore.kernel.org/git/[email protected]/

I'm starting with the model used by the existing FSMonitor feature that Ben Peart and Kevin Willford added to Git.

Item [3] looks to be an earlier draft of that effort. The idea there was to add the fsmonitor hook that could talk to a daemon like Watchman and quickly update the in-memory cache-entry flags without the need to lstat() and similarly update the untracked-cache. An index extension was added to remember the last fsmonitor response processed.

Currently in Git, we have a fsmonitor hook (usually a perl script) that talks to Watchman and translates the Watchman response back into something that the Git client can understand. This comes back as a list of files that have changed since some timestamp (or in V2, relative to some daemon-specific token).

Items [1,2] are not related to that. That was a different effort to quickly fetch a read-only copy of an already-parsed index via shared memory. In the last version I saw, there were 2 daemons. index-helper kept a fresh view of the index in shared memory and could give it to the Git client. The client could just mmap the pre-parsed index and avoid calling read_index(). Index-helper would drive Watchman to keep track of cache-entries as they changed and handle the lstat's.

I'm not familiar with [4] (and I only quickly scanned it). There are several ideas for finding slow spots while reading the index. I don't want to go into all of them, but several are obsolete now. They didn't contribute to the current effort.

The Simple IPC series (and a soon to be submitted fsmonitor--daemon series) are intended to be a follow on to FSMonitor effort that is currently in Git.

1. Build a git-native daemon to watch the file system and avoid needing a third-party tool. This doesn't preclude the use of Watchman, but having a builtin tool might simplify engineering support costs when deploying to a large team.

2. Use direct IPC between the Git command and the daemon to avoid the expense of the Hook API (which is expensive on Windows).

3. Make the daemon Git-aware. For example, it might want to pre-filter ignored files. (This might not be present in V1. And we might extend the daemon to do more of this as we improve performance.)

Jeff

What kind of security implications does this bring into the system?

Can a Simple IPC daemon be connected by any client? How does the daemon know that the other side is authorized to make requests? When a git binary acting as client connect to whatever happens to be listening to the well-known location, how does it know if the other side is the daemon it wanted to talk to and not a malicious MITM or other impersonator?

Or is this to be only used for "this is meant to be used to give read-only access to data that is public anyway" kind of daemon, e.g. "git://" transport that serves clones and fetches?

Or is this meant to be used on client machines where all the processes are assumed to be working for the end user, so it is OK to declare that anything will go (good old DOS mental model?)

I know at the Mechanism level we do not yet know how it will be used, but we cannot retrofit sufficient security, so it would be necessary to know answers to these questions.

Thanks.

Good questions.

Yes, this is a local-only mechanism. A local-only named pipe on Windows and a Unix domain socket on Unix. In both cases the daemon creates the pipe/socket as the foreground user (since the daemon process will be implicitly started by the first Git command that needs to talk to it). Later client process try to open the pipe/socket with RW access if they can.

On Windows a local named pipe is created by the server side. It rejects
remote connections. I did not put an ACL, so it should inherit the
system default which grants the user RW access (since the daemon is
implicitly started by the first foreground client command that needs
to talk to it.) Other users in the user's group and the anonymous
user should have R but not W access to it, so they could not be able
to connect. The name pipe is kept in the local Named Pipe File System
(NPFS) as \\.\pipe\<unique-path> so it is globally visible on the system, but I don't think it is a problem.

On the Unix side, the socket is created inside the .git directory by the daemon. Potential clients would have to have access to the working directory and the .git directory to connect to the socket, so in normal circumstances they would be able to read everything in the WD anyway. So again, I don't think it is a problem.

Alternatively, if a malicious server is started and holds the named pipe or socket: the client might be able to talk to it (assuming that bad server grants access or impersonates the rightful user). The client might not be able to tell they've been tricked, but at that point the system is already compromised.

So unless I'm missing something, I think we're OK either way.

Jeff

Sorry, that's a quirk of Windows. Windows has a funky virtual drive where named pipes are stored -- kind of like a magic directory in /proc on Linux. All local named pipes have the "\\.\pipe\" path prefix.

So they are globally visible as a side-effect of that "namespace" restriction.

It is not intuitively obvious why globalluy visible thing is OK to me, but I'll take your word for it on stuff about Windows.

OK, yes, writability to .git would automatically mean that everything is a fair game to those who can talk to the daemon, so there is no new issue here, as long as the first process that creates the socket is careful not to loosen the permission.

Thanks.

Sounds like a good goal, but...

Just thinking out loud, here are two potential issues with putting it in .git that we may have to deal with later:

- fsmonitor is conceptually a read-only thing (i.e., it would speed up "git status", etc). And not knowing much about how it will work, I'd guess that is carried through (i.e., even though you may open the socket R/W so that you can write requests and read them back, there is no operation you can request that will overwrite data). But the running user may not have write access to .git.

As long as we cleanly bail to the non-fsmonitor code paths, I don't think it's the end of the world. Those read-only users just won't get to use the speedup (and it may even be desirable). They may complain, but it is open source so the onus is on them to improve it. You will not have made anything worse. :)

- repositories may be on network filesystems that do not support unix sockets.

So it would be nice if there was some way to specify an alternate path to be used for the socket. Possibly one or both of:

- a config option to give a root path for sockets, where Git would then canonicalize the $GIT_DIR name and use $root/$GIT_DIR for the socket. That solves the problem for a given user once for all repos.

- a config option to say "use this path for the socket". This would be per-repo, but is more flexible and possibly less confusing.

One final note: on some systems[1] the permissions on the socket file itself are ignored. The safe way to protect it is to make sure the permissions on the surrounding directory are what you want. See credential-cache's init_socket_directory() for an example.

-Peff

[1] Sorry, I don't remember which systems. This is one of those random bits of Unix lore I've carried around for 20 years, and it's entirely possible it is simply obsolete at this point.

The existing one is meant to be gentle. Maybe it is worth fixing it instead.

Yeah, I think this is just a bug. My thinking in the original was that socket() would basically never fail. And it generally wouldn't, but things like EMFILE do happen. There are only two callers, and both would be one-liners to propagate the error up the stack.

The trouble here is that one cannot tell if the existing file is active, and you are orphaning an existing server, or if there is leftover cruft from an exited server that did not clean up after itself (you will get EADDRINUSE either way).

Handling those cases (and especially doing so in a non-racy way) is probably outside the scope of unix_stream_listen(), but it makes sense for this to be an option. And it looks like you even made it so here, so unix_stream_listen() could just become a wrapper that sets the option. Or since there is only one caller in the whole code-base, perhaps it could just learn to pass the option struct. :)

Likewise for the no-chdir option added in the follow-on patch.

I'll be curious to see how you do this atomically. From my skim of patch 10, you will connect to see if it's active, and unlink if it's not. But then two simultaneous new processes could both see an inactive one and race to forcefully create the new one. One of them will lose and be orphaned with a socket that has no filesystem name.

There might be a solution using link() to have an atomic winner, but it gets tricky around unlinking the old name out of the way. You might need a separate dot-lock to make sure only one process does the unlink-and-create process at a time.

-Peff

According to StackExchange lore this seems to have been the case with 4.2 BSD & maybe something obscure like HP/UX:
https://unix.stackexchange.com/questions/83032/which-systems-do-not-honor-socket-read-write-permissions

I'd say it's probably safe to ignore this as a concern for new features in git in general, and certainly for something like a thing intended for a watchman-like program which users are likely to only run on modern OS's.

I had saved this to comment on, but Peff beat me to it :-)

You definitely should be able to do this atomically with link(), but the cleanup is indeed messy, and there's already existing locking code, so it's probably better to press that into service here.

Chris

Good point.

I'll look at the callers and see if I can do something safer.

Jeeff

From: Junio C Hamano <[email protected]>

We run "git pull" against "$cask_repo"; clarify that we are expecting not to have any of our own modifications and running "git pull" to merely update, by passing "--ff-only" on the command line.

Also, the "brew cask install" command line triggers an error message that says:

Error: Calling brew cask install is disabled! Use brew install [--cask] instead.

In addition, "brew install caskroom/cask/perforce" step triggers an error that says:

Error: caskroom/cask was moved. Tap homebrew/cask instead.

Attempt to see if blindly following the suggestion in these error messages gets us into a better shape.

---
 ci/install-dependencies.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 0229a77f7d2..0b1184e04ad 100755
@@ -44,13 +44,13 @@ osx-clang|osx-gcc)
 	test -z "$BREW_INSTALL_PACKAGES" ||
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
-	brew cask install --no-quarantine perforce || {
+	brew install --cask --no-quarantine perforce || {
 		# Update the definitions and try again
 		cask_repo="$(brew --repository)"/Library/Taps/homebrew/homebrew-cask &&
-		git -C "$cask_repo" pull --no-stat &&
-		brew cask install --no-quarantine perforce
+		git -C "$cask_repo" pull --no-stat --ff-only &&
+		brew install --cask --no-quarantine perforce
 	} ||
-	brew install caskroom/cask/perforce
+	brew install homebrew/cask/perforce
 	case "$jobname" in
 	osx-gcc)
 		brew install gcc@9

gitgitgadget

Here is version 2 of my "Simple IPC" series and addresses the following review comments:

[1] Redo packet_write_gently() to take a scratch buffer argument and fixup callers to avoid potential thread-stack problems caused by a very large stack buffer when used multi-threaded callers. This turned out to be a little more involved than anticipated because the pkt-line code doesn't know about its thread state nor an opportunity to initialize thread-state.

[2] Deleted the unix_stream_socket() helper function and inline it in the few callers and then let those call sites decide whether to call die() or not.

[3] Refactor unix_stream_listen() to take an "options" structure and to incorporate the changes I described in my earlier unix_stream_listen_gently().

[4] Update unix_stream_connect() to return errors rather than calling die().

[5] Update the simple-ipc server startup to detect dead and/or in-use Unix domain sockets and/or create a new socket in a race-friendly way. I now use a variation of the atomic lock-rename-trick when creating the socket (details are in a large comment in the code).

Jeff Hostetler (10):
pkt-line: promote static buffer in packet_write_gently() to callers
pkt-line: add write_packetized_from_buf2() that takes scratch buffer
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
simple-ipc: add t/helper/test-simple-ipc and t0052
unix-socket: elimiate static unix_stream_socket() helper function
unix-socket: add options to unix_stream_listen()
unix-socket: add no-chdir option to unix_stream_listen()
unix-socket: do not call die in unix_stream_connect()
simple-ipc: add Unix domain socket implementation

Johannes Schindelin (3):
pkt-line: optionally skip the flush packet in
write_packetized_from_buf()
pkt-line: (optionally) libify the packet readers
pkt-line: accept additional options in read_packetized_to_strbuf()

Junio C Hamano (1):
ci/install-depends: attempt to fix "brew cask" stuff

 Documentation/technical/api-simple-ipc.txt |   34 +
 Makefile                                   |    8 +
 builtin/credential-cache--daemon.c         |    3 +-
 ci/install-dependencies.sh                 |    8 +-
 compat/simple-ipc/ipc-shared.c             |   28 +
 compat/simple-ipc/ipc-unix-socket.c        | 1127 ++++++++++++++++++++
 compat/simple-ipc/ipc-win32.c              |  751 +++++++++++++
 config.mak.uname                           |    2 +
 contrib/buildsystems/CMakeLists.txt        |    6 +
 convert.c                                  |    4 +-
 pkt-line.c                                 |   70 +-
 pkt-line.h                                 |   26 +-
 simple-ipc.h                               |  230 ++++
 t/helper/test-simple-ipc.c                 |  485 +++++++++
 t/helper/test-tool.c                       |    1 +
 t/helper/test-tool.h                       |    1 +
 t/t0052-simple-ipc.sh                      |  129 +++
 unix-socket.c                              |   67 +-
 unix-socket.h                              |   16 +-
 19 files changed, 2949 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/technical/api-simple-ipc.txt
 create mode 100644 compat/simple-ipc/ipc-shared.c
 create mode 100644 compat/simple-ipc/ipc-unix-socket.c
 create mode 100644 compat/simple-ipc/ipc-win32.c
 create mode 100644 simple-ipc.h
 create mode 100644 t/helper/test-simple-ipc.c
 create mode 100755 t/t0052-simple-ipc.sh

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/766

Range-diff vs v1:

  1:  1155a45cf64 <  -:  ----------- pkt-line: use stack rather than static buffer in packet_write_gently()
  -:  ----------- >  1:  4c6766d4183 ci/install-depends: attempt to fix "brew cask" stuff
  -:  ----------- >  2:  3b03a8ff7a7 pkt-line: promote static buffer in packet_write_gently() to callers
  -:  ----------- >  3:  e671894b4c0 pkt-line: add write_packetized_from_buf2() that takes scratch buffer
  3:  edf5ac95d66 !  4:  0832f7d324d pkt-line: optionally skip the flush packet in write_packetized_from_buf()

@@ Commit message
packets before a final flush packet, so let's extend this function to prepare for that scenario.

+ Signed-off-by: Jeff Hostetler <[email protected]>

## convert.c ##
@@ pkt-line.c: int write_packetized_from_fd(int fd_in, int fd_out)
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out) +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end)
+ {
+ static struct packet_scratch_space scratch;
+
+- return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
++ return write_packetized_from_buf2(src_in, len, fd_out,
++ flush_at_end, &scratch);
+ }
+
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch)
{
int err = 0;
- size_t bytes_written = 0;
-@@ pkt-line.c: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
- err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+@@ pkt-line.c: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+ err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
bytes_written += bytes_to_write;
}
- if (!err)
@@ pkt-line.h: void packet_buf_write_len(struct strbuf *buf, const char *data, size
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end);
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch);

/*
- * Read a packetized line into the buffer, which must be at least size bytes
2: b7d678bc918 ! 5: 43bc4a26b79 pkt-line: (optionally) libify the packet readers
@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
*pktlen = -1;

## pkt-line.h ##
-@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); +@@ pkt-line.h: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
*
* If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
* ERR packet.

  4:  2f399ac107c =  6:  6a389a35335 pkt-line: accept additional options in read_packetized_to_strbuf()
  5:  7064c5e9ffa !  7:  a7275b4bdc2 simple-ipc: design documentation for new IPC mechanism
     @@ Documentation/technical/api-simple-ipc.txt (new)
      +
      +This IPC mechanism differs from the existing `sub-process.c` model
      +(Documentation/technical/long-running-process-protocol.txt) and used
     -+by applications like Git-LFS because the server is assumed to be very
     -+long running system service.  In contrast, a "sub-process model process"
     -+is started with the foreground process and exits when the foreground
     -+process terminates.  How the server is started is also outside the
     -+scope of the IPC mechanism.
     ++by applications like Git-LFS.  In the simple-ipc model the server is
     ++assumed to be a very long-running system service.  In contrast, in the
     ++LFS-style sub-process model the helper is started with the foreground
     ++process and exits when the foreground process terminates.
     ++
     ++How the simple-ipc server is started is also outside the scope of the
     ++IPC mechanism.  For example, the server might be started during
     ++maintenance operations.
      +
      +The IPC protocol consists of a single request message from the client and
      +an optional request message from the server.  For simplicity, pkt-line
  6:  9e27c07d785 !  8:  388366913d4 simple-ipc: add win32 implementation
     @@ compat/simple-ipc/ipc-win32.c (new)
      +enum ipc_active_state ipc_client_try_connect(
      +	const char *path,
      +	const struct ipc_client_connect_options *options,
     -+	int *pfd)
     ++	struct ipc_client_connection **p_connection)
      +{
      +	wchar_t wpath[MAX_PATH];
      +	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
     ++	int fd = -1;
      +
     -+	*pfd = -1;
     ++	*p_connection = NULL;
      +
      +	trace2_region_enter("ipc-client", "try-connect", NULL);
      +	trace2_data_string("ipc-client", NULL, "try-connect/path", path);
     @@ compat/simple-ipc/ipc-win32.c (new)
      +		state = IPC_STATE__INVALID_PATH;
      +	else
      +		state = connect_to_server(wpath, WINDOWS_CONNECTION_TIMEOUT_MS,
     -+					  options, pfd);
     ++					  options, &fd);
      +
      +	trace2_data_intmax("ipc-client", NULL, "try-connect/state",
      +			   (intmax_t)state);
      +	trace2_region_leave("ipc-client", "try-connect", NULL);
     ++
     ++	if (state == IPC_STATE__LISTENING) {
     ++		(*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
     ++		(*p_connection)->fd = fd;
     ++	}
     ++
      +	return state;
      +}
      +
     -+int ipc_client_send_command_to_fd(int fd, const char *message,
     -+				  struct strbuf *answer)
     ++void ipc_client_close_connection(struct ipc_client_connection *connection)
     ++{
     ++	if (!connection)
     ++		return;
     ++
     ++	if (connection->fd != -1)
     ++		close(connection->fd);
     ++
     ++	free(connection);
     ++}
     ++
     ++int ipc_client_send_command_to_connection(
     ++	struct ipc_client_connection *connection,
     ++	const char *message, struct strbuf *answer)
      +{
      +	int ret = 0;
      +
     @@ compat/simple-ipc/ipc-win32.c (new)
      +
      +	trace2_region_enter("ipc-client", "send-command", NULL);
      +
     -+	if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
     ++	if (write_packetized_from_buf2(message, strlen(message),
     ++				       connection->fd, 1,
     ++				       &connection->scratch_write_buffer) < 0) {
      +		ret = error(_("could not send IPC command"));
      +		goto done;
      +	}
      +
     -+	FlushFileBuffers((HANDLE)_get_osfhandle(fd));
     ++	FlushFileBuffers((HANDLE)_get_osfhandle(connection->fd));
      +
     -+	if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
     ++	if (read_packetized_to_strbuf(connection->fd, answer,
     ++				      PACKET_READ_NEVER_DIE) < 0) {
      +		ret = error(_("could not read IPC response"));
      +		goto done;
      +	}
     @@ compat/simple-ipc/ipc-win32.c (new)
      +			    const struct ipc_client_connect_options *options,
      +			    const char *message, struct strbuf *response)
      +{
     -+	int fd;
      +	int ret = -1;
      +	enum ipc_active_state state;
     ++	struct ipc_client_connection *connection = NULL;
      +
     -+	state = ipc_client_try_connect(path, options, &fd);
     ++	state = ipc_client_try_connect(path, options, &connection);
      +
      +	if (state != IPC_STATE__LISTENING)
      +		return ret;
      +
     -+	ret = ipc_client_send_command_to_fd(fd, message, response);
     -+	close(fd);
     ++	ret = ipc_client_send_command_to_connection(connection, message, response);
     ++
     ++	ipc_client_close_connection(connection);
     ++
      +	return ret;
      +}
      +
     @@ compat/simple-ipc/ipc-win32.c (new)
      +	struct ipc_server_data *server_data;
      +	pthread_t pthread_id;
      +	HANDLE hPipe;
     ++	struct packet_scratch_space scratch_write_buffer;
      +};
      +
      +/*
     @@ compat/simple-ipc/ipc-win32.c (new)
      +static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
      +		       const char *response, size_t response_len)
      +{
     ++	struct packet_scratch_space *scratch =
     ++		&reply_data->server_thread_data->scratch_write_buffer;
     ++
      +	if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
      +		BUG("reply_cb called with wrong instance data");
      +
     -+	return write_packetized_from_buf(response, response_len,
     -+					 reply_data->fd, 0);
     ++	return write_packetized_from_buf2(response, response_len,
     ++					  reply_data->fd, 0, scratch);
      +}
      +
      +/*
     @@ simple-ipc.h (new)
      +#endif
      +
      +#ifdef SUPPORTS_SIMPLE_IPC
     ++#include "pkt-line.h"
      +
      +/*
      + * Simple IPC Client Side API.
     @@ simple-ipc.h (new)
      + */
      +enum ipc_active_state ipc_get_active_state(const char *path);
      +
     ++struct ipc_client_connection {
     ++	int fd;
     ++	struct packet_scratch_space scratch_write_buffer;
     ++};
     ++
      +/*
      + * Try to connect to the daemon on the named pipe or socket.
      + *
     -+ * Returns IPC_STATE__LISTENING (and an fd) when connected.
     ++ * Returns IPC_STATE__LISTENING and a connection handle.
      + *
      + * Otherwise, returns info to help decide whether to retry or to
      + * spawn/respawn the server.
     @@ simple-ipc.h (new)
      +enum ipc_active_state ipc_client_try_connect(
      +	const char *path,
      +	const struct ipc_client_connect_options *options,
     -+	int *pfd);
     ++	struct ipc_client_connection **p_connection);
     ++
     ++void ipc_client_close_connection(struct ipc_client_connection *connection);
      +
      +/*
      + * Used by the client to synchronously send and receive a message with
     -+ * the server on the provided fd.
     ++ * the server on the provided client connection.
      + *
      + * Returns 0 when successful.
      + *
      + * Calls error() and returns non-zero otherwise.
      + */
     -+int ipc_client_send_command_to_fd(int fd, const char *message,
     -+				  struct strbuf *answer);
     ++int ipc_client_send_command_to_connection(
     ++	struct ipc_client_connection *connection,
     ++	const char *message, struct strbuf *answer);
      +
      +/*
      + * Used by the client to synchronously connect and send and receive a
  9:  69969c2b8d3 =  9:  f0bebf1cdb3 simple-ipc: add t/helper/test-simple-ipc and t0052
  -:  ----------- > 10:  f5d5445cf42 unix-socket: elimiate static unix_stream_socket() helper function
  7:  96268351ac6 ! 11:  7a6a69dfc20 unix-socket: create gentle version of unix_stream_listen()
     @@ Metadata
      Author: Jeff Hostetler <[email protected]>
      
       ## Commit message ##
     -    unix-socket: create gentle version of unix_stream_listen()
     +    unix-socket: add options to unix_stream_listen()
      
     -    Create a gentle version of `unix_stream_listen()`.  This version does
     -    not call `die()` if a socket-fd cannot be created and does not assume
     -    that it is safe to `unlink()` an existing socket-inode.
     +    Update `unix_stream_listen()` to take an options structure to override
     +    default behaviors.  This includes the size of the `listen()` backlog
     +    and whether it should always unlink the socket file before trying to
     +    create a new one.  Also eliminate calls to `die()` if it cannot create
     +    a socket.
      
     -    `unix_stream_listen()` uses `unix_stream_socket()` helper function to
     -    create the socket-fd.  Avoid that helper because it calls `die()` on
     -    errors.
     -
     -    `unix_stream_listen()` always tries to `unlink()` the socket-path before
     -    calling `bind()`.  If there is an existing server/daemon already bound
     -    and listening on that socket-path, our `unlink()` would have the effect
     -    of disassociating the existing server's bound-socket-fd from the socket-path
     -    without notifying the existing server.  The existing server could continue
     -    to service existing connections (accepted-socket-fd's), but would not
     -    receive any futher new connections (since clients rendezvous via the
     -    socket-path).  The existing server would effectively be offline but yet
     -    appear to be active.
     +    Normally, `unix_stream_listen()` always tries to `unlink()` the
     +    socket-path before calling `bind()`.  If there is an existing
     +    server/daemon already bound and listening on that socket-path, our
     +    `unlink()` would have the effect of disassociating the existing
     +    server's bound-socket-fd from the socket-path without notifying the
     +    existing server.  The existing server could continue to service
     +    existing connections (accepted-socket-fd's), but would not receive any
     +    futher new connections (since clients rendezvous via the socket-path).
     +    The existing server would effectively be offline but yet appear to be
     +    active.
      
          Furthermore, `unix_stream_listen()` creates an opportunity for a brief
          race condition for connecting clients if they try to connect in the
     @@ Commit message
      
          Signed-off-by: Jeff Hostetler <[email protected]>
      
     + ## builtin/credential-cache--daemon.c ##
     +@@ builtin/credential-cache--daemon.c: static int serve_cache_loop(int fd)
     + 
     + static void serve_cache(const char *socket_path, int debug)
     + {
     ++	struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
     + 	int fd;
     + 
     +-	fd = unix_stream_listen(socket_path);
     ++	fd = unix_stream_listen(socket_path, &opts);
     + 	if (fd < 0)
     + 		die_errno("unable to bind to '%s'", socket_path);
     + 
     +
       ## unix-socket.c ##
     -@@ unix-socket.c: int unix_stream_listen(const char *path)
     - 	errno = saved_errno;
     +@@ unix-socket.c: int unix_stream_connect(const char *path)
       	return -1;
       }
     -+
     -+int unix_stream_listen_gently(const char *path,
     -+			      const struct unix_stream_listen_opts *opts)
     -+{
     + 
     +-int unix_stream_listen(const char *path)
     ++int unix_stream_listen(const char *path,
     ++		       const struct unix_stream_listen_opts *opts)
     + {
     +-	int fd, saved_errno;
      +	int fd = -1;
     -+	int bind_successful = 0;
      +	int saved_errno;
     -+	struct sockaddr_un sa;
     -+	struct unix_sockaddr_context ctx;
     -+
     -+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
     -+		goto fail;
     ++	int bind_successful = 0;
     ++	int backlog;
     + 	struct sockaddr_un sa;
     + 	struct unix_sockaddr_context ctx;
     + 
     +-	unlink(path);
     +-
     + 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
     + 		return -1;
      +
     -+	fd = socket(AF_UNIX, SOCK_STREAM, 0);
     -+	if (fd < 0)
     + 	fd = socket(AF_UNIX, SOCK_STREAM, 0);
     + 	if (fd < 0)
     +-		die_errno("unable to create socket");
      +		goto fail;
      +
      +	if (opts->force_unlink_before_bind)
      +		unlink(path);
     -+
     -+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
     -+		goto fail;
     + 
     + 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
     + 		goto fail;
      +	bind_successful = 1;
     -+
     -+	if (listen(fd, opts->listen_backlog_size) < 0)
     -+		goto fail;
     -+
     -+	unix_sockaddr_cleanup(&ctx);
     -+	return fd;
     -+
     -+fail:
     -+	saved_errno = errno;
     -+	unix_sockaddr_cleanup(&ctx);
     -+	close(fd);
     + 
     +-	if (listen(fd, 5) < 0)
     ++	if (opts->listen_backlog_size > 0)
     ++		backlog = opts->listen_backlog_size;
     ++	else
     ++		backlog = 5;
     ++	if (listen(fd, backlog) < 0)
     + 		goto fail;
     + 
     + 	unix_sockaddr_cleanup(&ctx);
     +@@ unix-socket.c: int unix_stream_listen(const char *path)
     + fail:
     + 	saved_errno = errno;
     + 	unix_sockaddr_cleanup(&ctx);
     +-	close(fd);
     ++	if (fd != -1)
     ++		close(fd);
      +	if (bind_successful)
      +		unlink(path);
     -+	errno = saved_errno;
     -+	return -1;
     -+}
     + 	errno = saved_errno;
     + 	return -1;
     + }
      
       ## unix-socket.h ##
      @@
     - int unix_stream_connect(const char *path);
     - int unix_stream_listen(const char *path);
     + #ifndef UNIX_SOCKET_H
     + #define UNIX_SOCKET_H
       
      +struct unix_stream_listen_opts {
      +	int listen_backlog_size;
      +	unsigned int force_unlink_before_bind:1;
      +};
      +
     -+int unix_stream_listen_gently(const char *path,
     -+			      const struct unix_stream_listen_opts *opts);
     ++#define UNIX_STREAM_LISTEN_OPTS_INIT \
     ++{ \
     ++	.listen_backlog_size = 5, \
     ++	.force_unlink_before_bind = 1, \
     ++}
      +
     + int unix_stream_connect(const char *path);
     +-int unix_stream_listen(const char *path);
     ++int unix_stream_listen(const char *path,
     ++		       const struct unix_stream_listen_opts *opts);
     + 
       #endif /* UNIX_SOCKET_H */

8: 383a9755669 ! 12: 745b6d5fb74 unix-socket: add no-chdir option to unix_stream_listen_gently()
@@ Metadata
Author: Jeff Hostetler <[email protected]>

## Commit message ##
- unix-socket: add no-chdir option to unix_stream_listen_gently()
+ unix-socket: add no-chdir option to unix_stream_listen()

Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() is given a socket pathname that is too big to
@@ Commit message
Teach unix_sockaddr_init() to not allow calls to chdir() when flag is set.

- Extend the public interface to unix_stream_listen_gently() to also
- expose this new flag.
-

## unix-socket.c ##
@@ unix-socket.c: int unix_stream_connect(const char *path)

if (unix_sockaddr_init(&sa, path, &ctx) < 0)
return -1;
-@@ unix-socket.c: int unix_stream_listen(const char *path)
- {
- int fd, saved_errno;
- struct sockaddr_un sa;
-- struct unix_sockaddr_context ctx;
-+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
-
- unlink(path);
-
-@@ unix-socket.c: int unix_stream_listen_gently(const char *path, +@@ unix-socket.c: int unix_stream_listen(const char *path,
int bind_successful = 0;
- int saved_errno;
+ int backlog;
struct sockaddr_un sa;
- struct unix_sockaddr_context ctx;
+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
@@ unix-socket.c: int unix_stream_listen_gently(const char *path,
+ ctx.disallow_chdir = opts->disallow_chdir;

if (unix_sockaddr_init(&sa, path, &ctx) < 0)
- goto fail;
+ return -1;

## unix-socket.h ##
-@@ unix-socket.h: int unix_stream_listen(const char *path); +@@
struct unix_stream_listen_opts {
int listen_backlog_size;
unsigned int force_unlink_before_bind:1;
+ unsigned int disallow_chdir:1;
};

- int unix_stream_listen_gently(const char *path,
+ #define UNIX_STREAM_LISTEN_OPTS_INIT \
+ { \
+ .listen_backlog_size = 5, \
+ .force_unlink_before_bind = 1, \
++ .disallow_chdir = 0, \
+ }
+
+ int unix_stream_connect(const char *path);

  -:  ----------- > 13:  2cca15a10ec unix-socket: do not call die in unix_stream_connect()
 10:  a1b15fb5cb0 ! 14:  72c1c209c38 simple-ipc: add Unix domain socket implementation
     @@ Commit message
      
          Create Unix domain socket based implementation of "simple-ipc".
      
     +    A set of `ipc_client` routines implement a client library to connect
     +    to an `ipc_server` over a Unix domain socket, send a simple request,
     +    and receive a single response.  Clients use blocking IO on the socket.
     +
     +    A set of `ipc_server` routines implement a thread pool to listen for
     +    and concurrently service client connections.
     +
     +    The server creates a new Unix domain socket at a known location.  If a
     +    socket already exists with that name, the server tries to determine if
     +    another server is already listening on the socket or if the socket is
     +    dead.  If socket is busy, the server exits with an error rather than
     +    stealing the socket.  If the socket is dead, the server creates a new
     +    one and starts up.
     +
     +    If while running, the server detects that its socket has been stolen
     +    by another server, it automatically exits.
     +
          Signed-off-by: Jeff Hostetler <[email protected]>
      
       ## Makefile ##
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	struct ipc_client_connect_options options
      +		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
      +	struct stat st;
     -+	int fd_test = -1;
     ++	struct ipc_client_connection *connection_test = NULL;
      +
      +	options.wait_if_busy = 0;
      +	options.wait_if_not_found = 0;
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	 * at `path`, doesn't mean it that there is a server listening.
      +	 * Ping it to be sure.
      +	 */
     -+	state = ipc_client_try_connect(path, &options, &fd_test);
     -+	close(fd_test);
     ++	state = ipc_client_try_connect(path, &options, &connection_test);
     ++	ipc_client_close_connection(connection_test);
      +
      +	return state;
      +}
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +enum ipc_active_state ipc_client_try_connect(
      +	const char *path,
      +	const struct ipc_client_connect_options *options,
     -+	int *pfd)
     ++	struct ipc_client_connection **p_connection)
      +{
      +	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
     ++	int fd = -1;
      +
     -+	*pfd = -1;
     ++	*p_connection = NULL;
      +
      +	trace2_region_enter("ipc-client", "try-connect", NULL);
      +	trace2_data_string("ipc-client", NULL, "try-connect/path", path);
      +
      +	state = connect_to_server(path, MY_CONNECTION_TIMEOUT_MS,
     -+				  options, pfd);
     ++				  options, &fd);
      +
      +	trace2_data_intmax("ipc-client", NULL, "try-connect/state",
      +			   (intmax_t)state);
      +	trace2_region_leave("ipc-client", "try-connect", NULL);
     ++
     ++	if (state == IPC_STATE__LISTENING) {
     ++		(*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
     ++		(*p_connection)->fd = fd;
     ++	}
     ++
      +	return state;
      +}
      +
     -+int ipc_client_send_command_to_fd(int fd, const char *message,
     -+				  struct strbuf *answer)
     ++void ipc_client_close_connection(struct ipc_client_connection *connection)
     ++{
     ++	if (!connection)
     ++		return;
     ++
     ++	if (connection->fd != -1)
     ++		close(connection->fd);
     ++
     ++	free(connection);
     ++}
     ++
     ++int ipc_client_send_command_to_connection(
     ++	struct ipc_client_connection *connection,
     ++	const char *message, struct strbuf *answer)
      +{
      +	int ret = 0;
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +
      +	trace2_region_enter("ipc-client", "send-command", NULL);
      +
     -+	if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
     ++	if (write_packetized_from_buf2(message, strlen(message),
     ++				       connection->fd, 1,
     ++				       &connection->scratch_write_buffer) < 0) {
      +		ret = error(_("could not send IPC command"));
      +		goto done;
      +	}
      +
     -+	if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
     ++	if (read_packetized_to_strbuf(connection->fd, answer,
     ++				      PACKET_READ_NEVER_DIE) < 0) {
      +		ret = error(_("could not read IPC response"));
      +		goto done;
      +	}
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +			    const struct ipc_client_connect_options *options,
      +			    const char *message, struct strbuf *answer)
      +{
     -+	int fd;
      +	int ret = -1;
      +	enum ipc_active_state state;
     ++	struct ipc_client_connection *connection = NULL;
      +
     -+	state = ipc_client_try_connect(path, options, &fd);
     ++	state = ipc_client_try_connect(path, options, &connection);
      +
      +	if (state != IPC_STATE__LISTENING)
      +		return ret;
      +
     -+	ret = ipc_client_send_command_to_fd(fd, message, answer);
     -+	close(fd);
     ++	ret = ipc_client_send_command_to_connection(connection, message, answer);
     ++
     ++	ipc_client_close_connection(connection);
     ++
      +	return ret;
      +}
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	struct ipc_worker_thread_data *next_thread;
      +	struct ipc_server_data *server_data;
      +	pthread_t pthread_id;
     ++	struct packet_scratch_space scratch_write_buffer;
      +};
      +
      +struct ipc_accept_thread_data {
      +	enum magic magic;
      +	struct ipc_server_data *server_data;
     ++
      +	int fd_listen;
     -+	ino_t inode_listen;
     ++	struct stat st_listen;
     ++
      +	int fd_send_shutdown;
      +	int fd_wait_shutdown;
      +	pthread_t pthread_id;
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
      +		       const char *response, size_t response_len)
      +{
     ++	struct packet_scratch_space *scratch =
     ++		&reply_data->worker_thread_data->scratch_write_buffer;
     ++
      +	if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
      +		BUG("reply_cb called with wrong instance data");
      +
     -+	return write_packetized_from_buf(response, response_len,
     -+					 reply_data->fd, 0);
     ++	return write_packetized_from_buf2(response, response_len,
     ++					  reply_data->fd, 0, scratch);
      +}
      +
      +/* A randomly chosen value. */
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +static int socket_was_stolen(struct ipc_accept_thread_data *accept_thread_data)
      +{
      +	struct stat st;
     ++	struct stat *ref_st = &accept_thread_data->st_listen;
      +
      +	if (lstat(accept_thread_data->server_data->buf_path.buf, &st) == -1)
      +		return 1;
      +
     -+	if (st.st_ino != accept_thread_data->inode_listen)
     ++	if (st.st_ino != ref_st->st_ino)
      +		return 1;
      +
     ++	/* We might also consider the creation time on some platforms. */
     ++
      +	return 0;
      +}
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      + * open/connect using the "socket-inode" pathname.
      + *
      + * Unix domain sockets have a fundamental design flaw because the
     -+ * "socket-inode" persists until the pathname is deleted; closing the listening
     -+ * "socket-fd" only closes the socket handle/descriptor, it does not delete
     -+ * the inode/pathname.
     ++ * "socket-inode" persists until the pathname is deleted; closing the
     ++ * listening "socket-fd" only closes the socket handle/descriptor, it
     ++ * does not delete the inode/pathname.
      + *
      + * Well-behaving service daemons are expected to also delete the inode
      + * before shutdown.  If a service crashes (or forgets) it can leave
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      + * inode *or* another service instance is already running.
      + *
      + * One possible solution is to blindly unlink the inode before
     -+ * attempting to bind a new socket-fd (and thus create) a new
     ++ * attempting to bind a new socket-fd and thus create a new
      + * socket-inode.  Then `bind(2)` should always succeed.  However, if
     -+ * there is an existing service instance, it would be orphaned --
     -+ * it would still be listening on a socket-fd that is still bound
     -+ * to an (unlinked) socket-inode, but that socket-inode is no longer
     ++ * there is an existing service instance, it would be orphaned -- it
     ++ * would still be listening on a socket-fd that is still bound to an
     ++ * (unlinked) socket-inode, but that socket-inode is no longer
      + * associated with the pathname.  New client connections will arrive
     -+ * at our new socket-inode and not the existing server's.  (It is upto
     -+ * the existing server to detect that its socket-inode has been
     -+ * stolen and shutdown.)
     ++ * at OUR new socket-inode -- rather than the existing server's
     ++ * socket.  (I suppose it is up to the existing server to detect that
     ++ * its socket-inode has been stolen and shutdown.)
     ++ *
     ++ * Another possible solution is to try to use the ".lock" trick, but
     ++ * bind() does not have a exclusive-create use bit like open() does,
     ++ * so we cannot have multiple servers fighting/racing to create the
     ++ * same file name without having losers lose without knowing that they
     ++ * lost.
     ++ *
     ++ * We try to avoid such stealing and would rather fail to run than
     ++ * steal an existing socket-inode (because we assume that the
     ++ * existing server has more context and value to the clients than a
     ++ * freshly started server).  However, if multiple servers are racing
     ++ * to start, we don't care which one wins -- none of them have any
     ++ * state information yet worth fighting for.
     ++ *
     ++ * Create a "unique" socket-inode (with our PID in it (and assume that
     ++ * we can force-delete an existing socket with that name)).  Stat it
     ++ * to get the inode number and ctime -- so that we can identify it as
     ++ * the one we created.  Then use the atomic-rename trick to install it
     ++ * in the real location.  (This will unlink an existing socket with
     ++ * that pathname -- and thereby steal the real socket-inode from an
     ++ * existing server.)
      + *
     -+ * Since this is rather obscure and infrequent, we try to "gently"
     -+ * create the socket-inode without disturbing an existing service.
     ++ * Elsewhere, our thread will periodically poll the socket-inode to
     ++ * see if someone else steals ours.
      + */
      +static int create_listener_socket(const char *path,
     -+				  const struct ipc_server_opts *ipc_opts)
     ++				  const struct ipc_server_opts *ipc_opts,
     ++				  struct stat *st_socket)
      +{
     ++	struct stat st;
     ++	struct strbuf buf_uniq = STRBUF_INIT;
      +	int fd_listen;
     -+	int fd_client;
     -+	struct unix_stream_listen_opts uslg_opts = {
     -+		.listen_backlog_size = LISTEN_BACKLOG,
     -+		.force_unlink_before_bind = 0,
     -+		.disallow_chdir = ipc_opts->uds_disallow_chdir
     -+	};
     -+
     -+	trace2_data_string("ipc-server", NULL, "try-listen-gently", path);
     -+
     -+	/*
     -+	 * Assume socket-inode does not exist and try to (gently)
     -+	 * create a new socket-inode on disk at pathname and bind
     -+	 * socket-fd to it.
     -+	 */
     -+	fd_listen = unix_stream_listen_gently(path, &uslg_opts);
     -+	if (fd_listen >= 0)
     -+		return fd_listen;
     ++	struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
      +
     -+	if (errno != EADDRINUSE)
     -+		return error_errno(_("could not create socket '%s'"),
     -+				   path);
     -+
     -+	trace2_data_string("ipc-server", NULL, "try-detect-server", path);
     -+
     -+	/*
     -+	 * A socket-inode at pathname exists on disk, but we don't
     -+	 * know if it a server is using it or if it is a stale inode.
     -+	 *
     -+	 * poke it with a trivial connection to try to find out.
     -+	 */
     -+	fd_client = unix_stream_connect(path);
     -+	if (fd_client >= 0) {
     ++	if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
     ++		int fd_client;
      +		/*
     -+		 * An existing service process is alive and accepted our
     -+		 * connection.
     ++		 * A socket-inode at `path` exists on disk, but we
     ++		 * don't know whether it belongs to an active server
     ++		 * or if the last server died without cleaning up.
     ++		 *
     ++		 * Poke it with a trivial connection to try to find out.
      +		 */
     -+		close(fd_client);
     -+
     -+		/*
     -+		 * We cannot create a new socket-inode here, so we cannot
     -+		 * startup a new server on this pathname.
     -+		 */
     -+		errno = EADDRINUSE;
     -+		return error_errno(_("socket already in use '%s'"),
     ++		trace2_data_string("ipc-server", NULL, "try-detect-server",
      +				   path);
     ++		fd_client = unix_stream_connect(path);
     ++		if (fd_client >= 0) {
     ++			close(fd_client);
     ++			errno = EADDRINUSE;
     ++			return error_errno(_("socket already in use '%s'"),
     ++					   path);
     ++		}
      +	}
      +
     -+	trace2_data_string("ipc-server", NULL, "try-listen-force", path);
     -+
      +	/*
     -+	 * A socket-inode at pathname exists on disk, but we were not
     -+	 * able to connect to it, so we believe that this is a stale
     -+	 * socket-inode that a previous server forgot to delete.  Use
     -+	 * the tradional solution: force unlink it and create a new
     -+	 * one.
     -+	 *
     -+	 * TODO Note that it is possible that another server is
     -+	 * listening, but is either just starting up and not yet
     -+	 * responsive or is stuck somehow.  For now, I'm OK with
     -+	 * stealing the socket-inode from it in this case.
     ++	 * Create pathname to our "unique" socket and set it up for
     ++	 * business.
      +	 */
     -+	uslg_opts.force_unlink_before_bind = 1;
     -+	fd_listen = unix_stream_listen_gently(path, &uslg_opts);
     -+	if (fd_listen >= 0)
     -+		return fd_listen;
     -+
     -+	return error_errno(_("could not force create socket '%s'"), path);
     -+}
     -+
     -+static int setup_listener_socket(const char *path, ino_t *inode,
     -+				 const struct ipc_server_opts *ipc_opts)
     -+{
     -+	int fd_listen;
     -+	struct stat st;
     -+
     -+	trace2_region_enter("ipc-server", "create-listener_socket", NULL);
     -+	fd_listen = create_listener_socket(path, ipc_opts);
     -+	trace2_region_leave("ipc-server", "create-listener_socket", NULL);
     ++	strbuf_addf(&buf_uniq, "%s.%d", path, getpid());
      +
     -+	if (fd_listen < 0)
     -+		return fd_listen;
     -+
     -+	/*
     -+	 * We just bound a socket (descriptor) to a newly created unix
     -+	 * domain socket in the filesystem.  Capture the inode number
     -+	 * so we can later detect if/when someone else force-creates a
     -+	 * new socket and effectively steals the path from us.  (Which
     -+	 * would leave us listening to a socket that no client could
     -+	 * reach.)
     -+	 */
     -+	if (lstat(path, &st) < 0) {
     ++	uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
     ++	uslg_opts.force_unlink_before_bind = 1;
     ++	uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
     ++	fd_listen = unix_stream_listen(buf_uniq.buf, &uslg_opts);
     ++	if (fd_listen < 0) {
      +		int saved_errno = errno;
     ++		error_errno(_("could not create listener socket '%s'"),
     ++			    buf_uniq.buf);
     ++		strbuf_release(&buf_uniq);
     ++		errno = saved_errno;
     ++		return -1;
     ++	}
      +
     ++	if (lstat(buf_uniq.buf, st_socket)) {
     ++		int saved_errno = errno;
     ++		error_errno(_("could not stat listener socket '%s'"),
     ++			    buf_uniq.buf);
      +		close(fd_listen);
     -+		unlink(path);
     -+
     ++		unlink(buf_uniq.buf);
     ++		strbuf_release(&buf_uniq);
      +		errno = saved_errno;
     -+		return error_errno(_("could not lstat listener socket '%s'"),
     -+				   path);
     ++		return -1;
      +	}
      +
      +	if (set_socket_blocking_flag(fd_listen, 1)) {
      +		int saved_errno = errno;
     -+
     ++		error_errno(_("could not set listener socket nonblocking '%s'"),
     ++			    buf_uniq.buf);
      +		close(fd_listen);
     -+		unlink(path);
     ++		unlink(buf_uniq.buf);
     ++		strbuf_release(&buf_uniq);
     ++		errno = saved_errno;
     ++		return -1;
     ++	}
      +
     ++	/*
     ++	 * Install it as the "real" socket so that clients will starting
     ++	 * connecting to our socket.
     ++	 */
     ++	if (rename(buf_uniq.buf, path)) {
     ++		int saved_errno = errno;
     ++		error_errno(_("could not create listener socket '%s'"), path);
     ++		close(fd_listen);
     ++		unlink(buf_uniq.buf);
     ++		strbuf_release(&buf_uniq);
      +		errno = saved_errno;
     -+		return error_errno(_("making listener socket nonblocking '%s'"),
     -+				   path);
     ++		return -1;
      +	}
      +
     -+	*inode = st.st_ino;
     ++	strbuf_release(&buf_uniq);
     ++	trace2_data_string("ipc-server", NULL, "try-listen", path);
     ++	return fd_listen;
     ++}
     ++
     ++static int setup_listener_socket(const char *path, struct stat *st_socket,
     ++				 const struct ipc_server_opts *ipc_opts)
     ++{
     ++	int fd_listen;
     ++
     ++	trace2_region_enter("ipc-server", "create-listener_socket", NULL);
     ++	fd_listen = create_listener_socket(path, ipc_opts, st_socket);
     ++	trace2_region_leave("ipc-server", "create-listener_socket", NULL);
      +
      +	return fd_listen;
      +}
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +{
      +	struct ipc_server_data *server_data;
      +	int fd_listen;
     -+	ino_t inode_listen;
     ++	struct stat st_listen;
      +	int sv[2];
      +	int k;
      +	int nr_threads = opts->nr_threads;
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +				   path);
      +	}
      +
     -+	fd_listen = setup_listener_socket(path, &inode_listen, opts);
     ++	fd_listen = setup_listener_socket(path, &st_listen, opts);
      +	if (fd_listen < 0) {
      +		int saved_errno = errno;
      +		close(sv[0]);
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	server_data->accept_thread->magic = MAGIC_ACCEPT_THREAD_DATA;
      +	server_data->accept_thread->server_data = server_data;
      +	server_data->accept_thread->fd_listen = fd_listen;
     -+	server_data->accept_thread->inode_listen = inode_listen;
     ++	server_data->accept_thread->st_listen = st_listen;
      +	server_data->accept_thread->fd_send_shutdown = sv[0];
      +	server_data->accept_thread->fd_wait_shutdown = sv[1];
      +

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function currently has only one caller: apply_multi_file_filter() in convert.c. That caller wants a flush packet to be written after writing the payload.

However, we are about to introduce a user that wants to write many packets before a final flush packet, so let's extend this function to prepare for that scenario.

---
 convert.c  | 2 +-
 pkt-line.c | 9 ++++++---
 pkt-line.h | 4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)

index ee360c2f07c..3f396a9b288 100644
@@ -886,7 +886,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (fd >= 0)
 		err = write_packetized_from_fd(fd, process->in);
 	else
-		err = write_packetized_from_buf(src, len, process->in);
+		err = write_packetized_from_buf(src, len, process->in, 1);
 	if (err)
 		goto done;
 

index 5d86354cbeb..d91a1deda95 100644
@@ -275,14 +275,17 @@ int write_packetized_from_fd(int fd_in, int fd_out)
 	return err;
 }
 
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end)
 {
 	static struct packet_scratch_space scratch;
 
-	return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
+	return write_packetized_from_buf2(src_in, len, fd_out,
+					  flush_at_end, &scratch);
 }
 
 int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+			       int flush_at_end,
 			       struct packet_scratch_space *scratch)
 {
 	int err = 0;

@@ -299,7 +302,7 @@ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
 		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
 		bytes_written += bytes_to_write;
 	}
-	if (!err)
+	if (!err && flush_at_end)
 		err = packet_flush_gently(fd_out);
 	return err;
 }

index f1d5625e91f..ccf27549227 100644
@@ -40,8 +40,10 @@ void 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_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end);
 int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+			       int flush_at_end,
 			       struct packet_scratch_space *scratch);
 
 /*

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

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 "Internal FSMonitor" feature in a later patch series.

---
 pkt-line.c | 19 +++++++++++++++++--
 pkt-line.h |  4 ++++
 2 files changed, 21 insertions(+), 2 deletions(-)

index d91a1deda95..528493bca21 100644
@@ -323,8 +323,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_NEVER_DIE)
+				return error_errno(_("read error"));
 			die_errno(_("read error"));
+		}
 	}
 
 	/* And complain if we didn't get enough bytes to satisfy the read. */

@@ -332,6 +335,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_NEVER_DIE)
+			return error(_("the remote end hung up unexpectedly"));
 		die(_("the remote end hung up unexpectedly"));
 	}
 

@@ -360,6 +365,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_NEVER_DIE)
+			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);

@@ -374,12 +382,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_NEVER_DIE)
+			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_NEVER_DIE)
+			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;

index ccf27549227..7f31c892165 100644
@@ -79,10 +79,14 @@ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
  *
  * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
  * ERR packet.
+ *
+ * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
+ * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
  */
 #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_NEVER_DIE         (1u<<3)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The read_packetized_to_strbuf() function reads packets into a strbuf until a flush packet has been received. So far, it has only one caller:
apply_multi_file_filter() in convert.c. This caller really only needs the PACKET_READ_GENTLE_ON_EOF option to be passed to packet_read() (which makes sense in the scenario where packets should be read until a flush packet is received).

We are about to introduce a caller that wants to pass other options through to packet_read(), so let's extend the function signature accordingly.

---
 convert.c  | 2 +-
 pkt-line.c | 4 ++--
 pkt-line.h | 6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

index 3f396a9b288..175c5cd51d5 100644
@@ -903,7 +903,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 		if (err)
 			goto done;
 
-		err = read_packetized_to_strbuf(process->out, &nbuf) < 0;
+		err = read_packetized_to_strbuf(process->out, &nbuf, 0) < 0;
 		if (err)
 			goto done;
 

index 528493bca21..f090fc56eef 100644
@@ -461,7 +461,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;
 

@@ -477,7 +477,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 | PACKET_READ_GENTLE_ON_EOF);
 		if (packet_len <= 0)
 			break;
 		sb_out->len += packet_len;

index 7f31c892165..150319a6f00 100644
@@ -145,8 +145,12 @@ 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.
+ *
+ * The options are augmented by PACKET_READ_GENTLE_ON_EOF and passed to
+ * packet_read.
  */
-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.

gitgitgadget

From: 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 | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/technical/api-simple-ipc.txt

new file mode 100644
index 00000000000..670a5c163e3
@@ -0,0 +1,34 @@
+simple-ipc API
+==============
+
+The simple-ipc API is used to send an IPC message and response between
+a (presumably) foreground Git client process to a background server or
+daemon process.  The server process must already be running.  Multiple
+client processes can simultaneously communicate with the server
+process.
+
+Communication occurs over a named pipe on Windows and a Unix domain
+socket on other platforms.  Clients and the server rendezvous at a
+previously agreed-to application-specific pathname (which is outside
+the scope of this design).
+
+This IPC mechanism differs from the existing `sub-process.c` model
+(Documentation/technical/long-running-process-protocol.txt) and used
+by applications like Git-LFS.  In the simple-ipc model the server is
+assumed to be a very long-running system service.  In contrast, in the
+LFS-style sub-process model the helper is started with the foreground
+process and exits when the foreground process terminates.
+
+How the simple-ipc server is started is also outside the scope of the
+IPC mechanism.  For example, the server might be started during
+maintenance operations.
+
+The IPC protocol consists of a single request message from the client and
+an optional request message from the server.  For simplicity, pkt-line
+routines are used to hide chunking and buffering concerns.  Each side
+terminates their message with a flush packet.
+(Documentation/technical/protocol-common.txt)
+
+The actual format of the client and server messages is application
+specific.  The IPC layer transmits and receives an opaque buffer without
+any concern for the content within.

gitgitgadget

From: Jeff Hostetler <[email protected]>

Update unix_stream_listen() to take an options structure to override default behaviors. This includes the size of the listen() backlog and whether it should always unlink the socket file before trying to create a new one. Also eliminate calls to die() if it cannot create a socket.

Normally, unix_stream_listen() always tries to unlink() the socket-path before calling bind(). If there is an existing server/daemon already bound and listening on that socket-path, our unlink() would have the effect of disassociating the existing server's bound-socket-fd from the socket-path without notifying the existing server. The existing server could continue to service existing connections (accepted-socket-fd's), but would not receive any futher new connections (since clients rendezvous via the socket-path). The existing server would effectively be offline but yet appear to be active.

Furthermore, unix_stream_listen() creates an opportunity for a brief race condition for connecting clients if they try to connect in the interval between the forced unlink() and the subsequent bind() (which recreates the socket-path that is bound to a new socket-fd in the current process).

---
 builtin/credential-cache--daemon.c |  3 ++-
 unix-socket.c                      | 28 +++++++++++++++++++++-------
 unix-socket.h                      | 14 +++++++++++++-
 3 files changed, 36 insertions(+), 9 deletions(-)

index c61f123a3b8..4c6c89ab0de 100644
@@ -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);
 

index ef2aeb46bcd..8bcef18ea55 100644
@@ -88,24 +88,35 @@ 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, saved_errno;
+	int fd = -1;
+	int saved_errno;
+	int bind_successful = 0;
+	int backlog;
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx;
 
-	unlink(path);
-
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
+
 	fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (fd < 0)
-		die_errno("unable to create socket");
+		goto fail;
+
+	if (opts->force_unlink_before_bind)
+		unlink(path);
 
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;
+	bind_successful = 1;
 
-	if (listen(fd, 5) < 0)
+	if (opts->listen_backlog_size > 0)
+		backlog = opts->listen_backlog_size;
+	else
+		backlog = 5;
+	if (listen(fd, backlog) < 0)
 		goto fail;
 
 	unix_sockaddr_cleanup(&ctx);

@@ -114,7 +125,10 @@ int unix_stream_listen(const char *path)
 fail:
 	saved_errno = errno;
 	unix_sockaddr_cleanup(&ctx);
-	close(fd);
+	if (fd != -1)
+		close(fd);
+	if (bind_successful)
+		unlink(path);
 	errno = saved_errno;
 	return -1;
 }

index e271aeec5a0..c28372ef48e 100644
@@ -1,7 +1,19 @@
 #ifndef UNIX_SOCKET_H
 #define UNIX_SOCKET_H
 
+struct unix_stream_listen_opts {
+	int listen_backlog_size;
+	unsigned int force_unlink_before_bind:1;
+};
+
+#define UNIX_STREAM_LISTEN_OPTS_INIT \
+{ \
+	.listen_backlog_size = 5, \
+	.force_unlink_before_bind = 1, \
+}
+
 int unix_stream_connect(const char *path);
-int unix_stream_listen(const char *path);
+int unix_stream_listen(const char *path,
+		       const struct unix_stream_listen_opts *opts);
 
 #endif /* UNIX_SOCKET_H */

gitgitgadget

From: Jeff Hostetler <[email protected]>

Teach unix_stream_connect() to return error rather than calling die() when a socket cannot be created.

---
 unix-socket.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 9726992f276..c7573df56a6 100644
@@ -76,15 +76,17 @@ 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;
+	int saved_errno;
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
+
 	fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (fd < 0)
-		die_errno("unable to create socket");
+		goto fail;
 
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;

@@ -94,7 +96,8 @@ int unix_stream_connect(const char *path)
 fail:
 	saved_errno = errno;
 	unix_sockaddr_cleanup(&ctx);
-	close(fd);
+	if (fd != -1)
+		close(fd);
 	errno = saved_errno;
 	return -1;
 }

gitgitgadget

From: 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 | 1127 +++++++++++++++++++++++++++
 contrib/buildsystems/CMakeLists.txt |    2 +
 simple-ipc.h                        |    7 +-
 4 files changed, 1137 insertions(+), 1 deletion(-)
 create mode 100644 compat/simple-ipc/ipc-unix-socket.c

index e7ba8853ea6..f2524c02ff0 100644
@@ -1681,6 +1681,8 @@ ifdef NO_UNIX_SOCKETS
 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
+	LIB_OBJS += compat/simple-ipc/ipc-shared.o
+	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
 endif
 
 ifdef USE_WIN32_IPC

index 4bd41054ee7..4c27a373414 100644
@@ -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})

index eb19b5da8b1..17b28bc1f83 100644
@@ -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
 

@@ -160,6 +160,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;
 };
 
 /*

gitgitgadget

From: 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                        | 225 +++++++++
 6 files changed, 1015 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

index 7b64106930a..c94d5847919 100644
@@ -1682,6 +1682,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

new file mode 100644
index 00000000000..1edec815953
@@ -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 */

index 198ab1e58f8..76087cff678 100644
@@ -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

index c151dd7257f..4bd41054ee7 100644
@@ -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

gitgitgadget

From: 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 move the existing error handling to the callers in preparation for adapting specific callers.

---
 unix-socket.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

index 19ed48be990..ef2aeb46bcd 100644
@@ -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);

@@ -79,7 +71,10 @@ int unix_stream_connect(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)
+		die_errno("unable to create socket");
+
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;
 	unix_sockaddr_cleanup(&ctx);

@@ -103,7 +98,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)
+		die_errno("unable to create socket");
 
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;

gitgitgadget

From: Jeff Hostetler <[email protected]>

Create unit tests for "simple-ipc". These are currently only enabled on Windows.

---
 Makefile                   |   1 +
 t/helper/test-simple-ipc.c | 485 +++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |   1 +
 t/helper/test-tool.h       |   1 +
 t/t0052-simple-ipc.sh      | 129 ++++++++++
 5 files changed, 617 insertions(+)
 create mode 100644 t/helper/test-simple-ipc.c
 create mode 100755 t/t0052-simple-ipc.sh

index c94d5847919..e7ba8853ea6 100644
@@ -740,6 +740,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

index 9d6d14d9293..a409655f03b 100644
@@ -64,6 +64,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 },

index a6470ff62c4..564eb3c8e91 100644
@@ -54,6 +54,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);

gitgitgadget

From: Jeff Hostetler <[email protected]>

Calls to chdir() are dangerous in a multi-threaded context. If unix_stream_listen() is given a socket pathname that is too big 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.

Add disallow_chdir flag to struct unix_sockaddr_context and change all callers to pass an initialized context structure.

Teach unix_sockaddr_init() to not allow calls to chdir() when flag is set.

---
 unix-socket.c | 19 ++++++++++++++++---
 unix-socket.h |  2 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

index 8bcef18ea55..9726992f276 100644
@@ -11,8 +11,15 @@ static int chdir_len(const char *orig, int len)
 
 struct unix_sockaddr_context {
 	char *orig_dir;
+	unsigned int disallow_chdir:1;
 };
 
+#define UNIX_SOCKADDR_CONTEXT_INIT \
+{ \
+	.orig_dir=NULL, \
+	.disallow_chdir=0, \
+}
+
 static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
 {
 	if (!ctx->orig_dir)

@@ -32,7 +39,11 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 {
 	int size = strlen(path) + 1;
 
-	ctx->orig_dir = NULL;
+	if (ctx->disallow_chdir && size > sizeof(sa->sun_path)) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;

@@ -67,7 +78,7 @@ int unix_stream_connect(const char *path)
 {
 	int fd, saved_errno;
 	struct sockaddr_un sa;
-	struct unix_sockaddr_context ctx;
+	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;

@@ -96,7 +107,9 @@ int unix_stream_listen(const char *path,
 	int bind_successful = 0;
 	int backlog;
 	struct sockaddr_un sa;
-	struct unix_sockaddr_context ctx;
+	struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
+
+	ctx.disallow_chdir = opts->disallow_chdir;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;

index c28372ef48e..5b0e8ccef10 100644
@@ -4,12 +4,14 @@
 struct unix_stream_listen_opts {
 	int listen_backlog_size;
 	unsigned int force_unlink_before_bind:1;
+	unsigned int disallow_chdir:1;
 };
 
 #define UNIX_STREAM_LISTEN_OPTS_INIT \
 { \
 	.listen_backlog_size = 5, \
 	.force_unlink_before_bind = 1, \
+	.disallow_chdir = 0, \
 }
 
 int unix_stream_connect(const char *path);

gitgitgadget

From: Jeff Hostetler <[email protected]>

Create version of write_packetized_from_buf() that takes a scratch buffer argument rather than assuming a static buffer. This will be used later as we make packet-line writing more thread-safe.

---
 pkt-line.c | 9 ++++++++-
 pkt-line.h | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

index 14af049cd9c..5d86354cbeb 100644
@@ -278,6 +278,13 @@ 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)
 {
 	static struct packet_scratch_space scratch;
+
+	return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
+}
+
+int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+			       struct packet_scratch_space *scratch)
+{
 	int err = 0;
 	size_t bytes_written = 0;
 	size_t bytes_to_write;

@@ -289,7 +296,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
 			bytes_to_write = len - bytes_written;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, &scratch);
+		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
 		bytes_written += bytes_to_write;
 	}
 	if (!err)

index 4ccd6f88926..f1d5625e91f 100644
@@ -41,6 +41,8 @@ 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_buf2(const char *src_in, size_t len, int fd_out,
+			       struct packet_scratch_space *scratch);
 
 /*
  * Read a packetized line into the buffer, which must be at least size bytes

gitgitgadget

From: Jeff Hostetler <[email protected]>

Move the static buffer used in packet_write_gently() to its callers. This is a first step to make packet writing more thread-safe.

---
 pkt-line.c | 33 ++++++++++++++++++++++++---------
 pkt-line.h | 10 ++++++++--
 2 files changed, 32 insertions(+), 11 deletions(-)

index d633005ef74..14af049cd9c 100644
@@ -194,26 +194,34 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	return status;
 }
 
-static int packet_write_gently(const int fd_out, const char *buf, size_t size)
+/*
+ * Use the provided scratch space to build a combined <hdr><buf> buffer
+ * and write it to the file descriptor (in one write if possible).
+ */
+static int packet_write_gently(const int fd_out, const char *buf, size_t size,
+			       struct packet_scratch_space *scratch)
 {
-	static char packet_write_buffer[LARGE_PACKET_MAX];
 	size_t packet_size;
 
-	if (size > sizeof(packet_write_buffer) - 4)
+	if (size > sizeof(scratch->buffer) - 4)
 		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(scratch->buffer, packet_size);
+	memcpy(scratch->buffer + 4, buf, size);
+
+	if (write_in_full(fd_out, scratch->buffer, packet_size) < 0)
 		return error(_("packet write failed"));
 	return 0;
 }
 
 void packet_write(int fd_out, const char *buf, size_t size)
 {
-	if (packet_write_gently(fd_out, buf, size))
+	static struct packet_scratch_space scratch;
+
+	if (packet_write_gently(fd_out, buf, size, &scratch))
 		die_errno(_("packet write failed"));
 }
 

@@ -244,6 +252,12 @@ 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)
 {
+	/*
+	 * TODO We could save a memcpy() if we essentially inline
+	 * TODO packet_write_gently() here and change the xread()
+	 * TODO to pass &buf[4].
+	 */
+	static struct packet_scratch_space scratch;
 	static char buf[LARGE_PACKET_DATA_MAX];
 	int err = 0;
 	ssize_t bytes_to_write;

@@ -254,7 +268,7 @@ int write_packetized_from_fd(int fd_in, int fd_out)
 			return COPY_READ_ERROR;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, buf, bytes_to_write);
+		err = packet_write_gently(fd_out, buf, bytes_to_write, &scratch);
 	}
 	if (!err)
 		err = packet_flush_gently(fd_out);

@@ -263,6 +277,7 @@ 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)
 {
+	static struct packet_scratch_space scratch;
 	int err = 0;
 	size_t bytes_written = 0;
 	size_t bytes_to_write;

@@ -274,7 +289,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
 			bytes_to_write = len - bytes_written;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, &scratch);
 		bytes_written += bytes_to_write;
 	}
 	if (!err)

index 8c90daa59ef..4ccd6f88926 100644
@@ -5,6 +5,13 @@
 #include "strbuf.h"
 #include "sideband.h"
 
+#define LARGE_PACKET_MAX 65520
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
+struct packet_scratch_space {
+	char buffer[LARGE_PACKET_MAX];
+};
+
 /*
  * Write a packetized stream, where each line is preceded by
  * its length (including the header) as a 4-byte hex number.

@@ -213,8 +220,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader);
 enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 
 #define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
 extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {

gitgitgadget

Huh?

Sorry. I had to prepend that one to the patch series to get the CI builds to run. I've been working rebased against "v2.30.0" and GitGitGadget references "master".

Jeff

OK, this is extending the changes from the first patch...

I think this is a sign that the function is not very well-designed in the first place. It seems like the code would be easier to understand overall if that caller just explicitly did the flush itself. It even already does so in other cases!

Something like (untested):

 convert.c  | 4 ++++
 pkt-line.c | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

index ee360c2f07..3968ac37b9 100644
@@ -890,6 +890,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
+	err = packet_flush_gently(process->in);
+	if (err)
+		goto done;
+
 	err = subprocess_read_status(process->out, &filter_status);
 	if (err)
 		goto done;

index d633005ef7..014520a9c2 100644
@@ -256,8 +256,6 @@ 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);
 	return err;
 }
 

@@ -277,8 +275,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;
 }
 

-Peff

Thanks, this looks good.

  return socket(AF_UNIX, SOCK_STREAM, 0);

I sent a follow-up on the previous patch, but I think this part about the die() should be folded in there.

Likewise I think it would probably be easier to follow if we added the backlog parameter and the unlink options in separate patches. The backlog thing is small, but the unlink part is subtle and requires explanation. That's a good sign it might do better in its own commit.

OK. I'm still not sure of the endgame here for writing non-racy code to establish the socket (which is going to require either some atomic renaming or some dot-locking in the caller). But it's plausible to me that this option will be a useful primitive.

The implementation looks correct, though here are a few small observations/questions/nits:

Makes sense, and it fits nicely into the options pattern you set up in the earlier patch.

index 19ed48be99..0eb14faf54 100644
@@ -36,16 +36,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;

  struct unix_stream_listen_opts_init {
	int listen_backlog_size;
	int disallow_unlink;
	int disallow_chdir;

Yeah, I had all of those thoughts and debates in my head. I'm not sure there is a clear winner here. And I was trying to prevent this change from having a massive footprint and all that. The FSMonitor stuff is enough to worry about...

Personally, I like the 2 syscall model (for now at least and not mess with writev()). There are only 3 calls to packet_write_gently() and this fixes 2 of them without any local buffers. I might as well update the 1 caller of write_packetized_from_fd() to pass a buffer rather than have a static buffer while we're at it. Then all of those routines are fixed.

Let me see what that looks like.

Thanks
Jeff

Hi Peff,

There must have been a reason why the original code went out of its way to copy the data. At least that's what I _assume_.

I could see, for example, that these extra round-trips just for the header, really have a negative impact on network operations.

No, because there is no equivalent for that on Windows. And since Windows is the primary target of our Simple IPC/FSMonitor work, that would break the bank.

Or we change it to

	struct packet_buffer {
		char count[4];
		char payload[LARGE_PACKET_MAX - 4];
	};

and then ask the callers to allocate one of those beauties Not sure how well we can guarantee that the compiler won't pad this, though.

And then there is write_packetized_from_buf() whose src parameter can come from convert_to_git() that _definitely_ would not be of the desired form.

So I guess if we can get away with the 2-syscall version, that's kind of better than that.

Ciao,
Dscho

Hi Peff,

Fine by me.

Thanks,
Dscho

Hi Junio & Jeff,

The idea being that we want to be able to merge this branch as-is into Git for Windows (and also into microsoft/git), and therefore do not want to base it on a later commit that is not reachable from git-for-windows/git's main branch.

Maybe it is time to merge jc/macos-install-dependencies-fix down to maint? Then we could base Simple IPC/FSMonitor on maint instead, and would still have the benefit we want.

Ciao,
Dscho

Yeah. The robust thing is to have the server indicate when it's ready to receive requests. There's some prior art in t/lib-git-daemon.sh using a fifo to get a line to the caller. It's ugly, but AFAIK pretty bulletproof.

-Peff

Having looked at the history, including the original mailing list threads, it doesn't seem to be.

Keep in mind these won't be network round-trips. They're just syscall round-trips. The OS would keep writing without an ACK while filling a TCP window. The worst case may be an extra packet on the wire, though the OS may end up coalescing the writes into a single packet anyway.

Are you concerned about the performance implications, or just portability? Falling back to two writes (and wrapping that in a function) would be easy for the latter. For the former, there's WSASend, but I have no idea what kind of difficulties/caveats we might run into.

Yeah, I almost suggested the same, but wasn't sure about padding. I think the standard allows there to be arbitrary padding between the two, so it's really up to the ABI to define. I'd be surprised if this struct is a problem in practice (we already have some structs which assume 4-byte alignment, and nobody seems to have complained).

Yep. It really does need to either use two writes or to copy, because it's slicing up a much larger buffer (it wouldn't be the end of the world for it to allocate a single LARGE_PACKET_MAX heap buffer for the duration of its run, though).

I do prefer it, because then the whole thing just becomes an implementation detail that callers don't need to care about.

-Peff

Now you mention it, if we ever have an update to the current 'maint' branch, we'd trigger this known failure in macOS build without a good reason, even though we have a fix that has been in use on the 'master' and above.

I definitely qshould merge the jc/macos-install-dependencies-fix topic down to 'maint' now.

Are there other topics that deserve to be in 'maint' that are "obviously correct" people can think of?

Thanks.

Hi Junio,

I looked over the branches merged into master that are not in maint, and from a cursory look, these seem to be good candidates:

- pk/subsub-fetch-fix-take-2
- rs/rebase-commit-validation
- en/stash-apply-sparse-checkout
- ds/for-each-repo-noopfix
- pb/mergetool-tool-help-fix
- jk/t5516-deflake
- jc/macos-install-dependencies-fix
- jk/forbid-lf-in-git-url
- jk/log-cherry-pick-duplicate-patches
- js/skip-dashed-built-ins-from-config-mak

From a cursory look, it might seem as if ds/maintenance-prefetch-cleanup would also be a good candidate, but it is not based on maint, although it _does_ appear to fix an issue introduced in v2.30.0-rc0~23^2~8.

There is another candidate that I am not _quite_ sure about:
dl/p4-encode-after-kw-expansion. It _seems_ as if it would be good to apply on the maintenance train, but I am uncertain how important a bug fix it is.

There are also a couple test updates that might be nice to have in maint:

- nk/perf-fsmonitor-cleanup
- mt/t4129-with-setgid-dir
- ad/t4129-setfacl-target-fix

Finally, there are documentation updates that I would probably merge, if I was tasked with updating maint:

- ta/doc-typofix
- pb/doc-modules-git-work-tree-typofix
- jc/sign-off
- vv/send-email-with-less-secure-apps-access
- ug/doc-lose-dircache
- ab/gettext-charset-comment-fix
- bc/doc-status-short
- tb/local-clone-race-doc
- ab/fsck-doc-fix
- jt/packfile-as-uri-doc

Ciao,
Dscho

I agree. I'll move flush to the caller and rename the write packetized function slightly to guard against new callers assuming the old behavior during the transition.

Jeff

Your list more or less matches what the ML (merge later) script on the todo/ branch produces when it is fed the RelNotes (the script just greps for "merge laster to maint" comments and shows the result in way a bit easier to use for me).

The ones that are in RelNotes but not in your list are

ab/branch-sort # 7 (11 days ago)
ar/t6016-modernise # 1 (3 weeks ago) dl/p4-encode-after-kw-expansion # 1 (3 weeks ago) fc/t6030-bisect-reset-removes-auxiliary-files # 1 (4 weeks ago) ma/doc-pack-format-varint-for-sizes # 1 (3 weeks ago) ma/more-opaque-lock-file # 5 (11 days ago) ma/t1300-cleanup # 3 (3 weeks ago) zh/arg-help-format # 2 (3 weeks ago)

and I think all of them are safe to merge down.

Thanks for being an independent source I can rely on to sanity check what is in RelNotes. Very much appreciated.

Yes, that helped having them in 2 patches each with 1 concern.

In part 14/14 in ipc-unix-sockets.c:create_listener_socket() I have code in the calling layer to (try to) handle both the startup races and basic collisions with existing long-running servers already using the socket.

But you're right, it might be good to revisit that as a primitive at this layer. We only have 1 other caller right now and I don't know enough about credential-cache--daemon to know if it would benefit from this or not.

Agreed.

I'll refactor this a bit.

I'm not sure it matters, but it does look better to unwind things in reverse order. And yes, unlinking first is a little bit safer.

Good point. Thanks!

There you make a temp socket and then try to rename it into place. But because rename() overwrites the destination, it still seems like two creating processes can race each other. Something like:

0. There's no "foo" socket (or maybe there is a stale one that nobody's listening on).

1. Process A wants to become the listener. So it creates foo.A.

2. Process B likewise. It creates foo.B.

3. Process A renames foo.A to foo. It believes it will now service clients.

4. Process B renames foo.B to foo. Now process A is stranded but doesn't realize it.

I.e., I don't think this is much different than an unlink+create strategy. You've eliminated the window where a process C shows up during steps 3 and 4 and sees no socket (because somebody else is in the midst of a non-atomic unlink+create operation). But there's no atomicity between the "ping the socket" and "create the socket" steps.

Yeah, having seen patch 14, it looks like your only new caller always sets the new unlink option to 1. So it might not be worth making it optional if you don't need it (especially because the rename trick, assuming it's portable, is superior to unlink+create; and you'd always be fine with an unlink on the temp socket).

The call in credential-cache--daemon is definitely racy. It's pretty much the same thing: it pings the socket to see if it's alive, but is still susceptible to the problem above. I was was never too concerned about it, since the whole point of the daemon is to hang around until its contents expire. If it loses the race and nobody contacts it, the worst case is it waits 30 seconds for somebody to give it data before exiting. It would benefit slightly from switching to the rename strategy, but the bigger race would remain.

-Peff

Yeah, in my version two processes could still create uniquely named sockets and then do the rename trick. But they capture the inode number of the socket before they do that. They periodically lstat the socket to see if the inode number has changed and if so, assume it has been stolen from them. (A bit of a hack, I admit.)

And I was assuming that 2 servers starting at about the same time are effectively equivalent -- it doesn't matter which one dies, since they both should have the same amount of cached state. Unlike the case where a long-running server (with lots of state) is replaced by a newcomer.

I am wondering if we can use the .LOCK file magic to our advantage here (in sort of an off-label use). If we have the server create a lockfile "<path>.LOCK" and if successful leave it open/locked for the life of the server (rather than immediately renaming it onto <path>) and let the normal shutdown code rollback/delete the lockfile in the cleanup/atexit.

If the server successfully creates the lockfile, then unlink+create the socket at <path>.

That would give us the unique/exclusive creation (on the lock) that we need. Then wrap that with all the edge case cleanup code to create/delete/manage the peer socket. Basically if the lock exists, there should be a live server listening to the socket (unless there was a crash...).

And yes, then I don't think I need the preserve_existing bit in the opts struct.

OK, that makes more sense. I saw the mention of the inode stuff in a comment, but I didn't see it in the code (I guess if it's a periodic check it's not in that initial socket creation function).

Yeah, I agree with that notion in general. I do think it would be easier to reason about if the creation were truly race-proof (probably with a dot-lock; see below), rather than the later "check if we got replaced" thing. OTOH, that "check" strategy covers a variety of cases (including that somebody tried to ping us, decided we weren't alive due to a timeout or some other system reason, and then replaced our socket).

Another strategy there could be having the daemon just decide to quit if nobody contacts it for N time units. It is, after all, a cache. Even if nobody replaces the socket, it probably makes sense to eventually decide that the memory we're holding isn't going to good use.

I don't even think this is off-label. Though the normal use is for the .lock file to get renamed into place as the official file, there are a few other places where we use it solely for mutual exclusion. You just always end with rollback_lock_file(), and never "commit" it.

So something like:

1. Optimistically see if socket "foo" is present and accepting connections.

2. If not, then take "foo.lock". If somebody else is holding it, loop with a timeout waiting for them to come alive.

3. Assuming we got the lock, then either unlink+create the socket as "foo", or rename-into-place. I don't think it matters that much which.

4. Rollback "foo.lock", unlinking it.

Then one process wins the lock and creates the socket, while any simultaneous creators spin in step 2, and eventually connect to the winner.

I think you'd want to delete the lock as soon as you're done with the setup. That reduces the chances that a dead server (e.g., killed by a power outage without the chance to clean up after itself) leaves a stale lock sitting around.

-Peff

Yeah, there's a very slow poll(2) loop in the listen/accept thread that watches for that and new connections (and quit messages).

I have the poll(2) loop set to recheck the inode for theft every 60 seconds (randomly chosen).

Assuming the socket isn't stolen, I want to leave any thoughts of an auto-shutdown to the application layer above it. My next patch series will use this ipc mechanism to build a FSMonitor daemon that will watch the filesystem for changes and then be able to quickly respond to a git status, so it is important that it be allowed to run without any clients for a while (such a during a build). Yes, memory concerns are important, so I do want it to auto-shutdown if the socket is stolen (or the workdir is deleted).

Thanks this helps. I've got a version now that is a slight variation on what you have here that seems to work nicely and has the short-lived lock file. I'll post this shortly.

Jeff

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 the API of write_packetized_from_fd() to accept a scratch space argument from its caller 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.

---
 convert.c  |  7 ++++---
 pkt-line.c | 28 +++++++++++++++++++---------
 pkt-line.h | 12 +++++++++---
 3 files changed, 32 insertions(+), 15 deletions(-)

index ee360c2f07ce..41012c2d301c 100644
@@ -883,9 +883,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	if (fd >= 0)
-		err = write_packetized_from_fd(fd, process->in);
-	else
+	if (fd >= 0) {
+		struct packet_scratch_space scratch;
+		err = write_packetized_from_fd(fd, process->in, &scratch);
+	} else
 		err = write_packetized_from_buf(src, len, process->in);
 	if (err)
 		goto done;

index d633005ef746..4cff2f7a68a5 100644
@@ -196,17 +196,25 @@ 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 avoids perf
+	 * and 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;
 }

@@ -242,19 +250,21 @@ 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(int fd_in, int fd_out,
+			     struct packet_scratch_space *scratch)
 {
-	static char buf[LARGE_PACKET_DATA_MAX];
 	int err = 0;
 	ssize_t bytes_to_write;
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, buf, sizeof(buf));
+		bytes_to_write = xread(fd_in, scratch->buffer,
+				       sizeof(scratch->buffer));
 		if (bytes_to_write < 0)
 			return COPY_READ_ERROR;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, buf, bytes_to_write);
+		err = packet_write_gently(fd_out, scratch->buffer,
+					  bytes_to_write);
 	}
 	if (!err)
 		err = packet_flush_gently(fd_out);

index 8c90daa59ef0..c0722aefe638 100644
@@ -5,6 +5,13 @@
 #include "strbuf.h"
 #include "sideband.h"
 
+#define LARGE_PACKET_MAX 65520
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
+struct packet_scratch_space {
+	char buffer[LARGE_PACKET_DATA_MAX]; /* does not include header bytes */
+};
+
 /*
  * Write a packetized stream, where each line is preceded by
  * its length (including the header) as a 4-byte hex number.

@@ -32,7 +39,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
 void 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_fd(int fd_in, int fd_out, struct packet_scratch_space *scratch);
 int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 
 /*

@@ -213,8 +220,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader);
 enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 
 #define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
 extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {

gitgitgadget

From: 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 | 10 +++-------
 pkt-line.h |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

index 41012c2d301c..bccf7afa8797 100644
@@ -885,9 +885,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 
 	if (fd >= 0) {
 		struct packet_scratch_space scratch;
-		err = write_packetized_from_fd(fd, process->in, &scratch);
+		err = write_packetized_from_fd_no_flush(fd, process->in, &scratch);
 	} 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;
 

index 4cff2f7a68a5..3602b0d37092 100644
@@ -250,8 +250,8 @@ 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,
-			     struct packet_scratch_space *scratch)
+int write_packetized_from_fd_no_flush(int fd_in, int fd_out,
+				      struct packet_scratch_space *scratch)
 {
 	int err = 0;
 	ssize_t bytes_to_write;