| Message ID | patch-1.1-2e41e3e4cb-20210520T110357Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c7a015f99cf176f83c2c1566c230f060dccd8a0 |
| Headers | show |
| Series | [v3] trace2: refactor to avoid gcc warning under -O3 | expand |
On Thu, May 20, 2021 at 01:05:45PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> 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 > <YJrIMbr6VkYGQMfs@coredump.intra.peff.net>), 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 <peff@peff.net> > @@ -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
Jeff King <peff@peff.net> writes: >> @@ -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); }
On Fri, May 21, 2021 at 07:08:11AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> @@ -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(-) Ah, yeah. I didn't look to see if there were existing cases of the same thing. I could go either way on this kind of saved_errno thing in general (the tr2 functions called in between are really quite unlikely to set errno (I am not even sure if getenv() and atoi() can, so this really might just be future-proofing in case those tr2 functions get more complicated). But seeing that there are other cases of the same, I definitely think it is not something that should be in Ævar's patch. It is a cleanup we could do on top if we cared to. -Peff
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,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)); tr2_dst_trace_disable(dst); return 0;
Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that appears under -O3 (but not -O2). This makes the build pass under DEVELOPER=1 without needing a DEVOPTS=no-error. This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with clang 7.0.1-8+deb10u2. We've had this warning since ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). As noted in [2] this warning happens because the compiler doesn't assume that errno must be non-zero after a failed syscall. Let's work around by using the well-established "saved_errno" pattern, along with returning -1 ourselves instead of "errno". The caller can thus rely on our "errno" on failure. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related bug report against GCC. 1. trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’: trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] dst->fd = fd; ~~~~~~~~^~~~ trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here int fd; ^~ 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- On Thu, May 20 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> 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. The patch is entirely Jeff's at this point (from <YJrIMbr6VkYGQMfs@coredump.intra.peff.net>), 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. Range-diff against v2: 1: 782555daad ! 1: 2e41e3e4cb trace2: refactor to avoid gcc warning under -O3 @@ Commit message ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). As noted in [2] this warning happens because the compiler doesn't - assume that errno must be non-zero after a failed syscall. Let's work - around it as suggested in that analysis. We now return -1 ourselves on - error, and save away the value of errno in a variable the caller - passes in. + assume that errno must be non-zero after a failed syscall. + + Let's work around by using the well-established "saved_errno" pattern, + along with returning -1 ourselves instead of "errno". The caller can + thus rely on our "errno" on failure. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related bug report against GCC. @@ Commit message 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> + Signed-off-by: Jeff King <peff@peff.net> ## trace2/tr2_dst.c ## -@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value) - #define PREFIX_AF_UNIX_STREAM "af_unix:stream:" - #define PREFIX_AF_UNIX_DGRAM "af_unix:dgram:" - --static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd) -+static int tr2_dst_try_uds_connect(const char *path, int sock_type, -+ int *out_fd, int *e) - { - int fd; - struct sockaddr_un sa; +@@ trace2/tr2_dst.c: 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) + if (fd == -1) - return errno; -+ if (fd == -1) { -+ *e = 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; -+ *e = errno; ++ int saved_errno = errno; close(fd); - return e; ++ errno = saved_errno; + return -1; } *out_fd = fd; +@@ trace2/tr2_dst.c: 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; + + /* @@ trace2/tr2_dst.c: 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, &e)) ++ if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd)) goto connected; - if (e != EPROTOTYPE) +- 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, &e)) ++ if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd)) goto connected; } +@@ trace2/tr2_dst.c: 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)); + + tr2_dst_trace_disable(dst); + return 0; trace2/tr2_dst.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)