[PATCH 0/8] Simple IPC Cleanups

[PATCH 0/8] Simple IPC Cleanups

[Jeff Hostetler via GitGitGadget]

This patch series adds a few final cleanup and refactoring commits onto the
jh/simple-ipc branch. These are in response to mailing list comments that I
received on my V4 version after it was queued into next.

https://lore.kernel.org/git/pull.766.v4.git.1613598529.gitgitgadget@gmail.com/T/#mbd1da5ff93ef273049090f697aeab68c74f698f1

There are a couple of large changes that I'll call out here.

In the third commit, I moved the new lock-aware code out of unix-socket.c
and into its own source file. This creates a slightly misleading diff at
times (in gitk) where it looks like 51% copy of unix-socket.c rather than a
new file. On the command line and on GitHub it looks better.

In the commits prefixed with test-simple-ipc: ... I refactored the options
parsing to allow the name of the socket/named-pipe to be passed on the
command line so that the Windows version could do so (since it needs to exec
a child rather than fork). This turned into a larger cleanup/refactoring
than I had expected, but I think the result is much better. I unified all of
the option parsing into the main cmd__simple_ipc() function and got rid of
the smaller parsers inside of each subcommand. With this, all of the
subcommands now allow an alternate socket path to be used. (Just fixing the
unused arg on the Windows side would allow us to spawn a background daemon
on a different socket, but none of the client subcommands would be able to
talk to it.)

Jeff Hostetler (8):
  pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()
  unix-socket: simplify initialization of unix_stream_listen_opts
  unix-stream-server: create unix-stream-server.c
  simple-ipc: move error handling up a level
  unix-stream-server: add st_dev and st_mode to socket stolen checks
  test-simple-ipc: refactor command line option processing in helper
  test-simple-ipc: add --token=<token> string option
  simple-ipc: update design documentation with more details

 Documentation/technical/api-simple-ipc.txt | 131 +++++++--
 Makefile                                   |   1 +
 compat/simple-ipc/ipc-unix-socket.c        |  49 ++--
 compat/simple-ipc/ipc-win32.c              |  14 +-
 contrib/buildsystems/CMakeLists.txt        |   2 +-
 convert.c                                  |   7 +-
 pkt-line.c                                 |  19 +-
 pkt-line.h                                 |   6 +-
 simple-ipc.h                               |   4 +
 t/helper/test-simple-ipc.c                 | 326 +++++++++++----------
 t/t0052-simple-ipc.sh                      |  10 +-
 unix-socket.c                              | 117 +-------
 unix-socket.h                              |  33 +--
 unix-stream-server.c                       | 130 ++++++++
 unix-stream-server.h                       |  35 +++
 15 files changed, 502 insertions(+), 382 deletions(-)
 create mode 100644 unix-stream-server.c
 create mode 100644 unix-stream-server.h


base-commit: edce16a37ab87513a3f0bc805e9bf372bdd02961
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-893%2Fjeffhostetler%2Fnext-simple-ipc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-893/jeffhostetler/next-simple-ipc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/893
-- 
gitgitgadget

