| Message ID | b368318e6a23f8c4e60f77a8b81b558c523d5b03.1613598529.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b71d9193cd57ec183e928859563269329e2a1dd7 |
| Headers | show |
| Series | Simple IPC Mechanism | expand |
On Wed, Feb 17, 2021 at 09:48:43PM +0000, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > The static helper function `unix_stream_socket()` calls `die()`. This > is not appropriate for all callers. Eliminate the wrapper function > and make the callers propagate the error. Thanks for breaking it up this way. It's (IMHO) much easier to see the motivation and impact of the changes now. There's a small typo in the subject: > Subject: unix-socket: elimiate static unix_stream_socket() helper function -Peff
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > int unix_stream_connect(const char *path) > { > - int fd, saved_errno; > + int fd = -1, saved_errno; > struct sockaddr_un sa; > struct unix_sockaddr_context ctx; > > if (unix_sockaddr_init(&sa, path, &ctx) < 0) > return -1; > - fd = unix_stream_socket(); > + fd = socket(AF_UNIX, SOCK_STREAM, 0); > + if (fd < 0) > + goto fail; > + > if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) > goto fail; > unix_sockaddr_cleanup(&ctx); > @@ -87,15 +82,16 @@ int unix_stream_connect(const char *path) > > fail: > saved_errno = errno; > + if (fd != -1) > + close(fd); > unix_sockaddr_cleanup(&ctx); > - close(fd); > errno = saved_errno; > return -1; > } So, the difference is that the caller must be prepared to see and handle error return from this function when creating socket fails, but existing callers must be prepared to handle error returns from this function for different reasons (e.g. we may successfully make a socket, but connect may fail) already anyway, so this should be a fairly safe thing to do. The sole caller send_request() in credential-cache.c will relay the error return back to do_cache() which cares what errno it got, and that code does seem to care what kind of error caused unix_stream_connect() to fail. And the new error case introduced by this patch won't result in ENOENT or ECONNREFUSED to cause the code to fall back to "if the thing is not running, let's try starting it and try again". OK. > int unix_stream_listen(const char *path) > { This one is simpler to vet its caller. It immediately dies upon any error return. Thanks.
diff --git a/unix-socket.c b/unix-socket.c index 19ed48be9902..69f81d64e9d5 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); @@ -73,13 +65,16 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, int unix_stream_connect(const char *path) { - int fd, saved_errno; + int fd = -1, saved_errno; struct sockaddr_un sa; struct unix_sockaddr_context ctx; if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1; - fd = unix_stream_socket(); + fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) + goto fail; + if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) goto fail; unix_sockaddr_cleanup(&ctx); @@ -87,15 +82,16 @@ int unix_stream_connect(const char *path) fail: saved_errno = errno; + if (fd != -1) + close(fd); unix_sockaddr_cleanup(&ctx); - close(fd); errno = saved_errno; return -1; } int unix_stream_listen(const char *path) { - int fd, saved_errno; + int fd = -1, saved_errno; struct sockaddr_un sa; struct unix_sockaddr_context ctx; @@ -103,7 +99,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) + goto fail; if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) goto fail; @@ -116,8 +114,9 @@ int unix_stream_listen(const char *path) fail: saved_errno = errno; + if (fd != -1) + close(fd); unix_sockaddr_cleanup(&ctx); - close(fd); errno = saved_errno; return -1; }