# [PATCHv2] ident.c: add support for IPv6 2 messages from 2015-11-19 to 2015-11-19. Participants: Elia Pinto, Jeff King. Thread: https://gitlist.dev/t/40835 ## Elia Pinto, 2015-11-19 13:54 Subject: [PATCHv2] ident.c: add support for IPv6 Message-ID: <1447941260-5395-1-git-send-email-gitter.spiros@gmail.com> URL: https://gitlist.dev/e/1447941260-5395-1-git-send-email-gitter.spiros%40gmail.com ``` Add IPv6 support by implementing name resolution with the protocol agnostic getaddrinfo(3) API. The old gethostbyname(3) code is still available when git is compiled with NO_IPV6. Signed-off-by: Elia Pinto Helped-by: Jeff King Helped-by: Eric Sunshine --- This is the second version of the patch ($gmane/280488) Changes from previous: - Simplified the implementation, avoiding the duplication of the function add_domainname (Jeff King) ($gmane/280512) - Fixed a possible memory leak with getaddrinfo (Eric Sunshine) ($gmane/280507) ident.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/ident.c b/ident.c index 5ff1aad..283e83f 100644 --- a/ident.c +++ b/ident.c @@ -73,7 +73,12 @@ static int add_mailname_host(struct strbuf *buf) static void add_domainname(struct strbuf *out) { char buf[1024]; +#ifndef NO_IPV6 + struct addrinfo hints, *ai; + int gai; +#else struct hostent *he; +#endif /* NO_IPV6 */ if (gethostname(buf, sizeof(buf))) { warning("cannot get host name: %s", strerror(errno)); @@ -82,10 +87,23 @@ static void add_domainname(struct strbuf *out) } if (strchr(buf, '.')) strbuf_addstr(out, buf); - else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.')) - strbuf_addstr(out, he->h_name); - else - strbuf_addf(out, "%s.(none)", buf); + else { +#ifndef NO_IPV6 + memset (&hints, '\0', sizeof (hints)); + hints.ai_flags = AI_CANONNAME; + if (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) + strbuf_addstr(out, ai->ai_canonname); +#else + if ((he = gethostbyname(buf)) && strchr(he->h_name, '.')) + strbuf_addstr(out, he->h_name); +#endif /* NO_IPV6 */ + else + strbuf_addf(out, "%s.(none)", buf); + +#ifndef NO_IPV6 + if (gai) freeaddrinfo(ai); +#endif /* NO_IPV6 */ + } } static void copy_email(const struct passwd *pw, struct strbuf *email) -- 2.5.0 ``` ## Jeff King, 2015-11-19 14:46 Subject: Re: [PATCHv2] ident.c: add support for IPv6 Message-ID: <20151119144632.GD9353@sigill.intra.peff.net> URL: https://gitlist.dev/e/20151119144632.GD9353%40sigill.intra.peff.net In-Reply-To: <1447941260-5395-1-git-send-email-gitter.spiros@gmail.com> ``` On Thu, Nov 19, 2015 at 02:54:20PM +0100, Elia Pinto wrote: > Add IPv6 support by implementing name resolution with the > protocol agnostic getaddrinfo(3) API. The old gethostbyname(3) > code is still available when git is compiled with NO_IPV6. > > Signed-off-by: Elia Pinto > Helped-by: Jeff King > Helped-by: Eric Sunshine > --- > This is the second version of the patch ($gmane/280488) > Changes from previous: > > - Simplified the implementation, avoiding the duplication of > the function add_domainname (Jeff King) ($gmane/280512) > > - Fixed a possible memory leak with getaddrinfo (Eric Sunshine) > ($gmane/280507) Thanks, this looks much nicer than the last round. I still have a few comments: > diff --git a/ident.c b/ident.c > index 5ff1aad..283e83f 100644 > --- a/ident.c > +++ b/ident.c > @@ -73,7 +73,12 @@ static int add_mailname_host(struct strbuf *buf) > static void add_domainname(struct strbuf *out) > { > char buf[1024]; > +#ifndef NO_IPV6 > + struct addrinfo hints, *ai; > + int gai; > +#else > struct hostent *he; > +#endif /* NO_IPV6 */ Can we bump these declarations closer to the point of use? I think that makes it easier to read, as the matching bits are all together. So this: > + else { > +#ifndef NO_IPV6 > + memset (&hints, '\0', sizeof (hints)); > + hints.ai_flags = AI_CANONNAME; > + if (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) > + strbuf_addstr(out, ai->ai_canonname); > +#else > + if ((he = gethostbyname(buf)) && strchr(he->h_name, '.')) > + strbuf_addstr(out, he->h_name); > +#endif /* NO_IPV6 */ becomes: else { #ifndef NO_IPV6 struct addrinfo hints, *ai; int gai; if (!(gai = getaddrinfo()...)) #else struct hostent *he; if ((he = gethostbyname()....)) #endif > + else > + strbuf_addf(out, "%s.(none)", buf); This depends on both sides of the #if ending with a hanging "if" clause. I guess that's OK in such a small function, but I do still wonder if we can have a helper like: static void canonical_name(const char *host, struct strbuf *out) { #ifndef NO_IPV6 struct addrinfo hints, *ai; int gai = getaddrinfo(host, NULL, &hints, &ai); if (!gai && ai && strchr(ai->ai_canonname, '.')) { strbuf_addstr(out, ai->ai_canonname); freeaddrinfo(ai); } #else struct hostent *he = gethostbyname(buf); if (he && strchr(he->h_name, '.')) strbuf_addstr(out, he->h_name); } ... canonical_name(buf, out); if (!out.len) strbuf_addf(out, "%s.(none)", buf); (or possibly have an integer return value from canonical_name instead of checking the strbuf length). > +#ifndef NO_IPV6 > + if (gai) freeaddrinfo(ai); > +#endif /* NO_IPV6 */ Should this be "if (!gai)"? -Peff ```