threads / patch / 27796

patchDo not log unless all connect() attempts fail

Subject: [PATCH] Do not log unless all connect() attempts fail

## tl;dr

6 messages between Jul 12, 2011 and Jul 13, 2011. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Dave Zarzycki· Jul 12, 2011, 16:28 UTC · lore

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.

Signed-off-by: Dave Zarzycki <zarzycki@apple.com>
---
 connect.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

Show changes to connect.c +6 −6

diff --git a/connect.c b/connect.c
index 2119c3f..8eb9f44 100644
--- a/connect.c
+++ b/connect.c
@@ -192,6 +192,7 @@ static const char *ai_name(const struct addrinfo *ai)
  */
 static int git_tcp_connect_sock(char *host, int flags)
 {
+	struct strbuf error_message = STRBUF_INIT;
 	int sockfd = -1, saved_errno = 0;
 	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
@@ -225,11 +226,8 @@ 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));
+			strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
+				host, cnt, ai_name(ai), strerror(saved_errno));
 			close(sockfd);
 			sockfd = -1;
 			continue;
@@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
 	freeaddrinfo(ai0);
 
 	if (sockfd < 0)
-		die("unable to connect a socket (%s)", strerror(saved_errno));
+		die("unable to connect to %s:\n%s", host, error_message.buf);
 
 	if (flags & CONNECT_VERBOSE)
 		fprintf(stderr, "done.\n");
 
+	strbuf_release(&error_message);
+
 	return sockfd;
 }

-- 
1.7.6.135.g8cdba

Erik Faye-Lund· Jul 13, 2011, 09:23 UTC · re: Dave Zarzycki · lore

Re: [PATCH] Do not log unless all connect() attempts fail

On Tue, Jul 12, 2011 at 6:28 PM, Dave Zarzycki <zarzycki@apple.com> wrote:

Show 42 quoted lines

> 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.
>
> Signed-off-by: Dave Zarzycki <zarzycki@apple.com>
> ---
>  connect.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 2119c3f..8eb9f44 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -192,6 +192,7 @@ static const char *ai_name(const struct addrinfo *ai)
>  */
>  static int git_tcp_connect_sock(char *host, int flags)
>  {
> +       struct strbuf error_message = STRBUF_INIT;
>        int sockfd = -1, saved_errno = 0;
>        const char *port = STR(DEFAULT_GIT_PORT);
>        struct addrinfo hints, *ai0, *ai;
> @@ -225,11 +226,8 @@ 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));
> +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
> +                               host, cnt, ai_name(ai), strerror(saved_errno));
>                        close(sockfd);
>                        sockfd = -1;
>                        continue;
> @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>        freeaddrinfo(ai0);
>
>        if (sockfd < 0)
> -               die("unable to connect a socket (%s)", strerror(saved_errno));
> +               die("unable to connect to %s:\n%s", host, error_message.buf);
>

This kills the output from the case where "sockfd < 0" evaluates to true for the last entry in ai, no (just above your second hunk), no? In that case errno gets copied to saved_errno, and the old output would do strerror(old_errno), but now you just print the log you've gathered, and don't even look at saved_errno.

If this is intentional then you should probably kill the saved_errno variable also, it's rendered pointless by this patch.

Junio C Hamano· Jul 13, 2011, 20:39 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] Do not log unless all connect() attempts fail

Erik Faye-Lund <kusmabite@gmail.com> writes:

Show 29 quoted lines

>> diff --git a/connect.c b/connect.c
>> index 2119c3f..8eb9f44 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)
>>                }
>>                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
>>                        saved_errno = errno;
>> +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
>> +                               host, cnt, ai_name(ai), strerror(saved_errno));
>>                        close(sockfd);
>>                        sockfd = -1;
>>                        continue;
>> @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>>        freeaddrinfo(ai0);
>>
>>        if (sockfd < 0)
>> -               die("unable to connect a socket (%s)", strerror(saved_errno));
>> +               die("unable to connect to %s:\n%s", host, error_message.buf);
>>
>
> This kills the output from the case where "sockfd < 0" evaluates to
> true for the last entry in ai, no (just above your second hunk), no?
> In that case errno gets copied to saved_errno, and the old output
> would do strerror(old_errno), but now you just print the log you've
> gathered, and don't even look at saved_errno.
>
> If this is intentional then you should probably kill the saved_errno
> variable also, it's rendered pointless by this patch.

As error_message.buf contains strerror([saved_]errno), I think it is intentional to remove strerror() from the final die(). Without looking at the code outside the context of the patch I cannot tell if saved_errno has become unneeded, but I tend to trust your analysis so...

Jeff King· Jul 13, 2011, 21:06 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] Do not log unless all connect() attempts fail

On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:

Show 33 quoted lines

