Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
From: Junio C Hamano <hidden>
Date: 2021-05-11 21:09:35
Jeff King [off-list ref] writes:
quoted hunk
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. :)
Yeah, the above looks nicer to me too.
(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