[PATCH 1/8] pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`
and allocate a temporary buffer within the function.

In 3e4447f1ea (pkt-line: eliminate the need for static buffer in
packet_write_gently(), 2021-02-13) we added the argument in order to
get rid of a static buffer to make the routine safe in multi-threaded
contexts and to avoid putting a very large buffer on the stack.  This
delegated the problem to the caller who has more knowledge of the use
case and can best decide the most efficient way to provide such a
buffer.

However, in practice, the (currently, only) caller just created the
buffer on the stack, so we might as well just allocate it within the
function and restore the original API.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 convert.c  |  7 +++----
 pkt-line.c | 19 +++++++++++--------
 pkt-line.h |  6 +-----
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/convert.c b/convert.c
index 9f44f00d841f..516f1095b06e 100644
--- a/convert.c
+++ b/convert.c
@@ -883,10 +883,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	if (fd >= 0) {
-		struct packet_scratch_space scratch;
-		err = write_packetized_from_fd_no_flush(fd, process->in, &scratch);
-	} else
+	if (fd >= 0)
+		err = write_packetized_from_fd_no_flush(fd, process->in);
+	else
 		err = write_packetized_from_buf_no_flush(src, len, process->in);
 	if (err)
 		goto done;
diff --git a/pkt-line.c b/pkt-line.c
index 18ecad65e08c..c933bb95586d 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -250,22 +250,25 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
 	packet_trace(data, len, 1);
 }
 
-int write_packetized_from_fd_no_flush(int fd_in, int fd_out,
-				      struct packet_scratch_space *scratch)
+int write_packetized_from_fd_no_flush(int fd_in, int fd_out)
 {
 	int err = 0;
 	ssize_t bytes_to_write;
+	char *buf = xmalloc(LARGE_PACKET_DATA_MAX);
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, scratch->buffer,
-				       sizeof(scratch->buffer));
-		if (bytes_to_write < 0)
-			return COPY_READ_ERROR;
+		bytes_to_write = xread(fd_in, buf, LARGE_PACKET_DATA_MAX);
+		if (bytes_to_write < 0) {
+			err = COPY_READ_ERROR;
+			break;
+		}
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, scratch->buffer,
-					  bytes_to_write);
+		err = packet_write_gently(fd_out, buf, bytes_to_write);
 	}
+
+	free(buf);
+
 	return err;
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index e347fe46832a..54eec72dc0af 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -8,10 +8,6 @@
 #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.
@@ -39,7 +35,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_no_flush(int fd_in, int fd_out, struct packet_scratch_space *scratch);
+int write_packetized_from_fd_no_flush(int fd_in, int fd_out);
 int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out);
 
 /*
-- 
gitgitgadget

[PATCH 2/8] unix-socket: simplify initialization of unix_stream_listen_opts

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Change the public initialization of `struct unix_stream_listen_opts`
to be all zeroes.  Hide the default values for the timeout and backlog
values inside `unix-socket.c`.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 unix-socket.c | 11 +++++++++--
 unix-socket.h |  7 ++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/unix-socket.c b/unix-socket.c
index 647bbde37f97..c9ea1de43bd2 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -2,6 +2,9 @@
 #include "lockfile.h"
 #include "unix-socket.h"
 
+#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
+#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
+
 static int chdir_len(const char *orig, int len)
 {
 	char *path = xmemdupz(orig, len);
@@ -165,14 +168,18 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	const struct unix_stream_listen_opts *opts)
 {
 	struct lock_file lock = LOCK_INIT;
+	long timeout;
 	int fd_socket;
 	struct unix_stream_server_socket *server_socket;
 
+	timeout = opts->timeout_ms;
+	if (opts->timeout_ms <= 0)
+		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
+
 	/*
 	 * Create a lock at "<path>.lock" if we can.
 	 */
-	if (hold_lock_file_for_update_timeout(&lock, path, 0,
-					      opts->timeout_ms) < 0) {
+	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {
 		error_errno(_("could not lock listener socket '%s'"), path);
 		return NULL;
 	}
diff --git a/unix-socket.h b/unix-socket.h
index 8faf5b692f90..bec925ee0213 100644
--- a/unix-socket.h
+++ b/unix-socket.h
@@ -7,13 +7,10 @@ struct unix_stream_listen_opts {
 	unsigned int disallow_chdir:1;
 };
 
-#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
-#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
-
 #define UNIX_STREAM_LISTEN_OPTS_INIT \
 { \
-	.timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \
-	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
+	.timeout_ms = 0, \
+	.listen_backlog_size = 0, \
 	.disallow_chdir = 0, \
 }
 
-- 
gitgitgadget

[PATCH 3/8] unix-stream-server: create unix-stream-server.c

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Move unix_stream_server__listen_with_lock() and associated routines
and data types out of unix-socket.[ch] and into their own source file.

We added code to safely create a Unix domain socket using a lockfile in:
9406d12e14 (unix-socket: create `unix_stream_server__listen_with_lock()`,
2021-02-13).  This code conceptually exists at a higher-level than the
core unix_stream_connect() and unix_stream_listen() routines that it
consumes and should be isolated for clarity.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                            |   1 +
 compat/simple-ipc/ipc-unix-socket.c |   1 +
 contrib/buildsystems/CMakeLists.txt |   2 +-
 unix-socket.c                       | 120 ---------------------------
 unix-socket.h                       |  26 ------
 unix-stream-server.c                | 124 ++++++++++++++++++++++++++++
 unix-stream-server.h                |  32 +++++++
 7 files changed, 159 insertions(+), 147 deletions(-)
 create mode 100644 unix-stream-server.c
 create mode 100644 unix-stream-server.h

diff --git a/Makefile b/Makefile
index 93f2e7ca9e1f..dfc37b0480e4 100644
--- a/Makefile
+++ b/Makefile
@@ -1678,6 +1678,7 @@ ifdef NO_UNIX_SOCKETS
 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
+	LIB_OBJS += unix-stream-server.o
 	LIB_OBJS += compat/simple-ipc/ipc-shared.o
 	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
 endif
diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index b7fd0b34329e..b0264ca6cdc6 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -4,6 +4,7 @@
 #include "pkt-line.h"
 #include "thread-utils.h"
 #include "unix-socket.h"
+#include "unix-stream-server.h"
 
 #ifdef NO_UNIX_SOCKETS
 #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 4c27a373414a..629a1817459b 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -243,7 +243,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
 	add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY )
-	list(APPEND compat_SOURCES unix-socket.c)
+	list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c)
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
diff --git a/unix-socket.c b/unix-socket.c
index c9ea1de43bd2..e0be1badb58d 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -1,8 +1,6 @@
 #include "cache.h"
-#include "lockfile.h"
 #include "unix-socket.h"
 
-#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
 #define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
 
 static int chdir_len(const char *orig, int len)
@@ -136,121 +134,3 @@ int unix_stream_listen(const char *path,
 	errno = saved_errno;
 	return -1;
 }
-
-static int is_another_server_alive(const char *path,
-				   const struct unix_stream_listen_opts *opts)
-{
-	struct stat st;
-	int fd;
-
-	if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
-		/*
-		 * A socket-inode exists on disk at `path`, but we
-		 * don't know whether it belongs to an active server
-		 * or whether the last server died without cleaning
-		 * up.
-		 *
-		 * Poke it with a trivial connection to try to find
-		 * out.
-		 */
-		fd = unix_stream_connect(path, opts->disallow_chdir);
-		if (fd >= 0) {
-			close(fd);
-			return 1;
-		}
-	}
-
-	return 0;
-}
-
-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
-	const char *path,
-	const struct unix_stream_listen_opts *opts)
-{
-	struct lock_file lock = LOCK_INIT;
-	long timeout;
-	int fd_socket;
-	struct unix_stream_server_socket *server_socket;
-
-	timeout = opts->timeout_ms;
-	if (opts->timeout_ms <= 0)
-		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
-
-	/*
-	 * Create a lock at "<path>.lock" if we can.
-	 */
-	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {
-		error_errno(_("could not lock listener socket '%s'"), path);
-		return NULL;
-	}
-
-	/*
-	 * If another server is listening on "<path>" give up.  We do not
-	 * want to create a socket and steal future connections from them.
-	 */
-	if (is_another_server_alive(path, opts)) {
-		errno = EADDRINUSE;
-		error_errno(_("listener socket already in use '%s'"), path);
-		rollback_lock_file(&lock);
-		return NULL;
-	}
-
-	/*
-	 * Create and bind to a Unix domain socket at "<path>".
-	 */
-	fd_socket = unix_stream_listen(path, opts);
-	if (fd_socket < 0) {
-		error_errno(_("could not create listener socket '%s'"), path);
-		rollback_lock_file(&lock);
-		return NULL;
-	}
-
-	server_socket = xcalloc(1, sizeof(*server_socket));
-	server_socket->path_socket = strdup(path);
-	server_socket->fd_socket = fd_socket;
-	lstat(path, &server_socket->st_socket);
-
-	/*
-	 * Always rollback (just delete) "<path>.lock" because we already created
-	 * "<path>" as a socket and do not want to commit_lock to do the atomic
-	 * rename trick.
-	 */
-	rollback_lock_file(&lock);
-
-	return server_socket;
-}
-
-void unix_stream_server__free(
-	struct unix_stream_server_socket *server_socket)
-{
-	if (!server_socket)
-		return;
-
-	if (server_socket->fd_socket >= 0) {
-		if (!unix_stream_server__was_stolen(server_socket))
-			unlink(server_socket->path_socket);
-		close(server_socket->fd_socket);
-	}
-
-	free(server_socket->path_socket);
-	free(server_socket);
-}
-
-int unix_stream_server__was_stolen(
-	struct unix_stream_server_socket *server_socket)
-{
-	struct stat st_now;
-
-	if (!server_socket)
-		return 0;
-
-	if (lstat(server_socket->path_socket, &st_now) == -1)
-		return 1;
-
-	if (st_now.st_ino != server_socket->st_socket.st_ino)
-		return 1;
-
-	/* We might also consider the ctime on some platforms. */
-
-	return 0;
-}
diff --git a/unix-socket.h b/unix-socket.h
index bec925ee0213..b079e79cb3e1 100644
--- a/unix-socket.h
+++ b/unix-socket.h
@@ -18,30 +18,4 @@ int unix_stream_connect(const char *path, int disallow_chdir);
 int unix_stream_listen(const char *path,
 		       const struct unix_stream_listen_opts *opts);
 
-struct unix_stream_server_socket {
-	char *path_socket;
-	struct stat st_socket;
-	int fd_socket;
-};
-
-/*
- * Create a Unix Domain Socket at the given path under the protection
- * of a '.lock' lockfile.
- */
-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
-	const char *path,
-	const struct unix_stream_listen_opts *opts);
-
-/*
- * Close and delete the socket.
- */
-void unix_stream_server__free(
-	struct unix_stream_server_socket *server_socket);
-
-/*
- * Return 1 if the inode of the pathname to our socket changes.
- */
-int unix_stream_server__was_stolen(
-	struct unix_stream_server_socket *server_socket);
-
 #endif /* UNIX_SOCKET_H */
diff --git a/unix-stream-server.c b/unix-stream-server.c
new file mode 100644
index 000000000000..ad175ba731ca
--- /dev/null
+++ b/unix-stream-server.c
@@ -0,0 +1,124 @@
+#include "cache.h"
+#include "lockfile.h"
+#include "unix-socket.h"
+#include "unix-stream-server.h"
+
+#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
+
+static int is_another_server_alive(const char *path,
+				   const struct unix_stream_listen_opts *opts)
+{
+	struct stat st;
+	int fd;
+
+	if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
+		/*
+		 * A socket-inode exists on disk at `path`, but we
+		 * don't know whether it belongs to an active server
+		 * or whether the last server died without cleaning
+		 * up.
+		 *
+		 * Poke it with a trivial connection to try to find
+		 * out.
+		 */
+		fd = unix_stream_connect(path, opts->disallow_chdir);
+		if (fd >= 0) {
+			close(fd);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
+	const char *path,
+	const struct unix_stream_listen_opts *opts)
+{
+	struct lock_file lock = LOCK_INIT;
+	long timeout;
+	int fd_socket;
+	struct unix_stream_server_socket *server_socket;
+
+	timeout = opts->timeout_ms;
+	if (opts->timeout_ms <= 0)
+		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
+
+	/*
+	 * Create a lock at "<path>.lock" if we can.
+	 */
+	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {
+		error_errno(_("could not lock listener socket '%s'"), path);
+		return NULL;
+	}
+
+	/*
+	 * If another server is listening on "<path>" give up.  We do not
+	 * want to create a socket and steal future connections from them.
+	 */
+	if (is_another_server_alive(path, opts)) {
+		errno = EADDRINUSE;
+		error_errno(_("listener socket already in use '%s'"), path);
+		rollback_lock_file(&lock);
+		return NULL;
+	}
+
+	/*
+	 * Create and bind to a Unix domain socket at "<path>".
+	 */
+	fd_socket = unix_stream_listen(path, opts);
+	if (fd_socket < 0) {
+		error_errno(_("could not create listener socket '%s'"), path);
+		rollback_lock_file(&lock);
+		return NULL;
+	}
+
+	server_socket = xcalloc(1, sizeof(*server_socket));
+	server_socket->path_socket = strdup(path);
+	server_socket->fd_socket = fd_socket;
+	lstat(path, &server_socket->st_socket);
+
+	/*
+	 * Always rollback (just delete) "<path>.lock" because we already created
+	 * "<path>" as a socket and do not want to commit_lock to do the atomic
+	 * rename trick.
+	 */
+	rollback_lock_file(&lock);
+
+	return server_socket;
+}
+
+void unix_stream_server__free(
+	struct unix_stream_server_socket *server_socket)
+{
+	if (!server_socket)
+		return;
+
+	if (server_socket->fd_socket >= 0) {
+		if (!unix_stream_server__was_stolen(server_socket))
+			unlink(server_socket->path_socket);
+		close(server_socket->fd_socket);
+	}
+
+	free(server_socket->path_socket);
+	free(server_socket);
+}
+
+int unix_stream_server__was_stolen(
+	struct unix_stream_server_socket *server_socket)
+{
+	struct stat st_now;
+
+	if (!server_socket)
+		return 0;
+
+	if (lstat(server_socket->path_socket, &st_now) == -1)
+		return 1;
+
+	if (st_now.st_ino != server_socket->st_socket.st_ino)
+		return 1;
+
+	/* We might also consider the ctime on some platforms. */
+
+	return 0;
+}
diff --git a/unix-stream-server.h b/unix-stream-server.h
new file mode 100644
index 000000000000..745849e31c30
--- /dev/null
+++ b/unix-stream-server.h
@@ -0,0 +1,32 @@
+#ifndef UNIX_STREAM_SERVER_H
+#define UNIX_STREAM_SERVER_H
+
+#include "unix-socket.h"
+
+struct unix_stream_server_socket {
+	char *path_socket;
+	struct stat st_socket;
+	int fd_socket;
+};
+
+/*
+ * Create a Unix Domain Socket at the given path under the protection
+ * of a '.lock' lockfile.
+ */
+struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
+	const char *path,
+	const struct unix_stream_listen_opts *opts);
+
+/*
+ * Close and delete the socket.
+ */
+void unix_stream_server__free(
+	struct unix_stream_server_socket *server_socket);
+
+/*
+ * Return 1 if the inode of the pathname to our socket changes.
+ */
+int unix_stream_server__was_stolen(
+	struct unix_stream_server_socket *server_socket);
+
+#endif /* UNIX_STREAM_SERVER_H */
-- 
gitgitgadget

[PATCH 4/8] simple-ipc: move error handling up a level

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Move error reporting for socket/named pipe creation out of platform
specific and simple-ipc layer code and rely on returned error value
and `errno`.

Update t/helper/test-simple-ipc.c to print errors.

Simplify testing for another server in `is_another_server_is_alive()`.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-unix-socket.c | 48 ++++++++++++----------
 compat/simple-ipc/ipc-win32.c       | 14 ++++---
 simple-ipc.h                        |  4 ++
 t/helper/test-simple-ipc.c          | 10 ++++-
 unix-stream-server.c                | 63 +++++++++++++++--------------
 unix-stream-server.h                |  7 +++-
 6 files changed, 86 insertions(+), 60 deletions(-)

diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index b0264ca6cdc6..55cbb346328a 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -729,44 +729,51 @@ static void *accept_thread_proc(void *_accept_thread_data)
  */
 #define LISTEN_BACKLOG (50)
 
-static struct unix_stream_server_socket *create_listener_socket(
+static int create_listener_socket(
 	const char *path,
-	const struct ipc_server_opts *ipc_opts)
+	const struct ipc_server_opts *ipc_opts,
+	struct unix_stream_server_socket **new_server_socket)
 {
 	struct unix_stream_server_socket *server_socket = NULL;
 	struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+	int ret;
 
 	uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
 	uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
 
-	server_socket = unix_stream_server__listen_with_lock(path, &uslg_opts);
-	if (!server_socket)
-		return NULL;
+	ret = unix_stream_server__create(path, &uslg_opts, &server_socket);
+	if (ret)
+		return ret;
 
 	if (set_socket_blocking_flag(server_socket->fd_socket, 1)) {
 		int saved_errno = errno;
-		error_errno(_("could not set listener socket nonblocking '%s'"),
-			    path);
 		unix_stream_server__free(server_socket);
 		errno = saved_errno;
-		return NULL;
+		return -1;
 	}
 
+	*new_server_socket = server_socket;
+
 	trace2_data_string("ipc-server", NULL, "listen-with-lock", path);
-	return server_socket;
+	return 0;
 }
 
-static struct unix_stream_server_socket *setup_listener_socket(
+static int setup_listener_socket(
 	const char *path,
-	const struct ipc_server_opts *ipc_opts)
+	const struct ipc_server_opts *ipc_opts,
+	struct unix_stream_server_socket **new_server_socket)
 {
-	struct unix_stream_server_socket *server_socket;
+	int ret, saved_errno;
 
 	trace2_region_enter("ipc-server", "create-listener_socket", NULL);
-	server_socket = create_listener_socket(path, ipc_opts);
+
+	ret = create_listener_socket(path, ipc_opts, new_server_socket);
+
+	saved_errno = errno;
 	trace2_region_leave("ipc-server", "create-listener_socket", NULL);
+	errno = saved_errno;
 
-	return server_socket;
+	return ret;
 }
 
 /*
@@ -781,6 +788,7 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,
 	struct ipc_server_data *server_data;
 	int sv[2];
 	int k;
+	int ret;
 	int nr_threads = opts->nr_threads;
 
 	*returned_server_data = NULL;
@@ -792,25 +800,23 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,
 	 * connection or a shutdown request without spinning.
 	 */
 	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) < 0)
-		return error_errno(_("could not create socketpair for '%s'"),
-				   path);
+		return -1;
 
 	if (set_socket_blocking_flag(sv[1], 1)) {
 		int saved_errno = errno;
 		close(sv[0]);
 		close(sv[1]);
 		errno = saved_errno;
-		return error_errno(_("making socketpair nonblocking '%s'"),
-				   path);
+		return -1;
 	}
 
-	server_socket = setup_listener_socket(path, opts);
-	if (!server_socket) {
+	ret = setup_listener_socket(path, opts, &server_socket);
+	if (ret) {
 		int saved_errno = errno;
 		close(sv[0]);
 		close(sv[1]);
 		errno = saved_errno;
-		return -1;
+		return ret;
 	}
 
 	server_data = xcalloc(1, sizeof(*server_data));
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index f0cfbf9d15c3..5fd5960a1328 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -614,14 +614,16 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,
 	*returned_server_data = NULL;
 
 	ret = initialize_pipe_name(path, wpath, ARRAY_SIZE(wpath));
-	if (ret < 0)
-		return error(
-			_("could not create normalized wchar_t path for '%s'"),
-			path);
+	if (ret < 0) {
+		errno = EINVAL;
+		return -1;
+	}
 
 	hPipeFirst = create_new_pipe(wpath, 1);
-	if (hPipeFirst == INVALID_HANDLE_VALUE)
-		return error(_("IPC server already running on '%s'"), path);
+	if (hPipeFirst == INVALID_HANDLE_VALUE) {
+		errno = EADDRINUSE;
+		return -2;
+	}
 
 	server_data = xcalloc(1, sizeof(*server_data));
 	server_data->magic = MAGIC_SERVER_DATA;
diff --git a/simple-ipc.h b/simple-ipc.h
index f7e72e966f9a..dc3606e30bd6 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -178,6 +178,8 @@ struct ipc_server_opts
  *
  * Returns 0 if the asynchronous server pool was started successfully.
  * Returns -1 if not.
+ * Returns -2 if we could not startup because another server is using
+ * the socket or named pipe.
  *
  * When a client IPC message is received, the `application_cb` will be
  * called (possibly on a random thread) to handle the message and
@@ -217,6 +219,8 @@ void ipc_server_free(struct ipc_server_data *server_data);
  *
  * Returns 0 after the server has completed successfully.
  * Returns -1 if the server cannot be started.
+ * Returns -2 if we could not startup because another server is using
+ * the socket or named pipe.
  *
  * When a client IPC message is received, the `application_cb` will be
  * called (possibly on a random thread) to handle the message and
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index d67eaa9a6ecc..d0e6127528a7 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -219,6 +219,8 @@ static int test_app_cb(void *application_data,
  */
 static int daemon__run_server(const char *path, int argc, const char **argv)
 {
+	int ret;
+
 	struct ipc_server_opts opts = {
 		.nr_threads = 5
 	};
@@ -243,7 +245,13 @@ static int daemon__run_server(const char *path, int argc, const char **argv)
 	 * instance data, so pass an arbitrary pointer (that we'll later
 	 * verify made the round trip).
 	 */
-	return ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);
+	ret = ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);
+	if (ret == -2)
+		error(_("socket/pipe already in use: '%s'"), path);
+	else if (ret == -1)
+		error_errno(_("could not start server on: '%s'"), path);
+
+	return ret;
 }
 
 #ifndef GIT_WINDOWS_NATIVE
diff --git a/unix-stream-server.c b/unix-stream-server.c
index ad175ba731ca..f00298ca7ec3 100644
--- a/unix-stream-server.c
+++ b/unix-stream-server.c
@@ -5,41 +5,44 @@
 
 #define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
 
+/*
+ * Try to connect to a unix domain socket at `path` (if it exists) and
+ * see if there is a server listening.
+ *
+ * We don't know if the socket exists, whether a server died and
+ * failed to cleanup, or whether we have a live server listening, so
+ * we "poke" it.
+ *
+ * We immediately hangup without sending/receiving any data because we
+ * don't know anything about the protocol spoken and don't want to
+ * block while writing/reading data.  It is sufficient to just know
+ * that someone is listening.
+ */
 static int is_another_server_alive(const char *path,
 				   const struct unix_stream_listen_opts *opts)
 {
-	struct stat st;
-	int fd;
-
-	if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
-		/*
-		 * A socket-inode exists on disk at `path`, but we
-		 * don't know whether it belongs to an active server
-		 * or whether the last server died without cleaning
-		 * up.
-		 *
-		 * Poke it with a trivial connection to try to find
-		 * out.
-		 */
-		fd = unix_stream_connect(path, opts->disallow_chdir);
-		if (fd >= 0) {
-			close(fd);
-			return 1;
-		}
+	int fd = unix_stream_connect(path, opts->disallow_chdir);
+
+	if (fd >= 0) {
+		close(fd);
+		return 1;
 	}
 
 	return 0;
 }
 
-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
+int unix_stream_server__create(
 	const char *path,
-	const struct unix_stream_listen_opts *opts)
+	const struct unix_stream_listen_opts *opts,
+	struct unix_stream_server_socket **new_server_socket)
 {
 	struct lock_file lock = LOCK_INIT;
 	long timeout;
 	int fd_socket;
 	struct unix_stream_server_socket *server_socket;
 
+	*new_server_socket = NULL;
+
 	timeout = opts->timeout_ms;
 	if (opts->timeout_ms <= 0)
 		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
@@ -47,20 +50,17 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	/*
 	 * Create a lock at "<path>.lock" if we can.
 	 */
-	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {
-		error_errno(_("could not lock listener socket '%s'"), path);
-		return NULL;
-	}
+	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0)
+		return -1;
 
 	/*
 	 * If another server is listening on "<path>" give up.  We do not
 	 * want to create a socket and steal future connections from them.
 	 */
 	if (is_another_server_alive(path, opts)) {
-		errno = EADDRINUSE;
-		error_errno(_("listener socket already in use '%s'"), path);
 		rollback_lock_file(&lock);
-		return NULL;
+		errno = EADDRINUSE;
+		return -2;
 	}
 
 	/*
@@ -68,9 +68,10 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	 */
 	fd_socket = unix_stream_listen(path, opts);
 	if (fd_socket < 0) {
-		error_errno(_("could not create listener socket '%s'"), path);
+		int saved_errno = errno;
 		rollback_lock_file(&lock);
-		return NULL;
+		errno = saved_errno;
+		return -1;
 	}
 
 	server_socket = xcalloc(1, sizeof(*server_socket));
@@ -78,6 +79,8 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	server_socket->fd_socket = fd_socket;
 	lstat(path, &server_socket->st_socket);
 
+	*new_server_socket = server_socket;
+
 	/*
 	 * Always rollback (just delete) "<path>.lock" because we already created
 	 * "<path>" as a socket and do not want to commit_lock to do the atomic
@@ -85,7 +88,7 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	 */
 	rollback_lock_file(&lock);
 
-	return server_socket;
+	return 0;
 }
 
 void unix_stream_server__free(
diff --git a/unix-stream-server.h b/unix-stream-server.h
index 745849e31c30..f7e76dec59ac 100644
--- a/unix-stream-server.h
+++ b/unix-stream-server.h
@@ -12,10 +12,13 @@ struct unix_stream_server_socket {
 /*
  * Create a Unix Domain Socket at the given path under the protection
  * of a '.lock' lockfile.
+ *
+ * Returns 0 on success, -1 on error, -2 if socket is in use.
  */
-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
+int unix_stream_server__create(
 	const char *path,
-	const struct unix_stream_listen_opts *opts);
+	const struct unix_stream_listen_opts *opts,
+	struct unix_stream_server_socket **server_socket);
 
 /*
  * Close and delete the socket.
-- 
gitgitgadget

[PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

When checking to see if our unix domain socket was stolen, also check
whether the st.st_dev and st.st_mode fields changed (in addition to
the st.st_ino field).

The inode by itself is not unique; it is only unique on a given
device.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 unix-stream-server.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/unix-stream-server.c b/unix-stream-server.c
index f00298ca7ec3..366ece69306b 100644
--- a/unix-stream-server.c
+++ b/unix-stream-server.c
@@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(
 
 	if (st_now.st_ino != server_socket->st_socket.st_ino)
 		return 1;
+	if (st_now.st_dev != server_socket->st_socket.st_dev)
+		return 1;
 
-	/* We might also consider the ctime on some platforms. */
+	if (!S_ISSOCK(st_now.st_mode))
+		return 1;
 
 	return 0;
 }
-- 
gitgitgadget

[PATCH 6/8] test-simple-ipc: refactor command line option processing in helper

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Refactor command line option processing in simple-ipc helper under
the main "simple-ipc" command rather than in the individual subcommands.

Update "start-daemon" subcommand to pass socket pathname to background
process on both platforms.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/helper/test-simple-ipc.c | 307 ++++++++++++++++++-------------------
 1 file changed, 153 insertions(+), 154 deletions(-)

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index d0e6127528a7..116c824d7555 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -213,43 +213,53 @@ static int test_app_cb(void *application_data,
 	return app__unhandled_command(command, reply_cb, reply_data);
 }
 
+struct cl_args
+{
+	const char *subcommand;
+	const char *path;
+
+	int nr_threads;
+	int max_wait_sec;
+	int bytecount;
+	int batchsize;
+
+	char bytevalue;
+};
+
+struct cl_args cl_args = {
+	.subcommand = NULL,
+	.path = "ipc-test",
+
+	.nr_threads = 5,
+	.max_wait_sec = 60,
+	.bytecount = 1024,
+	.batchsize = 10,
+
+	.bytevalue = 'x',
+};
+
 /*
  * This process will run as a simple-ipc server and listen for IPC commands
  * from client processes.
  */
-static int daemon__run_server(const char *path, int argc, const char **argv)
+static int daemon__run_server(void)
 {
 	int ret;
 
 	struct ipc_server_opts opts = {
-		.nr_threads = 5
-	};
-
-	const char * const daemon_usage[] = {
-		N_("test-helper simple-ipc run-daemon [<options>"),
-		NULL
+		.nr_threads = cl_args.nr_threads,
 	};
-	struct option daemon_options[] = {
-		OPT_INTEGER(0, "threads", &opts.nr_threads,
-			    N_("number of threads in server thread pool")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, daemon_options, daemon_usage, 0);
-
-	if (opts.nr_threads < 1)
-		opts.nr_threads = 1;
 
 	/*
 	 * Synchronously run the ipc-server.  We don't need any application
 	 * instance data, so pass an arbitrary pointer (that we'll later
 	 * verify made the round trip).
 	 */
-	ret = ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);
+	ret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data);
 	if (ret == -2)
-		error(_("socket/pipe already in use: '%s'"), path);
+		error(_("socket/pipe already in use: '%s'"), cl_args.path);
 	else if (ret == -1)
-		error_errno(_("could not start server on: '%s'"), path);
+		error_errno(_("could not start server on: '%s'"), cl_args.path);
 
 	return ret;
 }
@@ -259,10 +269,12 @@ static int daemon__run_server(const char *path, int argc, const char **argv)
  * This is adapted from `daemonize()`.  Use `fork()` to directly create and
  * run the daemon in a child process.
  */
-static int spawn_server(const char *path,
-			const struct ipc_server_opts *opts,
-			pid_t *pid)
+static int spawn_server(pid_t *pid)
 {
+	struct ipc_server_opts opts = {
+		.nr_threads = cl_args.nr_threads,
+	};
+
 	*pid = fork();
 
 	switch (*pid) {
@@ -274,7 +286,8 @@ static int spawn_server(const char *path,
 		close(2);
 		sanitize_stdfds();
 
-		return ipc_server_run(path, opts, test_app_cb, (void*)&my_app_data);
+		return ipc_server_run(cl_args.path, &opts, test_app_cb,
+				      (void*)&my_app_data);
 
 	case -1:
 		return error_errno(_("could not spawn daemon in the background"));
@@ -289,9 +302,7 @@ static int spawn_server(const char *path,
  * have `fork(2)`.  Spawn a normal Windows child process but without the
  * limitations of `start_command()` and `finish_command()`.
  */
-static int spawn_server(const char *path,
-			const struct ipc_server_opts *opts,
-			pid_t *pid)
+static int spawn_server(pid_t *pid)
 {
 	char test_tool_exe[MAX_PATH];
 	struct strvec args = STRVEC_INIT;
@@ -305,7 +316,8 @@ static int spawn_server(const char *path,
 	strvec_push(&args, test_tool_exe);
 	strvec_push(&args, "simple-ipc");
 	strvec_push(&args, "run-daemon");
-	strvec_pushf(&args, "--threads=%d", opts->nr_threads);
+	strvec_pushf(&args, "--name=%s", cl_args.path);
+	strvec_pushf(&args, "--threads=%d", cl_args.nr_threads);
 
 	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
 	close(in);
@@ -325,8 +337,7 @@ static int spawn_server(const char *path,
  * let it get started and begin listening for requests on the socket
  * before reporting our success.
  */
-static int wait_for_server_startup(const char * path, pid_t pid_child,
-				   int max_wait_sec)
+static int wait_for_server_startup(pid_t pid_child)
 {
 	int status;
 	pid_t pid_seen;
@@ -334,7 +345,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,
 	time_t time_limit, now;
 
 	time(&time_limit);
-	time_limit += max_wait_sec;
+	time_limit += cl_args.max_wait_sec;
 
 	for (;;) {
 		pid_seen = waitpid(pid_child, &status, WNOHANG);
@@ -354,7 +365,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,
 			 * after a timeout on the lock), but we don't
 			 * care (who responds) if the socket is live.
 			 */
-			s = ipc_get_active_state(path);
+			s = ipc_get_active_state(cl_args.path);
 			if (s == IPC_STATE__LISTENING)
 				return 0;
 
@@ -377,7 +388,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,
 			 *
 			 * Again, we don't care who services the socket.
 			 */
-			s = ipc_get_active_state(path);
+			s = ipc_get_active_state(cl_args.path);
 			if (s == IPC_STATE__LISTENING)
 				return 0;
 
@@ -404,39 +415,15 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,
  * more control and better error reporting (and makes it easier to write
  * unit tests).
  */
-static int daemon__start_server(const char *path, int argc, const char **argv)
+static int daemon__start_server(void)
 {
 	pid_t pid_child;
 	int ret;
-	int max_wait_sec = 60;
-	struct ipc_server_opts opts = {
-		.nr_threads = 5
-	};
-
-	const char * const daemon_usage[] = {
-		N_("test-helper simple-ipc start-daemon [<options>"),
-		NULL
-	};
-
-	struct option daemon_options[] = {
-		OPT_INTEGER(0, "max-wait", &max_wait_sec,
-			    N_("seconds to wait for daemon to startup")),
-		OPT_INTEGER(0, "threads", &opts.nr_threads,
-			    N_("number of threads in server thread pool")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, daemon_options, daemon_usage, 0);
-
-	if (max_wait_sec < 0)
-		max_wait_sec = 0;
-	if (opts.nr_threads < 1)
-		opts.nr_threads = 1;
 
 	/*
 	 * Run the actual daemon in a background process.
 	 */
-	ret = spawn_server(path, &opts, &pid_child);
+	ret = spawn_server(&pid_child);
 	if (pid_child <= 0)
 		return ret;
 
@@ -444,7 +431,7 @@ static int daemon__start_server(const char *path, int argc, const char **argv)
 	 * Let the parent wait for the child process to get started
 	 * and begin listening for requests on the socket.
 	 */
-	ret = wait_for_server_startup(path, pid_child, max_wait_sec);
+	ret = wait_for_server_startup(pid_child);
 
 	return ret;
 }
@@ -455,27 +442,27 @@ static int daemon__start_server(const char *path, int argc, const char **argv)
  *
  * Returns 0 if the server is alive.
  */
-static int client__probe_server(const char *path)
+static int client__probe_server(void)
 {
 	enum ipc_active_state s;
 
-	s = ipc_get_active_state(path);
+	s = ipc_get_active_state(cl_args.path);
 	switch (s) {
 	case IPC_STATE__LISTENING:
 		return 0;
 
 	case IPC_STATE__NOT_LISTENING:
-		return error("no server listening at '%s'", path);
+		return error("no server listening at '%s'", cl_args.path);
 
 	case IPC_STATE__PATH_NOT_FOUND:
-		return error("path not found '%s'", path);
+		return error("path not found '%s'", cl_args.path);
 
 	case IPC_STATE__INVALID_PATH:
-		return error("invalid pipe/socket name '%s'", path);
+		return error("invalid pipe/socket name '%s'", cl_args.path);
 
 	case IPC_STATE__OTHER_ERROR:
 	default:
-		return error("other error for '%s'", path);
+		return error("other error for '%s'", cl_args.path);
 	}
 }
 
@@ -486,9 +473,9 @@ static int client__probe_server(const char *path)
  * argv[2] contains a simple (1 word) command that `test_app_cb()` (in
  * the daemon process) will understand.
  */
-static int client__send_ipc(int argc, const char **argv, const char *path)
+static int client__send_ipc(const char *send_token)
 {
-	const char *command = argc > 2 ? argv[2] : "(no command)";
+	const char *command = send_token ? send_token : "(no-command)";
 	struct strbuf buf = STRBUF_INIT;
 	struct ipc_client_connect_options options
 		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
@@ -496,7 +483,7 @@ static int client__send_ipc(int argc, const char **argv, const char *path)
 	options.wait_if_busy = 1;
 	options.wait_if_not_found = 0;
 
-	if (!ipc_client_send_command(path, &options, command, &buf)) {
+	if (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {
 		if (buf.len) {
 			printf("%s\n", buf.buf);
 			fflush(stdout);
@@ -506,7 +493,7 @@ static int client__send_ipc(int argc, const char **argv, const char *path)
 		return 0;
 	}
 
-	return error("failed to send '%s' to '%s'", command, path);
+	return error("failed to send '%s' to '%s'", command, cl_args.path);
 }
 
 /*
@@ -515,41 +502,23 @@ static int client__send_ipc(int argc, const char **argv, const char *path)
  * event in the server, so we spin and wait here for it to actually
  * shutdown to make the unit tests a little easier to write.
  */
-static int client__stop_server(int argc, const char **argv, const char *path)
+static int client__stop_server(void)
 {
-	const char *send_quit[] = { argv[0], "send", "quit", NULL };
-	int max_wait_sec = 60;
 	int ret;
 	time_t time_limit, now;
 	enum ipc_active_state s;
 
-	const char * const stop_usage[] = {
-		N_("test-helper simple-ipc stop-daemon [<options>]"),
-		NULL
-	};
-
-	struct option stop_options[] = {
-		OPT_INTEGER(0, "max-wait", &max_wait_sec,
-			    N_("seconds to wait for daemon to stop")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, stop_options, stop_usage, 0);
-
-	if (max_wait_sec < 0)
-		max_wait_sec = 0;
-
 	time(&time_limit);
-	time_limit += max_wait_sec;
+	time_limit += cl_args.max_wait_sec;
 
-	ret = client__send_ipc(3, send_quit, path);
+	ret = client__send_ipc("quit");
 	if (ret)
 		return ret;
 
 	for (;;) {
 		sleep_millisec(100);
 
-		s = ipc_get_active_state(path);
+		s = ipc_get_active_state(cl_args.path);
 
 		if (s != IPC_STATE__LISTENING) {
 			/*
@@ -597,19 +566,8 @@ static int do_sendbytes(int bytecount, char byte, const char *path,
 /*
  * Send an IPC command with ballast to an already-running server daemon.
  */
-static int client__sendbytes(int argc, const char **argv, const char *path)
+static int client__sendbytes(void)
 {
-	int bytecount = 1024;
-	char *string = "x";
-	const char * const sendbytes_usage[] = {
-		N_("test-helper simple-ipc sendbytes [<options>]"),
-		NULL
-	};
-	struct option sendbytes_options[] = {
-		OPT_INTEGER(0, "bytecount", &bytecount, N_("number of bytes")),
-		OPT_STRING(0, "byte", &string, N_("byte"), N_("ballast")),
-		OPT_END()
-	};
 	struct ipc_client_connect_options options
 		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
 
@@ -617,9 +575,8 @@ static int client__sendbytes(int argc, const char **argv, const char *path)
 	options.wait_if_not_found = 0;
 	options.uds_disallow_chdir = 0;
 
-	argc = parse_options(argc, argv, NULL, sendbytes_options, sendbytes_usage, 0);
-
-	return do_sendbytes(bytecount, string[0], path, &options);
+	return do_sendbytes(cl_args.bytecount, cl_args.bytevalue, cl_args.path,
+			    &options);
 }
 
 struct multiple_thread_data {
@@ -666,43 +623,20 @@ static void *multiple_thread_proc(void *_multiple_thread_data)
  * Start a client-side thread pool.  Each thread sends a series of
  * IPC requests.  Each request is on a new connection to the server.
  */
-static int client__multiple(int argc, const char **argv, const char *path)
+static int client__multiple(void)
 {
 	struct multiple_thread_data *list = NULL;
 	int k;
-	int nr_threads = 5;
-	int bytecount = 1;
-	int batchsize = 10;
 	int sum_join_errors = 0;
 	int sum_thread_errors = 0;
 	int sum_good = 0;
 
-	const char * const multiple_usage[] = {
-		N_("test-helper simple-ipc multiple [<options>]"),
-		NULL
-	};
-	struct option multiple_options[] = {
-		OPT_INTEGER(0, "bytecount", &bytecount, N_("number of bytes")),
-		OPT_INTEGER(0, "threads", &nr_threads, N_("number of threads")),
-		OPT_INTEGER(0, "batchsize", &batchsize, N_("number of requests per thread")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, multiple_options, multiple_usage, 0);
-
-	if (bytecount < 1)
-		bytecount = 1;
-	if (nr_threads < 1)
-		nr_threads = 1;
-	if (batchsize < 1)
-		batchsize = 1;
-
-	for (k = 0; k < nr_threads; k++) {
+	for (k = 0; k < cl_args.nr_threads; k++) {
 		struct multiple_thread_data *d = xcalloc(1, sizeof(*d));
 		d->next = list;
-		d->path = path;
-		d->bytecount = bytecount + batchsize*(k/26);
-		d->batchsize = batchsize;
+		d->path = cl_args.path;
+		d->bytecount = cl_args.bytecount + cl_args.batchsize*(k/26);
+		d->batchsize = cl_args.batchsize;
 		d->sum_errors = 0;
 		d->sum_good = 0;
 		d->letter = 'A' + (k % 26);
@@ -737,45 +671,110 @@ static int client__multiple(int argc, const char **argv, const char *path)
 
 int cmd__simple_ipc(int argc, const char **argv)
 {
-	const char *path = "ipc-test";
+	const char * const simple_ipc_usage[] = {
+		N_("test-helper simple-ipc is-active    [<name>] [<options>]"),
+		N_("test-helper simple-ipc run-daemon   [<name>] [<threads>]"),
+		N_("test-helper simple-ipc start-daemon [<name>] [<threads>] [<max-wait>]"),
+		N_("test-helper simple-ipc stop-daemon  [<name>] [<max-wait>]"),
+		N_("test-helper simple-ipc send         [<name>] [<token>]"),
+		N_("test-helper simple-ipc sendbytes    [<name>] [<bytecount>] [<byte>]"),
+		N_("test-helper simple-ipc multiple     [<name>] [<threads>] [<bytecount>] [<batchsize>]"),
+		NULL
+	};
+
+	const char *bytevalue = NULL;
+
+	struct option options[] = {
+#ifndef GIT_WINDOWS_NATIVE
+		OPT_STRING(0, "name", &cl_args.path, N_("name"), N_("name or pathname of unix domain socket")),
+#else
+		OPT_STRING(0, "name", &cl_args.path, N_("name"), N_("named-pipe name")),
+#endif
+		OPT_INTEGER(0, "threads", &cl_args.nr_threads, N_("number of threads in server thread pool")),
+		OPT_INTEGER(0, "max-wait", &cl_args.max_wait_sec, N_("seconds to wait for daemon to start or stop")),
+
+		OPT_INTEGER(0, "bytecount", &cl_args.bytecount, N_("number of bytes")),
+		OPT_INTEGER(0, "batchsize", &cl_args.batchsize, N_("number of requests per thread")),
+
+		OPT_STRING(0, "byte", &bytevalue, N_("byte"), N_("ballast character")),
+
+		OPT_END()
+	};
+
+	if (argc < 2)
+		usage_with_options(simple_ipc_usage, options);
+
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(simple_ipc_usage, options);
 
 	if (argc == 2 && !strcmp(argv[1], "SUPPORTS_SIMPLE_IPC"))
 		return 0;
 
+	cl_args.subcommand = argv[1];
+
+	argc--;
+	argv++;
+
+	argc = parse_options(argc, argv, NULL, options, simple_ipc_usage, 0);
+
+	if (cl_args.nr_threads < 1)
+		cl_args.nr_threads = 1;
+	if (cl_args.max_wait_sec < 0)
+		cl_args.max_wait_sec = 0;
+	if (cl_args.bytecount < 1)
+		cl_args.bytecount = 1;
+	if (cl_args.batchsize < 1)
+		cl_args.batchsize = 1;
+
+	if (bytevalue && *bytevalue)
+		cl_args.bytevalue = bytevalue[0];
+
 	/*
 	 * Use '!!' on all dispatch functions to map from `error()` style
 	 * (returns -1) style to `test_must_fail` style (expects 1).  This
 	 * makes shell error messages less confusing.
 	 */
 
-	if (argc == 2 && !strcmp(argv[1], "is-active"))
-		return !!client__probe_server(path);
+	if (!strcmp(cl_args.subcommand, "is-active"))
+		return !!client__probe_server();
 
-	if (argc >= 2 && !strcmp(argv[1], "run-daemon"))
-		return !!daemon__run_server(path, argc, argv);
+	if (!strcmp(cl_args.subcommand, "run-daemon"))
+		return !!daemon__run_server();
 
-	if (argc >= 2 && !strcmp(argv[1], "start-daemon"))
-		return !!daemon__start_server(path, argc, argv);
+	if (!strcmp(cl_args.subcommand, "start-daemon"))
+		return !!daemon__start_server();
 
 	/*
 	 * Client commands follow.  Ensure a server is running before
-	 * going any further.
+	 * sending any data.  This might be overkill, but then again
+	 * this is a test harness.
 	 */
-	if (client__probe_server(path))
-		return 1;
 
-	if (argc >= 2 && !strcmp(argv[1], "stop-daemon"))
-		return !!client__stop_server(argc, argv, path);
+	if (!strcmp(cl_args.subcommand, "stop-daemon")) {
+		if (client__probe_server())
+			return 1;
+		return !!client__stop_server();
+	}
 
-	if ((argc == 2 || argc == 3) && !strcmp(argv[1], "send"))
-		return !!client__send_ipc(argc, argv, path);
+	if (!strcmp(cl_args.subcommand, "send")) {
+		const char *send_token = argv[0];
+		if (client__probe_server())
+			return 1;
+		return !!client__send_ipc(send_token);
+	}
 
-	if (argc >= 2 && !strcmp(argv[1], "sendbytes"))
-		return !!client__sendbytes(argc, argv, path);
+	if (!strcmp(cl_args.subcommand, "sendbytes")) {
+		if (client__probe_server())
+			return 1;
+		return !!client__sendbytes();
+	}
 
-	if (argc >= 2 && !strcmp(argv[1], "multiple"))
-		return !!client__multiple(argc, argv, path);
+	if (!strcmp(cl_args.subcommand, "multiple")) {
+		if (client__probe_server())
+			return 1;
+		return !!client__multiple();
+	}
 
-	die("Unhandled argv[1]: '%s'", argv[1]);
+	die("Unhandled subcommand: '%s'", cl_args.subcommand);
 }
 #endif
-- 
gitgitgadget

[PATCH 7/8] test-simple-ipc: add --token=<token> string option

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Change the `test-tool simple-ipc send` subcommand take a string
option rather than assume the last unclaimed value of argv.

This makes it a little less awkward to use after the recent changes
that added the `--name=<name>` option.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/helper/test-simple-ipc.c | 25 ++++++++++++++++---------
 t/t0052-simple-ipc.sh      | 10 +++++-----
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 116c824d7555..4da63fd30c97 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -217,6 +217,7 @@ struct cl_args
 {
 	const char *subcommand;
 	const char *path;
+	const char *token;
 
 	int nr_threads;
 	int max_wait_sec;
@@ -229,6 +230,7 @@ struct cl_args
 struct cl_args cl_args = {
 	.subcommand = NULL,
 	.path = "ipc-test",
+	.token = NULL,
 
 	.nr_threads = 5,
 	.max_wait_sec = 60,
@@ -467,19 +469,22 @@ static int client__probe_server(void)
 }
 
 /*
- * Send an IPC command to an already-running server daemon and print the
- * response.
+ * Send an IPC command token to an already-running server daemon and
+ * print the response.
  *
- * argv[2] contains a simple (1 word) command that `test_app_cb()` (in
- * the daemon process) will understand.
+ * This is a simple 1 word command/token that `test_app_cb()` (in the
+ * daemon process) will understand.
  */
-static int client__send_ipc(const char *send_token)
+static int client__send_ipc(void)
 {
-	const char *command = send_token ? send_token : "(no-command)";
+	const char *command = "(no-command)";
 	struct strbuf buf = STRBUF_INIT;
 	struct ipc_client_connect_options options
 		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
 
+	if (cl_args.token && *cl_args.token)
+		command = cl_args.token;
+
 	options.wait_if_busy = 1;
 	options.wait_if_not_found = 0;
 
@@ -511,7 +516,9 @@ static int client__stop_server(void)
 	time(&time_limit);
 	time_limit += cl_args.max_wait_sec;
 
-	ret = client__send_ipc("quit");
+	cl_args.token = "quit";
+
+	ret = client__send_ipc();
 	if (ret)
 		return ret;
 
@@ -697,6 +704,7 @@ int cmd__simple_ipc(int argc, const char **argv)
 		OPT_INTEGER(0, "batchsize", &cl_args.batchsize, N_("number of requests per thread")),
 
 		OPT_STRING(0, "byte", &bytevalue, N_("byte"), N_("ballast character")),
+		OPT_STRING(0, "token", &cl_args.token, N_("token"), N_("command token to send to the server")),
 
 		OPT_END()
 	};
@@ -757,10 +765,9 @@ int cmd__simple_ipc(int argc, const char **argv)
 	}
 
 	if (!strcmp(cl_args.subcommand, "send")) {
-		const char *send_token = argv[0];
 		if (client__probe_server())
 			return 1;
-		return !!client__send_ipc(send_token);
+		return !!client__send_ipc();
 	}
 
 	if (!strcmp(cl_args.subcommand, "sendbytes")) {
diff --git a/t/t0052-simple-ipc.sh b/t/t0052-simple-ipc.sh
index 18dcc8130728..ff98be31a51b 100755
--- a/t/t0052-simple-ipc.sh
+++ b/t/t0052-simple-ipc.sh
@@ -20,7 +20,7 @@ test_expect_success 'start simple command server' '
 '
 
 test_expect_success 'simple command server' '
-	test-tool simple-ipc send ping >actual &&
+	test-tool simple-ipc send --token=ping >actual &&
 	echo pong >expect &&
 	test_cmp expect actual
 '
@@ -31,19 +31,19 @@ test_expect_success 'servers cannot share the same path' '
 '
 
 test_expect_success 'big response' '
-	test-tool simple-ipc send big >actual &&
+	test-tool simple-ipc send --token=big >actual &&
 	test_line_count -ge 10000 actual &&
 	grep -q "big: [0]*9999\$" actual
 '
 
 test_expect_success 'chunk response' '
-	test-tool simple-ipc send chunk >actual &&
+	test-tool simple-ipc send --token=chunk >actual &&
 	test_line_count -ge 10000 actual &&
 	grep -q "big: [0]*9999\$" actual
 '
 
 test_expect_success 'slow response' '
-	test-tool simple-ipc send slow >actual &&
+	test-tool simple-ipc send --token=slow >actual &&
 	test_line_count -ge 100 actual &&
 	grep -q "big: [0]*99\$" actual
 '
@@ -116,7 +116,7 @@ test_expect_success 'stress test threads' '
 test_expect_success 'stop-daemon works' '
 	test-tool simple-ipc stop-daemon &&
 	test_must_fail test-tool simple-ipc is-active &&
-	test_must_fail test-tool simple-ipc send ping
+	test_must_fail test-tool simple-ipc send --token=ping
 '
 
 test_done
-- 
gitgitgadget

[PATCH 8/8] simple-ipc: update design documentation with more details

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/technical/api-simple-ipc.txt | 131 ++++++++++++++++-----
 1 file changed, 101 insertions(+), 30 deletions(-)

diff --git a/Documentation/technical/api-simple-ipc.txt b/Documentation/technical/api-simple-ipc.txt
index 670a5c163e39..d79ad323e675 100644
--- a/Documentation/technical/api-simple-ipc.txt
+++ b/Documentation/technical/api-simple-ipc.txt
@@ -1,34 +1,105 @@
-simple-ipc API
+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.
+The Simple-IPC API is a collection of `ipc_` prefixed library routines
+and a basic communication protocol that allow an IPC-client process to
+send an application-specific IPC-request message to an IPC-server
+process and receive an application-specific IPC-response message.
 
 Communication occurs over a named pipe on Windows and a Unix domain
-socket on other platforms.  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.
+socket on other platforms.  IPC-clients and IPC-servers rendezvous at
+a previously agreed-to application-specific pathname (which is outside
+the scope of this design) that is local to the computer system.
+
+The IPC-server routines within the server application process create a
+thread pool to listen for connections and receive request messages
+from multiple concurrent IPC-clients.  When received, these messages
+are dispatched up to the server application callbacks for handling.
+IPC-server routines then incrementally relay responses back to the
+IPC-client.
+
+The IPC-client routines within a client application process connect
+to the IPC-server and send a request message and wait for a response.
+When received, the response is returned back the caller.
+
+For example, the `fsmonitor--daemon` feature will be built as a server
+application on top of the IPC-server library routines.  It will have
+threads watching for file system events and a thread pool waiting for
+client connections.  Clients, such as `git status` will request a list
+of file system events since a point in time and the server will
+respond with a list of changed files and directories.  The formats of
+the request and response are application-specific; the IPC-client and
+IPC-server routines treat them as opaque byte streams.
+
+
+Comparison with sub-process model
+---------------------------------
+
+The Simple-IPC mechanism differs from the existing `sub-process.c`
+model (Documentation/technical/long-running-process-protocol.txt) and
+used by applications like Git-LFS.  In the LFS-style sub-process model
+the helper is started by the foreground process, communication happens
+via a pair of file descriptors bound to the stdin/stdout of the
+sub-process, the sub-process only serves the current foreground
+process, and the sub-process exits when the foreground process
+terminates.
+
+In the Simple-IPC model the server is a very long-running service.  It
+can service many clients at the same time and has a private socket or
+named pipe connection to each active client.  It might be started
+(on-demand) by the current client process or it might have been
+started by a previous client or by the OS at boot time.  The server
+process is not associated with a terminal and it persists after
+clients terminate.  Clients do not have access to the stdin/stdout of
+the server process and therefore must communicate over sockets or
+named pipes.
+
+
+Server startup and shutdown
+---------------------------
+
+How an application server based upon IPC-server is started is also
+outside the scope of the Simple-IPC design and is a property of the
+application using it.  For example, the server might be started or
+restarted during routine maintenance operations, or it might be
+started as a system service during the system boot-up sequence, or it
+might be started on-demand by a foreground Git command when needed.
+
+Similarly, server shutdown is a property of the application using
+the simple-ipc routines.  For example, the server might decide to
+shutdown when idle or only upon explicit request.
+
+
+Simple-IPC protocol
+-------------------
+
+The Simple-IPC protocol consists of a single request message from the
+client and an optional response message from the server.  Both the
+client and server messages are unlimited in length and are terminated
+with a flush packet.
+
+The pkt-line routines (Documentation/technical/protocol-common.txt)
+are used to simplify buffer management during message generation,
+transmission, and reception.  A flush packet is used to mark the end
+of the message.  This allows the sender to incrementally generate and
+transmit the message.  It allows the receiver to incrementally receive
+the message in chunks and to know when they have received the entire
+message.
+
+The actual byte format of the client request and server response
+messages are application specific.  The IPC layer transmits and
+receives them as opaque byte buffers without any concern for the
+content within.  It is the job of the calling application layer to
+understand the contents of the request and response messages.
+
+
+Summary
+-------
+
+Conceptually, the Simple-IPC protocol is similar to an HTTP REST
+request.  Clients connect, make an application-specific and
+stateless request, receive an application-specific
+response, and disconnect.  It is a one round trip facility for
+querying the server.  The Simple-IPC routines hide the socket,
+named pipe, and thread pool details and allow the application
+layer to focus on the application at hand.
-- 
gitgitgadget

Re: [PATCH 1/8] pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`
> and allocate a temporary buffer within the function.
>
> In 3e4447f1ea (pkt-line: eliminate the need for static buffer in
> packet_write_gently(), 2021-02-13) we added the argument in order to
> get rid of a static buffer to make the routine safe in multi-threaded
> contexts and to avoid putting a very large buffer on the stack.  This
> delegated the problem to the caller who has more knowledge of the use
> case and can best decide the most efficient way to provide such a
> buffer.
>
> However, in practice, the (currently, only) caller just created the
> buffer on the stack, so we might as well just allocate it within the
> function and restore the original API.

