git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Denton Liu <liu.denton@gmail.com>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
Date: Tue, 11 May 2021 14:08:49 -0400	[thread overview]
Message-ID: <YJrIMbr6VkYGQMfs@coredump.intra.peff.net> (raw)
In-Reply-To: <YJrEy8IPi+PFWuzT@coredump.intra.peff.net>

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

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

  reply	other threads:[~2021-05-11 18:09 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-01  7:30 [PATCH] Fix -Wmaybe-uninitialized warnings under -O0 Denton Liu
     [not found] ` <CAPUEspgBkmxszgBee8C9hZnEwqztf-XKEj7LB_jWVFJaJCge0w@mail.gmail.com>
2020-04-01  9:05   ` Denton Liu
2020-04-01  9:52 ` Jeff King
2020-04-01 14:06   ` Denton Liu
2020-04-03 14:04     ` Jeff King
2020-04-03 14:38       ` Jeff King
2020-04-04 12:07       ` Denton Liu
2020-04-04 14:21         ` Jeff King
2021-05-05  8:40           ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
2021-05-05  9:47             ` Junio C Hamano
2021-05-05 13:34               ` Jeff King
2021-05-05 14:38             ` Johannes Schindelin
2021-05-06  1:26               ` Junio C Hamano
2021-05-06 20:29                 ` Johannes Schindelin
2021-05-06 21:10                   ` Junio C Hamano
2021-05-11 14:34                     ` Johannes Schindelin
2021-05-11 18:00                       ` Jeff King
2021-05-11 20:58                         ` Junio C Hamano
2021-05-11 21:07                           ` Jeff King
2021-05-11 21:33                             ` Junio C Hamano
2021-05-11  7:03             ` Junio C Hamano
2021-05-11 13:04             ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-05-11 16:40               ` Jeff Hostetler
2021-05-11 17:54               ` Jeff King
2021-05-11 18:08                 ` Jeff King [this message]
2021-05-11 21:09                   ` Junio C Hamano
2021-05-20  0:20                   ` Junio C Hamano
2021-05-20 11:05                     ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2021-05-20 13:13                       ` Jeff King
2021-05-20 22:08                         ` Junio C Hamano
2021-05-21  9:34                           ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJrIMbr6VkYGQMfs@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=liu.denton@gmail.com \
    --subject='Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).