> >  static int git_tcp_connect_sock(char *host, int flags)
> >  {
> > +       struct strbuf error_message = STRBUF_INIT;
> >        int sockfd = -1, saved_errno = 0;
> >        const char *port = STR(DEFAULT_GIT_PORT);
> >        struct addrinfo hints, *ai0, *ai;
> > @@ -225,11 +226,8 @@ 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));
> > +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
> > +                               host, cnt, ai_name(ai), strerror(saved_errno));
> >                        close(sockfd);
> >                        sockfd = -1;
> >                        continue;
> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
> >        freeaddrinfo(ai0);
> >
> >        if (sockfd < 0)
> > -               die("unable to connect a socket (%s)", strerror(saved_errno));
> > +               die("unable to connect to %s:\n%s", host, error_message.buf);
> >
> 
> This kills the output from the case where "sockfd < 0" evaluates to
> true for the last entry in ai, no (just above your second hunk), no?
> In that case errno gets copied to saved_errno, and the old output
> would do strerror(old_errno), but now you just print the log you've
> gathered, and don't even look at saved_errno.

But that's OK, because the value of that saved_errno is in the gathered log, isn't it? So the output is not identical, but IMHO it's much better. It's gone from:

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

to:

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

IMHO, the "Network is unreachable" at the end of the first one is just noise; it's a duplicate of what was already printed, and it may not be the errno value that is most interesting to you.

> If this is intentional then you should probably kill the saved_errno
> variable also, it's rendered pointless by this patch.

That is sensible, though, I think.

-Peff

Erik Faye-Lund· Jul 13, 2011, 22:29 UTC · re: Jeff King · lore

Re: [PATCH] Do not log unless all connect() attempts fail

On Wed, Jul 13, 2011 at 11:06 PM, Jeff King <peff@peff.net> wrote:

Show 38 quoted lines

> On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:
>
>> >  static int git_tcp_connect_sock(char *host, int flags)
>> >  {
>> > +       struct strbuf error_message = STRBUF_INIT;
>> >        int sockfd = -1, saved_errno = 0;
>> >        const char *port = STR(DEFAULT_GIT_PORT);
>> >        struct addrinfo hints, *ai0, *ai;
>> > @@ -225,11 +226,8 @@ 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));
>> > +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
>> > +                               host, cnt, ai_name(ai), strerror(saved_errno));
>> >                        close(sockfd);
>> >                        sockfd = -1;
>> >                        continue;
>> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>> >        freeaddrinfo(ai0);
>> >
>> >        if (sockfd < 0)
>> > -               die("unable to connect a socket (%s)", strerror(saved_errno));
>> > +               die("unable to connect to %s:\n%s", host, error_message.buf);
>> >
>>
>> This kills the output from the case where "sockfd < 0" evaluates to
>> true for the last entry in ai, no (just above your second hunk), no?
>> In that case errno gets copied to saved_errno, and the old output
>> would do strerror(old_errno), but now you just print the log you've
>> gathered, and don't even look at saved_errno.
>
> But that's OK, because the value of that saved_errno is in the gathered
> log, isn't it?

No, it's not. In the case where socket fails, we assign errno to saved_errno and _continue_. So nothing gets logged about the error. If there's only one entry in the address list, we don't end up reporting anything; the strbuf is empty. We used to at least report strerror(errno) in that case.

Jeff King· Jul 13, 2011, 23:43 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] Do not log unless all connect() attempts fail

On Thu, Jul 14, 2011 at 12:29:11AM +0200, Erik Faye-Lund wrote:

Show 45 quoted lines

> On Wed, Jul 13, 2011 at 11:06 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:
> >
> >> >  static int git_tcp_connect_sock(char *host, int flags)
> >> >  {
> >> > +       struct strbuf error_message = STRBUF_INIT;
> >> >        int sockfd = -1, saved_errno = 0;
> >> >        const char *port = STR(DEFAULT_GIT_PORT);
> >> >        struct addrinfo hints, *ai0, *ai;
> >> > @@ -225,11 +226,8 @@ 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));
> >> > +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
> >> > +                               host, cnt, ai_name(ai), strerror(saved_errno));
> >> >                        close(sockfd);
> >> >                        sockfd = -1;
> >> >                        continue;
> >> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
> >> >        freeaddrinfo(ai0);
> >> >
> >> >        if (sockfd < 0)
> >> > -               die("unable to connect a socket (%s)", strerror(saved_errno));
> >> > +               die("unable to connect to %s:\n%s", host, error_message.buf);
> >> >
> >>
> >> This kills the output from the case where "sockfd < 0" evaluates to
> >> true for the last entry in ai, no (just above your second hunk), no?
> >> In that case errno gets copied to saved_errno, and the old output
> >> would do strerror(old_errno), but now you just print the log you've
> >> gathered, and don't even look at saved_errno.
> >
> > But that's OK, because the value of that saved_errno is in the gathered
> > log, isn't it?
> 
> No, it's not. In the case where socket fails, we assign errno to
> saved_errno and _continue_. So nothing gets logged about the error. If
> there's only one entry in the address list, we don't end up reporting
> anything; the strbuf is empty. We used to at least report
> strerror(errno) in that case.

Ah, sorry. I was reading the patch, not the actual function, and the bit you are talking about was not in the context. Yes, you are right. Sorry for the noise.

-Peff