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

Jeff King [off-list ref] writes:

On Wed, Feb 17, 2021 at 09:48:44PM +0000, Jeff Hostetler via GitGitGadget wrote:

quoted

@@ -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...

quoted

+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.