Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3

From: Jeff King <peff@peff.net>
Date: May 11, 2021, 18:08 UTC
Message-ID: <YJrIMbr6VkYGQMfs@coredump.intra.peff.net>
In-Reply-To: <YJrEy8IPi+PFWuzT@coredump.intra.peff.net>

On Tue, May 11, 2021 at 01:54:19PM -0400, Jeff King wrote:

Show 8 quoted lines

> > > In any case, this needs to be signed off before it gets carved into
> > > our history.
> > 
> > Done, and also changed the variable name to minimize the size of the
> > diff. A shorter name allowed for less re-flowing of lines.
> 
> It's quite short. I'm OK with it for a static-local function with few
> callers like this, though.

Actually, I just noticed that you did not introduce "e" in the caller. So it is not even a new name, and you are just following convention.

I also wondered briefly why we needed the out-parameter at all, and not just letting the caller look at errno. The answer is that we need to preserve it across the close() call. The more usual thing in our code base _would_ be to use saved_errno, but not have it as an out-parameter.

I.e.:

diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index ae052a07fe..bda283e7f4 100644
--- a/trace2/tr2_dst.c
+++ b/trace2/tr2_dst.c
@@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
 
 	fd = socket(AF_UNIX, sock_type, 0);
 	if (fd == -1)
-		return errno;
+		return -1;
 
 	sa.sun_family = AF_UNIX;
 	strlcpy(sa.sun_path, path, sizeof(sa.sun_path));
 
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {
-		int e = errno;
+		int saved_errno = errno;
 		close(fd);
-		return e;
+		errno = saved_errno;
+		return -1;
 	}
 
 	*out_fd = fd;
@@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
 {
 	unsigned int uds_try = 0;
 	int fd;
-	int e;
 	const char *path = NULL;
 
 	/*
@@ -271,23 +271,21 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
 	}
 
 	if (uds_try & TR2_DST_UDS_TRY_STREAM) {
-		e = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);
-		if (!e)
+		if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))
 			goto connected;
-		if (e != EPROTOTYPE)
+		if (errno != EPROTOTYPE)
 			goto error;
 	}
 	if (uds_try & TR2_DST_UDS_TRY_DGRAM) {
-		e = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);
-		if (!e)
+		if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))
 			goto connected;
 	}
 
 error:
 	if (tr2_dst_want_warning())
 		warning("trace2: could not connect to socket '%s' for '%s' tracing: %s",
 			path, tr2_sysenv_display_name(dst->sysenv_var),
-			strerror(e));
+			strerror(errno));
 
 	tr2_dst_trace_disable(dst);
 	return 0;


I do prefer that approach, since I think it's more idiomatic for our
code base, but for the sake of wrapping up this simple fix which has
been discussed much more than I think it deserves, I am OK with either.
:)

(I also found it interesting that the "error" goto in the caller only
has one source. I think the code would be easier to reason about if it
were inlined, but I'm happy to stop here for now).

-Peff