Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3

From: Jeff King <peff@peff.net>
Date: May 20, 2021, 13:13 UTC
Message-ID: <YKZgZZrZdKyUk9d9@coredump.intra.peff.net>
In-Reply-To: <patch-1.1-2e41e3e4cb-20210520T110357Z-avarab@gmail.com>

On Thu, May 20, 2021 at 01:05:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 13 quoted lines

> >> 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>

Show 25 quoted lines

> @@ -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