Hmph, I would have expected, since we already have changed the
callchain to pass the buffer down, that we'd keep the structure and
update the caller to heap-allocate, but I think I like the result of
this patch better.  It's not like the caller can allocate and reuse
a single buffer with repeated calls to the function; rather, the
caller makes a single call that results in relaying many bytes by
the helper, all done in a loop, and it is sufficient for the helper
to allocate/deallocate outside of its loop to reuse the buffer.

Re: [PATCH 2/8] unix-socket: simplify initialization of unix_stream_listen_opts

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  	struct lock_file lock = LOCK_INIT;
> +	long timeout;
>  	int fd_socket;
>  	struct unix_stream_server_socket *server_socket;
>  
> +	timeout = opts->timeout_ms;
> +	if (opts->timeout_ms <= 0)
> +		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;

If we have 0 as a special value to tell this API to use the default
value, do we need to treat negative values the same way?

Do we see any value in being able to say "no timeout---if we can do
so immediately fine, otherwise return me a failure"?  Deep in the
callchain, lockfile.c::lock_file_timeout(), which is the workhorse
of the feature, notices timeout_ms==0 and makes a direct call to
lock_file() after all, so we are prepared for such a caller already.

And if this is such a caller that may benefit from being able to say
"fail if we cannot immediately lock", perhaps we might want to allow
0 to be used as a real value and use something else as a signal to
use the timeout value determined by the helper as the default.

