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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2021, 20:54 UTC
Message-ID: <xmqqim68hsb8.fsf@gitster.c.googlers.com>
In-Reply-To: <YDijhRaib5It/apG@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

> 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?

Show 17 quoted lines

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