diff mbox series

[v4,08/12] unix-socket: add backlog size option to unix_stream_listen()

Message ID 985b2e02b2df7725d70f1365f7cd2e525c9f3ade.1613598529.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 5c53ef087420fbbd52af367da455bac67e07ed9d
Headers show
Series Simple IPC Mechanism | expand

Commit Message

Jeff Hostetler Feb. 17, 2021, 9:48 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Update `unix_stream_listen()` to take an options structure to override
default behaviors.  This commit includes the size of the `listen()` backlog.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/credential-cache--daemon.c |  3 ++-
 unix-socket.c                      |  9 +++++++--
 unix-socket.h                      | 14 +++++++++++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

Comments

Jeff King Feb. 26, 2021, 7:30 a.m. UTC | #1 
On Wed, Feb 17, 2021 at 09:48:44PM +0000, Jeff Hostetler via GitGitGadget wrote:

> @@ -106,7 +108,10 @@ int unix_stream_listen(const char *path)
>  	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
>  		goto fail;
>  
> -	if (listen(fd, 5) < 0)
> +	backlog = opts->listen_backlog_size;
> +	if (backlog <= 0)
> +		backlog = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG;
> +	if (listen(fd, backlog) < 0)
>  		goto fail;

OK, so we still have the fallback-on-zero here, which is good...

> +struct unix_stream_listen_opts {
> +	int listen_backlog_size;
> +};
> +
> +#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
> +
> +#define UNIX_STREAM_LISTEN_OPTS_INIT \
> +{ \
> +	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
> +}

...but I thought the plan was to drop this initialization in favor of a
zero-initialization. What you have certainly wouldn't do the wrong
thing, but it just seems weirdly redundant. Unless some caller really
wants to know what the default will be?

-Peff
Junio C Hamano March 3, 2021, 8:54 p.m. UTC | #2 
Jeff King <peff@peff.net> writes:

> On Wed, Feb 17, 2021 at 09:48:44PM +0000, Jeff Hostetler via GitGitGadget wrote:
>
>> @@ -106,7 +108,10 @@ int unix_stream_listen(const char *path)
>>  	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
>>  		goto fail;
>>  
>> -	if (listen(fd, 5) < 0)
>> +	backlog = opts->listen_backlog_size;
>> +	if (backlog <= 0)
>> +		backlog = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG;
>> +	if (listen(fd, backlog) < 0)
>>  		goto fail;


Luckily there is no "pass 0 and the platforms will choose an
appropriate backlog value", so "pass 0 to get the default Git
chooses" is OK, but do we even want to allow passing any negative
value?  Shouldn't it be diagnosed as an error instead?

> OK, so we still have the fallback-on-zero here, which is good...
>
>> +struct unix_stream_listen_opts {
>> +	int listen_backlog_size;
>> +};
>> +
>> +#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
>> +
>> +#define UNIX_STREAM_LISTEN_OPTS_INIT \
>> +{ \
>> +	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
>> +}
>
> ...but I thought the plan was to drop this initialization in favor of a
> zero-initialization. What you have certainly wouldn't do the wrong
> thing, but it just seems weirdly redundant. Unless some caller really
> wants to know what the default will be?

Very true.  The code knows the exact value input 0 has to fall back
to; we shouldn't have to initialize to that same exact value and I
do not offhand see why the DEFAULT_UNIX_STREAM_LISTEN_BACKLOG needs
to be a public constant.

Thanks.
diff mbox series

Patch

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index c61f123a3b81..4c6c89ab0de2 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -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);
 
diff --git a/unix-socket.c b/unix-socket.c
index 69f81d64e9d5..5ac7dafe9828 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -89,9 +89,11 @@  int unix_stream_connect(const char *path)
 	return -1;
 }
 
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path,
+		       const struct unix_stream_listen_opts *opts)
 {
 	int fd = -1, saved_errno;
+	int backlog;
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx;
 
@@ -106,7 +108,10 @@  int unix_stream_listen(const char *path)
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;
 
-	if (listen(fd, 5) < 0)
+	backlog = opts->listen_backlog_size;
+	if (backlog <= 0)
+		backlog = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG;
+	if (listen(fd, backlog) < 0)
 		goto fail;
 
 	unix_sockaddr_cleanup(&ctx);
diff --git a/unix-socket.h b/unix-socket.h
index e271aeec5a07..06a5a05b03fe 100644
--- a/unix-socket.h
+++ b/unix-socket.h
@@ -1,7 +1,19 @@ 
 #ifndef UNIX_SOCKET_H
 #define UNIX_SOCKET_H
 
+struct unix_stream_listen_opts {
+	int listen_backlog_size;
+};
+
+#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
+
+#define UNIX_STREAM_LISTEN_OPTS_INIT \
+{ \
+	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
+}
+
 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 */