[v2,10/14] unix-socket: elimiate static unix_stream_socket() helper function

Commit Message

Jeff Hostetler Feb. 1, 2021, 7:45 p.m. UTC

From: Jeff Hostetler <[e-mail]>

The static helper function `unix_stream_socket()` calls `die()`.  This is not
appropriate for all callers.  Eliminate the wrapper function and move the
existing error handling to the callers in preparation for adapting specific
callers.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 unix-socket.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Jeff King Feb. 2, 2021, 9:54 a.m. UTC | #1

On Mon, Feb 01, 2021 at 07:45:43PM +0000, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <[e-mail]>
> 
> The static helper function `unix_stream_socket()` calls `die()`.  This is not
> appropriate for all callers.  Eliminate the wrapper function and move the
> existing error handling to the callers in preparation for adapting specific
> callers.

Thanks, this looks good.

> -static int unix_stream_socket(void)
> -{
> -	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> -	if (fd < 0)
> -		die_errno("unable to create socket");
> -	return fd;
> -}

This could become a one-liner:

  return socket(AF_UNIX, SOCK_STREAM, 0);

to keep the details abstracted. But it's local to this file, the callers
are already necessarily full of bsd-socket arcana, and it's not like the
magic words there have ever changed in 30+ years. Putting it inline
seems quite reasonable. :)

-Peff

Jeff King Feb. 2, 2021, 9:58 a.m. UTC | #2

On Mon, Feb 01, 2021 at 07:45:43PM +0000, Jeff Hostetler via GitGitGadget wrote:

>  static int chdir_len(const char *orig, int len)
>  {
>  	char *path = xmemdupz(orig, len);
> @@ -79,7 +71,10 @@ int unix_stream_connect(const char *path)
>  
>  	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
>  		return -1;
> -	fd = unix_stream_socket();
> +	fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +	if (fd < 0)
> +		die_errno("unable to create socket");
> +

Reading the next patch, I suddenly realized that these are die calls,
and not just passing along the error (which you then fix in the next
patch). It seems like that should be happening here in this patch.
Callers must already be ready to handle an error (we return -1 in the
context above).

> @@ -103,7 +98,9 @@ int unix_stream_listen(const char *path)
>  
>  	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
>  		return -1;
> -	fd = unix_stream_socket();
> +	fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +	if (fd < 0)
> +		die_errno("unable to create socket");

Ditto here.

-Peff

Patch

diff --git a/unix-socket.c b/unix-socket.c
index 19ed48be990..ef2aeb46bcd 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -1,14 +1,6 @@ 
 #include "cache.h"
 #include "unix-socket.h"
 
-static int unix_stream_socket(void)
-{
-	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
-	if (fd < 0)
-		die_errno("unable to create socket");
-	return fd;
-}
-
 static int chdir_len(const char *orig, int len)
 {
 	char *path = xmemdupz(orig, len);
@@ -79,7 +71,10 @@  int unix_stream_connect(const char *path)
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
-	fd = unix_stream_socket();
+	fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (fd < 0)
+		die_errno("unable to create socket");
+
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;
 	unix_sockaddr_cleanup(&ctx);
@@ -103,7 +98,9 @@  int unix_stream_listen(const char *path)
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
-	fd = unix_stream_socket();
+	fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (fd < 0)
+		die_errno("unable to create socket");
 
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;