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

["Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>]

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