[PATCH] Only print an error for the last connect() failure

[PATCH] Only print an error for the last connect() failure

[Dave Zarzycki]

IPv6 hosts are often unreachable on the primarily IPv4 Internet and
therefore we shouldn't print an error if there are still other hosts we
can try to connect() to. This helps "git fetch --quiet" stay quiet.
---
 connect.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/connect.c b/connect.c
index 2119c3f..7f70ce7 100644
--- a/connect.c
+++ b/connect.c
@@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)
 		}
 		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			saved_errno = errno;
-			fprintf(stderr, "%s[%d: %s]: errno=%s\n",
-				host,
-				cnt,
-				ai_name(ai),
-				strerror(saved_errno));
+			if (ai->ai_next == NULL) {
+				fprintf(stderr, "%s[%d: %s]: errno=%s\n",
+					host,
+					cnt,
+					ai_name(ai),
+					strerror(saved_errno));
+			}
 			close(sockfd);
 			sockfd = -1;
 			continue;
-- 
1.7.6.rc1.1.g2e27

Re: [PATCH] Only print an error for the last connect() failure

[Jeff King]

On Thu, Jun 09, 2011 at 09:18:10AM -0700, Dave Zarzycki wrote:

> IPv6 hosts are often unreachable on the primarily IPv4 Internet and
> therefore we shouldn't print an error if there are still other hosts we
> can try to connect() to. This helps "git fetch --quiet" stay quiet.
> ---
>  connect.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 2119c3f..7f70ce7 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>  		}
>  		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
>  			saved_errno = errno;
> -			fprintf(stderr, "%s[%d: %s]: errno=%s\n",
> -				host,
> -				cnt,
> -				ai_name(ai),
> -				strerror(saved_errno));
> +			if (ai->ai_next == NULL) {
> +				fprintf(stderr, "%s[%d: %s]: errno=%s\n",
> +					host,
> +					cnt,
> +					ai_name(ai),
> +					strerror(saved_errno));
> +			}

I agree being noisy about early failures when we succeed later is a bad
thing. But when we fail completely, doesn't your code now mask early
failures and print only the final one? The early failures might be the
important ones for the user.

So perhaps we should do something instead like:

  struct strbuf error_message = STRBUF_INIT;
  ...
  if (connect(...) < 0) {
          strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
                      host, cnt, ai_name(ai), strerror(errno));
          ...
  }

  if (sockfd < 0)
          die("unable to connect to %s:\n%s", host, error_message.buf);
  strbuf_release(&error_message);

-Peff

Re: [PATCH] Only print an error for the last connect() failure

[Dave Zarzycki]

On Jun 9, 2011, at 9:33 AM, Jeff King wrote:

> On Thu, Jun 09, 2011 at 09:18:10AM -0700, Dave Zarzycki wrote:
> 
>> IPv6 hosts are often unreachable on the primarily IPv4 Internet and
>> therefore we shouldn't print an error if there are still other hosts we
>> can try to connect() to. This helps "git fetch --quiet" stay quiet.
>> ---
>> connect.c |   12 +++++++-----
>> 1 files changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/connect.c b/connect.c
>> index 2119c3f..7f70ce7 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>> 		}
>> 		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
>> 			saved_errno = errno;
>> -			fprintf(stderr, "%s[%d: %s]: errno=%s\n",
>> -				host,
>> -				cnt,
>> -				ai_name(ai),
>> -				strerror(saved_errno));
>> +			if (ai->ai_next == NULL) {
>> +				fprintf(stderr, "%s[%d: %s]: errno=%s\n",
>> +					host,
>> +					cnt,
>> +					ai_name(ai),
>> +					strerror(saved_errno));
>> +			}
> 
> I agree being noisy about early failures when we succeed later is a bad
> thing. But when we fail completely, doesn't your code now mask early
> failures and print only the final one? The early failures might be the
> important ones for the user.
> 
> So perhaps we should do something instead like:
> 
>  struct strbuf error_message = STRBUF_INIT;
>  ...
>  if (connect(...) < 0) {
>          strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
>                      host, cnt, ai_name(ai), strerror(errno));
>          ...
>  }
> 
>  if (sockfd < 0)
>          die("unable to connect to %s:\n%s", host, error_message.buf);
>  strbuf_release(&error_message);

I'm happy to make that change.

Personally speaking, I don't think we're masking failures any more than git is masking failures when it doesn't find a ref in .git/refs and it falls back to .git/packed-refs. This fallback is by design, and the same is true of any classic loop around getaddrinfo(). Of course, reasonable people may disagree about what the "right" thing to do here is. :-)

In any case, I'll update the patch later today.

davez

Re: [PATCH] Only print an error for the last connect() failure

[Jeff King]

On Thu, Jun 09, 2011 at 09:49:28AM -0700, Dave Zarzycki wrote:

> I'm happy to make that change.
> 
> Personally speaking, I don't think we're masking failures any more
> than git is masking failures when it doesn't find a ref in .git/refs
> and it falls back to .git/packed-refs. This fallback is by design, and
> the same is true of any classic loop around getaddrinfo(). Of course,
> reasonable people may disagree about what the "right" thing to do here
> is. :-)

When the fallback actually _works_, then yes, there's no reason to say
anything. But when the fallback fails, it can be useful to get
information on each of the steps. In your refs analogy, it would just be
"I couldn't look up this ref; I tried .git/refs and packed refs". Which
would be the same every time, so it's not really interesting enough to
print.

But in the case of host lookup (especially with ipv6), it may be very
useful to say "I tried this address, and it failed for this reason; then
I tried this address, and it failed for this reason". If I see:

  $ git fetch git://example.com/foo
  example.com[0: 192.0.32.10]: errno=Connection timed out
  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable
  fatal: unable to connect a socket (Network is unreachable)

then I don't care about the second error. Of course the IPv6 network is
unreachable; I don't have IPv6 connectivity. The first line contains the
useful information. But with your patch, it won't be shown at all.

We should perhaps also always print intermediate error messages as we
get them in verbose mode. Even if we succeed, seeing connection timeouts
on earlier addresses can explain long pauses before success (but I agree
they shouldn't be shown by default).

-Peff

Re: [PATCH] Only print an error for the last connect() failure

[Junio C Hamano]

Dave Zarzycki <zarzycki@apple.com> writes:

> Personally speaking, I don't think we're masking failures any more than
> git is masking failures when it doesn't find a ref in .git/refs and it
> falls back to .git/packed-refs.

That is not even a fallback. If a loose ref is found, the corresponding
entry in packed-refs is *STALE* and should *NEVER* be looked at.

Back to the topic.

If a host advertises that it can be reached at any of these addresses,
then you can say these addresses are fall-back for each other.  It is fine
not to report about addresses we tried but didn't get connection as long
as we manage to reach it in one of the addresses.  We do not report other
addresses we didn't even try, and it is not useful to report earlier
addresses we tried but didn't get the dial tone in such a case.

As Peff said, if we failed to reach any connection, reporting all
failed addresses would give the user a better clue when diagnosing the
network problem.