From: Jeff King <peff@peff.net>
Date: Wed, 13 Jul 2011 23:43:57 GMT
Subject: Re: [PATCH] Do not log unless all connect() attempts fail
Message-ID: <20110713234357.GB18273@sigill.intra.peff.net>
In-Reply-To: <CABPQNSazGsgqcZZ=q9VJ+2u=O32ePeRAjqo4+FuyVwCkX4y4nQ@mail.gmail.com>

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