diff mbox series

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

Message ID 6ef867bf37d366071d5f0f101e7430d859f529b5.1614889047.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Simple IPC Cleanups | expand

Commit Message

Jeff Hostetler March 4, 2021, 8:17 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Change the public initialization of `struct unix_stream_listen_opts`
to be all zeroes.  Hide the default values for the timeout and backlog
values inside `unix-socket.c`.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 unix-socket.c | 11 +++++++++--
 unix-socket.h |  7 ++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

Comments

Junio C Hamano March 4, 2021, 11:12 p.m. UTC | #1 
"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.
diff mbox series

Patch

diff --git a/unix-socket.c b/unix-socket.c
index 647bbde37f97..c9ea1de43bd2 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -2,6 +2,9 @@ 
 #include "lockfile.h"
 #include "unix-socket.h"
 
+#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
+#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
+
 static int chdir_len(const char *orig, int len)
 {
 	char *path = xmemdupz(orig, len);
@@ -165,14 +168,18 @@  struct unix_stream_server_socket *unix_stream_server__listen_with_lock(
 	const struct unix_stream_listen_opts *opts)
 {
 	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;
+
 	/*
 	 * Create a lock at "<path>.lock" if we can.
 	 */
-	if (hold_lock_file_for_update_timeout(&lock, path, 0,
-					      opts->timeout_ms) < 0) {
+	if (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {
 		error_errno(_("could not lock listener socket '%s'"), path);
 		return NULL;
 	}
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, \
 }