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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]