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

From: Junio C Hamano <[email-masked]>
Date: Wed, 03 Mar 2021 12:54:03 -0800

Jeff King <[email-masked]> 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.