{"thread":{"id":"27796","subject":"[PATCH] Do not log unless all connect() attempts fail","startedAt":"2011-07-12T16:28:34Z","lastAt":"2011-07-13T23:43:57Z","messageCount":6,"participants":["Dave Zarzycki","Erik Faye-Lund","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"171155","messageId":"1EC2718A-A993-443C-8D7C-DEBD7C424EB9@apple.com","threadId":"27796","inReplyTo":null,"subject":"[PATCH] Do not log unless all connect() attempts fail","fromName":"Dave Zarzycki","fromEmail":"zarzycki@apple.com","sentAt":"2011-07-12T16:28:34Z","receivedAt":"2011-07-12T16:28:34Z","isPatch":true,"sender":{"key":"zarzycki@apple.com","avatar":"https://avatars.githubusercontent.com/u/1071982?v=4"},"body":"IPv6 hosts are often unreachable on the primarily IPv4 Internet and\ntherefore we shouldn't print an error if there are still other hosts we\ncan try to connect() to. This helps \"git fetch --quiet\" stay quiet.\n\nSigned-off-by: Dave Zarzycki <zarzycki@apple.com>\n---\n connect.c |   12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 2119c3f..8eb9f44 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -192,6 +192,7 @@ static const char *ai_name(const struct addrinfo *ai)\n  */\n static int git_tcp_connect_sock(char *host, int flags)\n {\n+\tstruct strbuf error_message = STRBUF_INIT;\n \tint sockfd = -1, saved_errno = 0;\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n \tstruct addrinfo hints, *ai0, *ai;\n@@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n \t\t}\n \t\tif (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n \t\t\tsaved_errno = errno;\n-\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n-\t\t\t\thost,\n-\t\t\t\tcnt,\n-\t\t\t\tai_name(ai),\n-\t\t\t\tstrerror(saved_errno));\n+\t\t\tstrbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n+\t\t\t\thost, cnt, ai_name(ai), strerror(saved_errno));\n \t\t\tclose(sockfd);\n \t\t\tsockfd = -1;\n \t\t\tcontinue;\n@@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n \tfreeaddrinfo(ai0);\n \n \tif (sockfd < 0)\n-\t\tdie(\"unable to connect a socket (%s)\", strerror(saved_errno));\n+\t\tdie(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n \n \tif (flags & CONNECT_VERBOSE)\n \t\tfprintf(stderr, \"done.\\n\");\n \n+\tstrbuf_release(&error_message);\n+\n \treturn sockfd;\n }\n \n-- \n1.7.6.135.g8cdba\n"},{"id":"171227","messageId":"CABPQNSaPXmHE1qECUbG9oWU43HbAXxAY42T1P=MNHgkkWM936w@mail.gmail.com","threadId":"27796","inReplyTo":"1EC2718A-A993-443C-8D7C-DEBD7C424EB9@apple.com","subject":"Re: [PATCH] Do not log unless all connect() attempts fail","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-13T09:23:33Z","receivedAt":"2011-07-13T09:23:33Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jul 12, 2011 at 6:28 PM, Dave Zarzycki <zarzycki@apple.com> wrote:\n> IPv6 hosts are often unreachable on the primarily IPv4 Internet and\n> therefore we shouldn't print an error if there are still other hosts we\n> can try to connect() to. This helps \"git fetch --quiet\" stay quiet.\n>\n> Signed-off-by: Dave Zarzycki <zarzycki@apple.com>\n> ---\n>  connect.c |   12 ++++++------\n>  1 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index 2119c3f..8eb9f44 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -192,6 +192,7 @@ static const char *ai_name(const struct addrinfo *ai)\n>  */\n>  static int git_tcp_connect_sock(char *host, int flags)\n>  {\n> +       struct strbuf error_message = STRBUF_INIT;\n>        int sockfd = -1, saved_errno = 0;\n>        const char *port = STR(DEFAULT_GIT_PORT);\n>        struct addrinfo hints, *ai0, *ai;\n> @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n>                }\n>                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n>                        saved_errno = errno;\n> -                       fprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n> -                               host,\n> -                               cnt,\n> -                               ai_name(ai),\n> -                               strerror(saved_errno));\n> +                       strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n> +                               host, cnt, ai_name(ai), strerror(saved_errno));\n>                        close(sockfd);\n>                        sockfd = -1;\n>                        continue;\n> @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n>        freeaddrinfo(ai0);\n>\n>        if (sockfd < 0)\n> -               die(\"unable to connect a socket (%s)\", strerror(saved_errno));\n> +               die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n>\n\nThis kills the output from the case where \"sockfd < 0\" evaluates to\ntrue for the last entry in ai, no (just above your second hunk), no?\nIn that case errno gets copied to saved_errno, and the old output\nwould do strerror(old_errno), but now you just print the log you've\ngathered, and don't even look at saved_errno.\n\nIf this is intentional then you should probably kill the saved_errno\nvariable also, it's rendered pointless by this patch.\n"},{"id":"171286","messageId":"7v1uxt7w5l.fsf@alter.siamese.dyndns.org","threadId":"27796","inReplyTo":"CABPQNSaPXmHE1qECUbG9oWU43HbAXxAY42T1P=MNHgkkWM936w@mail.gmail.com","subject":"Re: [PATCH] Do not log unless all connect() attempts fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-13T20:39:18Z","receivedAt":"2011-07-13T20:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n>> diff --git a/connect.c b/connect.c\n>> index 2119c3f..8eb9f44 100644\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n>>                }\n>>                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n>>                        saved_errno = errno;\n>> +                       strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n>> +                               host, cnt, ai_name(ai), strerror(saved_errno));\n>>                        close(sockfd);\n>>                        sockfd = -1;\n>>                        continue;\n>> @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n>>        freeaddrinfo(ai0);\n>>\n>>        if (sockfd < 0)\n>> -               die(\"unable to connect a socket (%s)\", strerror(saved_errno));\n>> +               die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n>>\n>\n> This kills the output from the case where \"sockfd < 0\" evaluates to\n> true for the last entry in ai, no (just above your second hunk), no?\n> In that case errno gets copied to saved_errno, and the old output\n> would do strerror(old_errno), but now you just print the log you've\n> gathered, and don't even look at saved_errno.\n>\n> If this is intentional then you should probably kill the saved_errno\n> variable also, it's rendered pointless by this patch.\n\nAs error_message.buf contains strerror([saved_]errno), I think it is\nintentional to remove strerror() from the final die().  Without looking at\nthe code outside the context of the patch I cannot tell if saved_errno has\nbecome unneeded, but I tend to trust your analysis so...\n"},{"id":"171289","messageId":"20110713210636.GF31965@sigill.intra.peff.net","threadId":"27796","inReplyTo":"CABPQNSaPXmHE1qECUbG9oWU43HbAXxAY42T1P=MNHgkkWM936w@mail.gmail.com","subject":"Re: [PATCH] Do not log unless all connect() attempts fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-13T21:06:36Z","receivedAt":"2011-07-13T21:06:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:\n\n> >  static int git_tcp_connect_sock(char *host, int flags)\n> >  {\n> > +       struct strbuf error_message = STRBUF_INIT;\n> >        int sockfd = -1, saved_errno = 0;\n> >        const char *port = STR(DEFAULT_GIT_PORT);\n> >        struct addrinfo hints, *ai0, *ai;\n> > @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n> >                }\n> >                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n> >                        saved_errno = errno;\n> > -                       fprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n> > -                               host,\n> > -                               cnt,\n> > -                               ai_name(ai),\n> > -                               strerror(saved_errno));\n> > +                       strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n> > +                               host, cnt, ai_name(ai), strerror(saved_errno));\n> >                        close(sockfd);\n> >                        sockfd = -1;\n> >                        continue;\n> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n> >        freeaddrinfo(ai0);\n> >\n> >        if (sockfd < 0)\n> > -               die(\"unable to connect a socket (%s)\", strerror(saved_errno));\n> > +               die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n> >\n> \n> This kills the output from the case where \"sockfd < 0\" evaluates to\n> true for the last entry in ai, no (just above your second hunk), no?\n> In that case errno gets copied to saved_errno, and the old output\n> would do strerror(old_errno), but now you just print the log you've\n> gathered, and don't even look at saved_errno.\n\nBut that's OK, because the value of that saved_errno is in the gathered\nlog, isn't it? So the output is not identical, but IMHO it's much\nbetter. It's gone from:\n\n  $ git fetch git://example.com/foo\n  example.com[0: 192.0.32.10]: errno=Connection timed out\n  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable\n  fatal: unable to connect a socket (Network is unreachable)\n\nto:\n\n  $ git fetch git://example.com/foo\n  fatal: unable to connect to example.com:\n  example.com[0: 192.0.32.10]: errno=Connection timed out\n  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable\n\nIMHO, the \"Network is unreachable\" at the end of the first one is just\nnoise; it's a duplicate of what was already printed, and it may not be\nthe errno value that is most interesting to you.\n\n> If this is intentional then you should probably kill the saved_errno\n> variable also, it's rendered pointless by this patch.\n\nThat is sensible, though, I think.\n\n-Peff\n"},{"id":"171296","messageId":"CABPQNSazGsgqcZZ=q9VJ+2u=O32ePeRAjqo4+FuyVwCkX4y4nQ@mail.gmail.com","threadId":"27796","inReplyTo":"20110713210636.GF31965@sigill.intra.peff.net","subject":"Re: [PATCH] Do not log unless all connect() attempts fail","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-13T22:29:11Z","receivedAt":"2011-07-13T22:29:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Jul 13, 2011 at 11:06 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:\n>\n>> >  static int git_tcp_connect_sock(char *host, int flags)\n>> >  {\n>> > +       struct strbuf error_message = STRBUF_INIT;\n>> >        int sockfd = -1, saved_errno = 0;\n>> >        const char *port = STR(DEFAULT_GIT_PORT);\n>> >        struct addrinfo hints, *ai0, *ai;\n>> > @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n>> >                }\n>> >                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n>> >                        saved_errno = errno;\n>> > -                       fprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n>> > -                               host,\n>> > -                               cnt,\n>> > -                               ai_name(ai),\n>> > -                               strerror(saved_errno));\n>> > +                       strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n>> > +                               host, cnt, ai_name(ai), strerror(saved_errno));\n>> >                        close(sockfd);\n>> >                        sockfd = -1;\n>> >                        continue;\n>> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n>> >        freeaddrinfo(ai0);\n>> >\n>> >        if (sockfd < 0)\n>> > -               die(\"unable to connect a socket (%s)\", strerror(saved_errno));\n>> > +               die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n>> >\n>>\n>> This kills the output from the case where \"sockfd < 0\" evaluates to\n>> true for the last entry in ai, no (just above your second hunk), no?\n>> In that case errno gets copied to saved_errno, and the old output\n>> would do strerror(old_errno), but now you just print the log you've\n>> gathered, and don't even look at saved_errno.\n>\n> But that's OK, because the value of that saved_errno is in the gathered\n> log, isn't it?\n\nNo, it's not. In the case where socket fails, we assign errno to\nsaved_errno and _continue_. So nothing gets logged about the error. If\nthere's only one entry in the address list, we don't end up reporting\nanything; the strbuf is empty. We used to at least report\nstrerror(errno) in that case.\n"},{"id":"171301","messageId":"20110713234357.GB18273@sigill.intra.peff.net","threadId":"27796","inReplyTo":"CABPQNSazGsgqcZZ=q9VJ+2u=O32ePeRAjqo4+FuyVwCkX4y4nQ@mail.gmail.com","subject":"Re: [PATCH] Do not log unless all connect() attempts fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-13T23:43:57Z","receivedAt":"2011-07-13T23:43:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2011 at 12:29:11AM +0200, Erik Faye-Lund wrote:\n\n> On Wed, Jul 13, 2011 at 11:06 PM, Jeff King <peff@peff.net> wrote:\n> > On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:\n> >\n> >> >  static int git_tcp_connect_sock(char *host, int flags)\n> >> >  {\n> >> > +       struct strbuf error_message = STRBUF_INIT;\n> >> >        int sockfd = -1, saved_errno = 0;\n> >> >        const char *port = STR(DEFAULT_GIT_PORT);\n> >> >        struct addrinfo hints, *ai0, *ai;\n> >> > @@ -225,11 +226,8 @@ static int git_tcp_connect_sock(char *host, int flags)\n> >> >                }\n> >> >                if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n> >> >                        saved_errno = errno;\n> >> > -                       fprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n> >> > -                               host,\n> >> > -                               cnt,\n> >> > -                               ai_name(ai),\n> >> > -                               strerror(saved_errno));\n> >> > +                       strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n> >> > +                               host, cnt, ai_name(ai), strerror(saved_errno));\n> >> >                        close(sockfd);\n> >> >                        sockfd = -1;\n> >> >                        continue;\n> >> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n> >> >        freeaddrinfo(ai0);\n> >> >\n> >> >        if (sockfd < 0)\n> >> > -               die(\"unable to connect a socket (%s)\", strerror(saved_errno));\n> >> > +               die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n> >> >\n> >>\n> >> This kills the output from the case where \"sockfd < 0\" evaluates to\n> >> true for the last entry in ai, no (just above your second hunk), no?\n> >> In that case errno gets copied to saved_errno, and the old output\n> >> would do strerror(old_errno), but now you just print the log you've\n> >> gathered, and don't even look at saved_errno.\n> >\n> > But that's OK, because the value of that saved_errno is in the gathered\n> > log, isn't it?\n> \n> No, it's not. In the case where socket fails, we assign errno to\n> saved_errno and _continue_. So nothing gets logged about the error. If\n> there's only one entry in the address list, we don't end up reporting\n> anything; the strbuf is empty. We used to at least report\n> strerror(errno) in that case.\n\nAh, sorry. I was reading the patch, not the actual function, and the bit\nyou are talking about was not in the context. Yes, you are right. Sorry\nfor the noise.\n\n-Peff\n"}]}