Re: [PATCH 2/8] unix-socket: simplify initialization of unix_stream_listen_opts

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2021, 23:12 UTC
Message-ID: <xmqqa6ricy2v.fsf@gitster.c.googlers.com>
In-Reply-To: <6ef867bf37d366071d5f0f101e7430d859f529b5.1614889047.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

>  	struct lock_file lock = LOCK_INIT;
> +	long timeout;
>  	int fd_socket;
>  	struct unix_stream_server_socket *server_socket;
>  
> +	timeout = opts->timeout_ms;
> +	if (opts->timeout_ms <= 0)
> +		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;

If we have 0 as a special value to tell this API to use the default value, do we need to treat negative values the same way?

Do we see any value in being able to say "no timeout---if we can do so immediately fine, otherwise return me a failure"? Deep in the callchain, lockfile.c::lock_file_timeout(), which is the workhorse of the feature, notices timeout_ms==0 and makes a direct call to lock_file() after all, so we are prepared for such a caller already.

And if this is such a caller that may benefit from being able to say "fail if we cannot immediately lock", perhaps we might want to allow 0 to be used as a real value and use something else as a signal to use the timeout value determined by the helper as the default.

IOW, I would find the above iffy and prefer any of the following over it:

(0)	if (!opts->timeout_ms)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
	else if (opts->timeout_ms < 0)
		BUG("...");

(1)	if (opts->timeout_ms < 0)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;

(2)	if (opts->timeout_ms == -1)
		timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
	else if (opts->timeout_ms < 0)
		BUG("...");

Show 19 quoted lines

> diff --git a/unix-socket.h b/unix-socket.h
> index 8faf5b692f90..bec925ee0213 100644
> --- a/unix-socket.h
> +++ b/unix-socket.h
> @@ -7,13 +7,10 @@ struct unix_stream_listen_opts {
>  	unsigned int disallow_chdir:1;
>  };
>  
> -#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
> -#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
> -
>  #define UNIX_STREAM_LISTEN_OPTS_INIT \
>  { \
> -	.timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \
> -	.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
> +	.timeout_ms = 0, \
> +	.listen_backlog_size = 0, \
>  	.disallow_chdir = 0, \
>  }

I thought the point of suggested fix was to allow 0-initialize the whole structure, so that we do not have to have the C preprocessor macro UNIX_STREAM_LISTEN_OPTS_INIT at all. I.e. it would allow us to do

- struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT; + struct unix_stream_listen_opts opts = { 0 };

in builtin/credential-cache--daemon.c::serve_cache().

If we cannot use 0 as a special value, however, for the timeout, then we cannot get rid of UNIX_STREAM_LISTEN_OPTS_INIT, but at least we should be able to do

	#define UNIX_STREAM_LISTEN_OPTS_INIT { .timeout_ms = -1 }

and leave everything else 0-initialized.

Thanks.