IOW, I would find the above iffy and prefer any of the following
over it:

(0)	if (!opts->timeout_ms)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
	else if (opts->timeout_ms < 0)
		BUG("...");

(1)	if (opts->timeout_ms < 0)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;

(2)	if (opts->timeout_ms == -1)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
	else if (opts->timeout_ms < 0)
		BUG("...");

> diff --git a/unix-socket.h b/unix-socket.h
> index 8faf5b692f90..bec925ee0213 100644
> --- a/unix-socket.h
> +++ b/unix-socket.h
> @@ -7,13 +7,10 @@ struct unix_stream_listen_opts {
>  	unsigned int disallow_chdir:1;
>  };
>  
> -#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
> -#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
> -
>  #define UNIX_STREAM_LISTEN_OPTS_INIT \
>  { \
> -	.timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \
> -	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
> +	.timeout_ms = 0, \
> +	.listen_backlog_size = 0, \
>  	.disallow_chdir = 0, \
>  }

I thought the point of suggested fix was to allow 0-initialize the
whole structure, so that we do not have to have the C preprocessor
macro UNIX_STREAM_LISTEN_OPTS_INIT at all.  I.e. it would allow us
to do

-	struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+	struct unix_stream_listen_opts opts = { 0 };

in builtin/credential-cache--daemon.c::serve_cache().

