From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_LOW, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id D306C1F5AE for ; Thu, 20 May 2021 13:13:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238683AbhETNOg (ORCPT ); Thu, 20 May 2021 09:14:36 -0400 Received: from cloud.peff.net ([104.130.231.41]:60384 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237915AbhETNOf (ORCPT ); Thu, 20 May 2021 09:14:35 -0400 Received: (qmail 27364 invoked by uid 109); 20 May 2021 13:13:10 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 20 May 2021 13:13:10 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 31392 invoked by uid 111); 20 May 2021 13:13:09 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 20 May 2021 09:13:09 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 20 May 2021 09:13:09 -0400 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: git@vger.kernel.org, Junio C Hamano , Denton Liu , Jeff Hostetler , Johannes Schindelin Subject: Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3 Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 > ), 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 > @@ -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