[PATCH] ident.c: add support for IPv6

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 <redacted>
---
 ident.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/ident.c b/ident.c
index 5ff1aad..86b62be 100644
--- a/ident.c
+++ b/ident.c

@@ -69,6 +69,34 @@ static int add_mailname_host(struct strbuf *buf)
 	fclose(mailname);
 	return 0;
 }
+#ifndef NO_IPV6
+
+static void add_domainname(struct strbuf *out)
+{
+	char buf[1024];
+	struct addrinfo hints, *ai;
+	int gai;
+
+	if (gethostname(buf, sizeof(buf))) {
+		warning("cannot get host name: %s", strerror(errno));
+		strbuf_addstr(out, "(none)");
+		return;
+	}
+	if (strchr(buf, '.'))
+		strbuf_addstr(out, buf);
+	else	{
+		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);
+			freeaddrinfo(ai);
+		}
+		else
+			strbuf_addf(out, "%s.(none)", buf);
+	}
+}
+#else /* NO_IPV6 */
+
 
 static void add_domainname(struct strbuf *out)
 {

@@ -88,6 +116,8 @@ static void add_domainname(struct strbuf *out)
 		strbuf_addf(out, "%s.(none)", buf);
 }
 
+#endif /* NO_IPV6 */
+
 static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*

-- 
2.3.3.GIT

On 2015-10-30 15.48, Elia Pinto wrote:

Add IPv6 support by implementing name resolution with the

Minor question: How is this related to IPV6?
Could the header line be written something like

"ident.c: Use getaddrinfo() instead of gethostbyname() if available"

On which systems has the patch been tested ?
Linux ?
Mac OS X ?
Windows ?
BSD ?

The motivation on which platforms the usage of getaddrinfo() is preferred
over gethostbyname() could be helpful to motivate this patch:
System XYZ behaves bad when gethostbyname() is used.
Fix it by using getaddrinfo() instead.

A more defensive patch could call getaddrinfo() (If available, iow
when NO_IPV6 is false), and if that fails for whatever reason,
fall back to gethostbyname(), which should be available on all systems.

The scope of these variables can be narrowed, by moving them into the "{" block,
where they are needed. (Before the memset())

+
+	if (gethostname(buf, sizeof(buf))) {
+		warning("cannot get host name: %s", strerror(errno));
+		strbuf_addstr(out, "(none)");
+		return;
+	}
+	if (strchr(buf, '.'))
+		strbuf_addstr(out, buf);
+	else	{

Many ' ' between else and '{', one should be enough

+		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);
+			freeaddrinfo(ai);
+		}
+		else

Colud be written in one line as "} else"

+			strbuf_addf(out, "%s.(none)", buf);
+	}
+}
+#else /* NO_IPV6 */

On Fri, Oct 30, 2015 at 1:26 PM, Torsten Bögershausen [off-list ref] wrote:

On 2015-10-30 15.48, Elia Pinto wrote:

quoted

Add IPv6 support by implementing name resolution with the
---
+#ifndef NO_IPV6
+
+static void add_domainname(struct strbuf *out)
+{
+     char buf[1024];
+     struct addrinfo hints, *ai;
+     int gai;

The scope of these variables can be narrowed, by moving them into the "{" block,
where they are needed. (Before the memset())

quoted

+
+     if (gethostname(buf, sizeof(buf))) {
+             warning("cannot get host name: %s", strerror(errno));
+             strbuf_addstr(out, "(none)");
+             return;
+     }
+     if (strchr(buf, '.'))
+             strbuf_addstr(out, buf);
+     else    {

Many ' ' between else and '{', one should be enough

quoted

+             memset (&hints, '\0', sizeof (hints));
+             hints.ai_flags = AI_CANONNAME;
+             if (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {

Why is 'gai' needed and assigned? It's value is never consulted thereafter.

quoted

+                     strbuf_addstr(out, ai->ai_canonname);
+                     freeaddrinfo(ai);

Also, aren't you leaking 'ai' when 'ai_canonname' doesn't contain a '.'?

quoted

+             }
+             else

Colud be written in one line as "} else"

quoted

+                     strbuf_addf(out, "%s.(none)", buf);
+     }
+}
+#else /* NO_IPV6 */

On Fri, Oct 30, 2015 at 03:48:07PM +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.

Makes sense. I'm not excited by the duplication in the early part of the
function, though:

+#ifndef NO_IPV6
+
+static void add_domainname(struct strbuf *out)
+{
+	char buf[1024];
+	struct addrinfo hints, *ai;
+	int gai;
+
+	if (gethostname(buf, sizeof(buf))) {
+		warning("cannot get host name: %s", strerror(errno));
+		strbuf_addstr(out, "(none)");
+		return;
+	}
+	if (strchr(buf, '.'))
+		strbuf_addstr(out, buf);
+	else	{
+		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);
+			freeaddrinfo(ai);
+		}
+		else
+			strbuf_addf(out, "%s.(none)", buf);
+	}
+}

Especially the "(none)" stuff is ugly enough as it is, without being
duplicated in two spots. Can we factor out the else clause that calls
gethostbyname(), and just override that part with the #ifdef?

For that matter, we have a few other spots that use getaddrinfo and
#ifdef. I wonder if it would be possible to simply use getaddrinfo
everywhere, and make a compatibility wrapper that uses gethostbyname for
older systems. The cut-and-paste duplication in connect.c, for example,
is pretty egregious.

-Peff