If we cannot use 0 as a special value, however, for the timeout,
then we cannot get rid of UNIX_STREAM_LISTEN_OPTS_INIT, but at least
we should be able to do

	#define UNIX_STREAM_LISTEN_OPTS_INIT { .timeout_ms = -1 }

and leave everything else 0-initialized.

Thanks.

Re: [PATCH 0/8] Simple IPC Cleanups

[Junio C Hamano]

sparse failed seen, so I tentatively added this on top of your
series.

Thanks.

 t/helper/test-simple-ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 4da63fd30c..42040ef81b 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -227,7 +227,7 @@ struct cl_args
 	char bytevalue;
 };
 
-struct cl_args cl_args = {
+static struct cl_args cl_args = {
 	.subcommand = NULL,
 	.path = "ipc-test",
 	.token = NULL,

Re: [PATCH 0/8] Simple IPC Cleanups

[Jeff Hostetler]

On 3/4/21 7:24 PM, Junio C Hamano wrote:
> sparse failed seen, so I tentatively added this on top of your
> series.
> 
> Thanks.
> 
>   t/helper/test-simple-ipc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
> index 4da63fd30c..42040ef81b 100644
> --- a/t/helper/test-simple-ipc.c
> +++ b/t/helper/test-simple-ipc.c
> @@ -227,7 +227,7 @@ struct cl_args
>   	char bytevalue;
>   };
>   
> -struct cl_args cl_args = {
> +static struct cl_args cl_args = {
>   	.subcommand = NULL,
>   	.path = "ipc-test",
>   	.token = NULL,
> 

Thanks!!

I'll update my copy.

Since things are settling down on the whole simple-ipc series and
we are waiting for the current release cycle to complete before
going any further, I'm going to let this series rest for a bit
in case there are any more comments.

Then I'll combine the 2 parts and rebase/re-roll all of this into
a single series that we can re-eval for "next" post 2.31.

Jeff

Re: [PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks

[René Scharfe]

Am 04.03.21 um 21:17 schrieb Jeff Hostetler via GitGitGadget:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> When checking to see if our unix domain socket was stolen, also check
> whether the st.st_dev and st.st_mode fields changed (in addition to
> the st.st_ino field).
>
> The inode by itself is not unique; it is only unique on a given
> device.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  unix-stream-server.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/unix-stream-server.c b/unix-stream-server.c
> index f00298ca7ec3..366ece69306b 100644
> --- a/unix-stream-server.c
> +++ b/unix-stream-server.c
> @@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(
>
>  	if (st_now.st_ino != server_socket->st_socket.st_ino)
>  		return 1;
> +	if (st_now.st_dev != server_socket->st_socket.st_dev)
> +		return 1;
>
> -	/* We might also consider the ctime on some platforms. */

Why remove that comment?  (This change is not mentioned in the commit
message.)

> +	if (!S_ISSOCK(st_now.st_mode))
> +		return 1;
>
>  	return 0;
>  }
>

Re: [PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks

[Jeff Hostetler]

On 3/6/21 6:42 AM, René Scharfe wrote:
> Am 04.03.21 um 21:17 schrieb Jeff Hostetler via GitGitGadget:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> When checking to see if our unix domain socket was stolen, also check
>> whether the st.st_dev and st.st_mode fields changed (in addition to
>> the st.st_ino field).
>>
>> The inode by itself is not unique; it is only unique on a given
>> device.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>   unix-stream-server.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/unix-stream-server.c b/unix-stream-server.c
>> index f00298ca7ec3..366ece69306b 100644
>> --- a/unix-stream-server.c
>> +++ b/unix-stream-server.c
>> @@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(
>>
>>   	if (st_now.st_ino != server_socket->st_socket.st_ino)
>>   		return 1;
>> +	if (st_now.st_dev != server_socket->st_socket.st_dev)
>> +		return 1;
>>
>> -	/* We might also consider the ctime on some platforms. */
> 
> Why remove that comment?  (This change is not mentioned in the commit
> message.)

I added it as a TODO to myself thinking that it might give us
additional assurances on some platforms while I was originally
getting things working.  In hindsight (and now that we have the
lockfile helping us), I didn't think it was actually needed, so
I removed it.

I didn't think it warranted a mention in the commit message.

> 
>> +	if (!S_ISSOCK(st_now.st_mode))
>> +		return 1;
>>
>>   	return 0;
>>   }
>>
>