Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 20, 2021, 22:08 UTC
- Message-ID
- <xmqqk0ntt5x0.fsf@gitster.g>
- In-Reply-To
- <YKZgZZrZdKyUk9d9@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 11 quoted lines
>> @@ -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.Being "really paranoid" consistently within the file would mean a change like the attached, I would think, on top of what was posted.
Or tr2_dst_want_warning() and tr2_sysenv_display_name() can be taught to preserve errno like tr2_dst_dry_uds_connect() was taught to do so by the patch under discussion, which may reduce the amount of apparent change, but constantly moving the contents of errno around just in case we later might want to use its value feels dirty.
I dunno.
trace2/tr2_dst.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git c/trace2/tr2_dst.c w/trace2/tr2_dst.c index 0031476350..f740a0a076 100644 --- c/trace2/tr2_dst.c +++ w/trace2/tr2_dst.c @@ -62,11 +62,13 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix) } if (fd == -1) { + int saved_errno = errno; + if (tr2_dst_want_warning()) warning("trace2: could not open '%.*s' for '%s' tracing: %s", (int) base_path_len, path.buf, tr2_sysenv_display_name(dst->sysenv_var), - strerror(errno)); + strerror(saved_errno)); tr2_dst_trace_disable(dst); strbuf_release(&path); @@ -86,6 +88,8 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value) { int fd = open(tgt_value, O_WRONLY | O_APPEND | O_CREAT, 0666); if (fd == -1) { + int saved_errno = errno; + if (tr2_dst_want_warning()) warning("trace2: could not open '%s' for '%s' tracing: %s", tgt_value, @@ -140,6 +144,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, unsigned int uds_try = 0; int fd; const char *path = NULL; + int saved_errno; /* * Allow "af_unix:[<type>:]<absolute_path>" @@ -193,10 +198,11 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, } error: + saved_errno = errno; 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(errno)); + strerror(saved_errno)); tr2_dst_trace_disable(dst); return 0; @@ -276,6 +282,7 @@ int tr2_dst_trace_want(struct tr2_dst *dst) void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line) { int fd = tr2_dst_get_trace_fd(dst); + int saved_errno; strbuf_complete_line(buf_line); /* ensure final NL on buffer */ @@ -297,9 +304,10 @@ void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line) if (write(fd, buf_line->buf, buf_line->len) >= 0) return; + saved_errno = errno; if (tr2_dst_want_warning()) warning("unable to write trace to '%s': %s", tr2_sysenv_display_name(dst->sysenv_var), - strerror(errno)); + strerror(saved_errno)); tr2_dst_trace_disable(dst); }