Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3
From: Jeff King <hidden>
Date: 2021-05-20 13:13:16
Possibly related (same subject, not in this thread)
- 2021-05-21 · Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3 · Jeff King <hidden>
- 2021-05-20 · Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3 · Junio C Hamano <hidden>
On Thu, May 20, 2021 at 01:05:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
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 think this alternative is more readable as well. I'll mark the topic to be "Expecting a reroll" in the what's cooking report. Thanks.Here's that re-roll, thanks both.
Thanks, this looks OK to me. There is one minor nit (introduced by me!) below, but I'm not sure if we should care or not.
The patch is entirely Jeff's at this point (from [ref]), with my amended commit message. So I added his SOB per his recent-ish ML "every patch of mine can be assumed to have my SOB" message.
So I have definitely said that, and I stand by it. But as a matter of project policy, I think we probably shouldn't consider that "enough". The point of the DSO is to make an affirmative statement about a particular patch. So probably my blanket statement should be taken more as "I will almost definitely add my signoff if you ask me". :) And of course in the case of this particular patch, it is very much: Signed-off-by: Jeff King [off-list ref]
quoted hunk ↗ jump to hunk
@@ -271,15 +271,13 @@ 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; }@@ -287,7 +285,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, 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));
We expect the value of errno to persist across tr2_dst_want_warning() and tr2_sysenv_display_name() here. The former may call getenv() and atoi(). I think that's probably fine, but if we wanted to be really paranoid, we'd have to preserve errno manually here, too. -Peff