* [PATCH] Do not log unless all connect() attempts fail
@ 2011-06-09 17:52 Dave Zarzycki
0 siblings, 0 replies; 7+ messages in thread
From: Dave Zarzycki @ 2011-06-09 17:52 UTC (permalink / raw)
To: git
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, 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);
if (flags & CONNECT_VERBOSE)
fprintf(stderr, "done.\n");
+ strbuf_release(&error_message);
+
return sockfd;
}
--
1.7.6.rc1.1.g2e27
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] Do not log unless all connect() attempts fail
@ 2011-07-12 16:28 Dave Zarzycki
2011-07-13 9:23 ` Erik Faye-Lund
0 siblings, 1 reply; 7+ messages in thread
From: Dave Zarzycki @ 2011-07-12 16:28 UTC (permalink / raw)
To: git
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);
if (flags & CONNECT_VERBOSE)
fprintf(stderr, "done.\n");
+ strbuf_release(&error_message);
+
return sockfd;
}
--
1.7.6.135.g8cdba
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not log unless all connect() attempts fail
2011-07-12 16:28 [PATCH] Do not log unless all connect() attempts fail Dave Zarzycki
@ 2011-07-13 9:23 ` Erik Faye-Lund
2011-07-13 20:39 ` Junio C Hamano
2011-07-13 21:06 ` Jeff King
0 siblings, 2 replies; 7+ messages in thread
From: Erik Faye-Lund @ 2011-07-13 9:23 UTC (permalink / raw)
To: Dave Zarzycki; +Cc: git
On Tue, Jul 12, 2011 at 6:28 PM, Dave Zarzycki <zarzycki@apple.com> 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.
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not log unless all connect() attempts fail
2011-07-13 9:23 ` Erik Faye-Lund
@ 2011-07-13 20:39 ` Junio C Hamano
2011-07-13 21:06 ` Jeff King
1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2011-07-13 20:39 UTC (permalink / raw)
To: kusmabite; +Cc: Dave Zarzycki, git
Erik Faye-Lund <kusmabite@gmail.com> writes:
>> 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...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not log unless all connect() attempts fail
2011-07-13 9:23 ` Erik Faye-Lund
2011-07-13 20:39 ` Junio C Hamano
@ 2011-07-13 21:06 ` Jeff King
2011-07-13 22:29 ` Erik Faye-Lund
1 sibling, 1 reply; 7+ messages in thread
From: Jeff King @ 2011-07-13 21:06 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: Dave Zarzycki, git
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? 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not log unless all connect() attempts fail
2011-07-13 21:06 ` Jeff King
@ 2011-07-13 22:29 ` Erik Faye-Lund
2011-07-13 23:43 ` Jeff King
0 siblings, 1 reply; 7+ messages in thread
From: Erik Faye-Lund @ 2011-07-13 22:29 UTC (permalink / raw)
To: Jeff King; +Cc: Dave Zarzycki, git
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not log unless all connect() attempts fail
2011-07-13 22:29 ` Erik Faye-Lund
@ 2011-07-13 23:43 ` Jeff King
0 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2011-07-13 23:43 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: Dave Zarzycki, git
On Thu, Jul 14, 2011 at 12:29:11AM +0200, Erik Faye-Lund wrote:
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-07-13 23:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-12 16:28 [PATCH] Do not log unless all connect() attempts fail Dave Zarzycki
2011-07-13 9:23 ` Erik Faye-Lund
2011-07-13 20:39 ` Junio C Hamano
2011-07-13 21:06 ` Jeff King
2011-07-13 22:29 ` Erik Faye-Lund
2011-07-13 23:43 ` Jeff King
-- strict thread matches above, loose matches on Subject: below --
2011-06-09 17:52 Dave Zarzycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox