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@xxxxxxxxxxxxxxxxxxxxxxx>), 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@xxxxxxxx>
> @@ -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
