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

[Junio C Hamano <gitster@pobox.com>]

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

>  	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("...");

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