{"thread":{"id":"40677","subject":"[PATCH] ident.c: add support for IPv6","startedAt":"2015-10-30T14:48:07Z","lastAt":"2015-10-30T18:29:39Z","messageCount":5,"participants":["Elia Pinto","Torsten Bögershausen","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272527","messageId":"1446216487-11503-1-git-send-email-gitter.spiros@gmail.com","threadId":"40677","inReplyTo":null,"subject":"[PATCH] ident.c: add support for IPv6","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2015-10-30T14:48:07Z","receivedAt":"2015-10-30T14:48:07Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Add IPv6 support by implementing name resolution with the\nprotocol agnostic getaddrinfo(3) API. The old gethostbyname(3)\ncode is still available when git is compiled with NO_IPV6.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n ident.c | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/ident.c b/ident.c\nindex 5ff1aad..86b62be 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -69,6 +69,34 @@ static int add_mailname_host(struct strbuf *buf)\n \tfclose(mailname);\n \treturn 0;\n }\n+#ifndef NO_IPV6\n+\n+static void add_domainname(struct strbuf *out)\n+{\n+\tchar buf[1024];\n+\tstruct addrinfo hints, *ai;\n+\tint gai;\n+\n+\tif (gethostname(buf, sizeof(buf))) {\n+\t\twarning(\"cannot get host name: %s\", strerror(errno));\n+\t\tstrbuf_addstr(out, \"(none)\");\n+\t\treturn;\n+\t}\n+\tif (strchr(buf, '.'))\n+\t\tstrbuf_addstr(out, buf);\n+\telse\t{\n+\t\tmemset (&hints, '\\0', sizeof (hints));\n+\t\thints.ai_flags = AI_CANONNAME;\n+\t\tif (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {\n+\t\t\tstrbuf_addstr(out, ai->ai_canonname);\n+\t\t\tfreeaddrinfo(ai);\n+\t\t}\n+\t\telse\n+\t\t\tstrbuf_addf(out, \"%s.(none)\", buf);\n+\t}\n+}\n+#else /* NO_IPV6 */\n+\n \n static void add_domainname(struct strbuf *out)\n {\n@@ -88,6 +116,8 @@ static void add_domainname(struct strbuf *out)\n \t\tstrbuf_addf(out, \"%s.(none)\", buf);\n }\n \n+#endif /* NO_IPV6 */\n+\n static void copy_email(const struct passwd *pw, struct strbuf *email)\n {\n \t/*\n-- \n2.3.3.GIT\n"},{"id":"272535","messageId":"5633A838.3070801@web.de","threadId":"40677","inReplyTo":"1446216487-11503-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] ident.c: add support for IPv6","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-10-30T17:26:16Z","receivedAt":"2015-10-30T17:26:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-10-30 15.48, Elia Pinto wrote:\n> Add IPv6 support by implementing name resolution with the\nMinor question: How is this related to IPV6?\nCould the header line be written something like\n\n\"ident.c: Use getaddrinfo() instead of gethostbyname() if available\"\n\nOn which systems has the patch been tested ?\nLinux ?\nMac OS X ?\nWindows ?\nBSD ?\n\nThe motivation on which platforms the usage of getaddrinfo() is preferred\nover gethostbyname() could be helpful to motivate this patch:\nSystem XYZ behaves bad when gethostbyname() is used.\nFix it by using getaddrinfo() instead.\n\nA more defensive patch could call getaddrinfo() (If available, iow\nwhen NO_IPV6 is false), and if that fails for whatever reason,\nfall back to gethostbyname(), which should be available on all systems.\n\n\n> protocol agnostic getaddrinfo(3) API. The old gethostbyname(3)\n> code is still available when git is compiled with NO_IPV6.\n> \n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n>  ident.c | 30 ++++++++++++++++++++++++++++++\n>  1 file changed, 30 insertions(+)\n> \n> diff --git a/ident.c b/ident.c\n> index 5ff1aad..86b62be 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -69,6 +69,34 @@ static int add_mailname_host(struct strbuf *buf)\n>  \tfclose(mailname);\n>  \treturn 0;\n>  }\n> +#ifndef NO_IPV6\n> +\n> +static void add_domainname(struct strbuf *out)\n> +{\n> +\tchar buf[1024];\n> +\tstruct addrinfo hints, *ai;\n> +\tint gai;\nThe scope of these variables can be narrowed, by moving them into the \"{\" block,\nwhere they are needed. (Before the memset())\n> +\n> +\tif (gethostname(buf, sizeof(buf))) {\n> +\t\twarning(\"cannot get host name: %s\", strerror(errno));\n> +\t\tstrbuf_addstr(out, \"(none)\");\n> +\t\treturn;\n> +\t}\n> +\tif (strchr(buf, '.'))\n> +\t\tstrbuf_addstr(out, buf);\n> +\telse\t{\nMany ' ' between else and '{', one should be enough\n> +\t\tmemset (&hints, '\\0', sizeof (hints));\n> +\t\thints.ai_flags = AI_CANONNAME;\n> +\t\tif (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {\n> +\t\t\tstrbuf_addstr(out, ai->ai_canonname);\n> +\t\t\tfreeaddrinfo(ai);\n> +\t\t}\n> +\t\telse\nColud be written in one line as \"} else\"\n> +\t\t\tstrbuf_addf(out, \"%s.(none)\", buf);\n> +\t}\n> +}\n> +#else /* NO_IPV6 */\n"},{"id":"272542","messageId":"xmqqtwp8kyxc.fsf@gitster.mtv.corp.google.com","threadId":"40677","inReplyTo":"5633A838.3070801@web.de","subject":"Re: [PATCH] ident.c: add support for IPv6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T18:13:03Z","receivedAt":"2015-10-30T18:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On 2015-10-30 15.48, Elia Pinto wrote:\n>> Add IPv6 support by implementing name resolution with the\n> Minor question: How is this related to IPV6?\n> Could the header line be written something like\n>\n> \"ident.c: Use getaddrinfo() instead of gethostbyname() if available\"\n>\n> On which systems has the patch been tested ?\n> Linux ?\n> Mac OS X ?\n> Windows ?\n> BSD ?\n>\n> The motivation on which platforms the usage of getaddrinfo() is preferred\n> over gethostbyname() could be helpful to motivate this patch:\n> System XYZ behaves bad when gethostbyname() is used.\n> Fix it by using getaddrinfo() instead.\n\ngethostbyname() fills a hostent that gives us the official name,\nlist of aliases, _one_ addrtype (either AF_INET or AF_INET6), and\nlist of addresses, so if we were asking for the physical addresses,\nwe may not be able to obtain all addresses for a host with both IPv4\nand IPv6 addresses, which may be an issue.\n\nBut this function is about learning the official name of the host,\ngiven the result of gethostname().  In that context, does that\n\"limited to single address family\" issue of gethostbyname() still\nmatter?  I am guessing it doesn't, and I somehow doubt that the\nvalue of this patch is about working around any platform bug.\n\nI think the real reason to favour getaddrinfo() over gethostbyname()\nis that the family of functions the latter belongs to is obsolete.\nIn other codepaths where we need to learn the inet address, we\nalready use getaddrinfo() if available (i.e. NO_IPV6 is not set),\nand gethostbyname() is used only when compiling with NO_IPV6.\n\nConverting this codepath to match that pattern incidentally allows\nyou to work around a platform bugs like \"gethostbyname() is broken\non sysmte XYZ\" (which you work around by not saying NO_IPV6), but I\nview it as a side effect.\n\n>> +static void add_domainname(struct strbuf *out)\n>> +{\n>> +\tchar buf[1024];\n>> +\tstruct addrinfo hints, *ai;\n>> +\tint gai;\n> The scope of these variables can be narrowed, by moving them into the \"{\" block,\n> where they are needed. (Before the memset())\n>> +\n>> +\tif (gethostname(buf, sizeof(buf))) {\n>> +\t\twarning(\"cannot get host name: %s\", strerror(errno));\n>> +\t\tstrbuf_addstr(out, \"(none)\");\n>> +\t\treturn;\n>> +\t}\n>> +\tif (strchr(buf, '.'))\n>> +\t\tstrbuf_addstr(out, buf);\n>> +\telse\t{\n> Many ' ' between else and '{', one should be enough\n>> +\t\tmemset (&hints, '\\0', sizeof (hints));\n>> +\t\thints.ai_flags = AI_CANONNAME;\n>> +\t\tif (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {\n>> +\t\t\tstrbuf_addstr(out, ai->ai_canonname);\n>> +\t\t\tfreeaddrinfo(ai);\n>> +\t\t}\n>> +\t\telse\n> Colud be written in one line as \"} else\"\n>> +\t\t\tstrbuf_addf(out, \"%s.(none)\", buf);\n>> +\t}\n>> +}\n>> +#else /* NO_IPV6 */\n"},{"id":"272545","messageId":"CAPig+cQkWc+R2vEn9DBdKVcsNNEWELCNNOMksBDxF7A5dTPHkQ@mail.gmail.com","threadId":"40677","inReplyTo":"5633A838.3070801@web.de","subject":"Re: [PATCH] ident.c: add support for IPv6","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-30T18:19:06Z","receivedAt":"2015-10-30T18:19:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 30, 2015 at 1:26 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 2015-10-30 15.48, Elia Pinto wrote:\n>> Add IPv6 support by implementing name resolution with the\n>> ---\n>> +#ifndef NO_IPV6\n>> +\n>> +static void add_domainname(struct strbuf *out)\n>> +{\n>> +     char buf[1024];\n>> +     struct addrinfo hints, *ai;\n>> +     int gai;\n> The scope of these variables can be narrowed, by moving them into the \"{\" block,\n> where they are needed. (Before the memset())\n>> +\n>> +     if (gethostname(buf, sizeof(buf))) {\n>> +             warning(\"cannot get host name: %s\", strerror(errno));\n>> +             strbuf_addstr(out, \"(none)\");\n>> +             return;\n>> +     }\n>> +     if (strchr(buf, '.'))\n>> +             strbuf_addstr(out, buf);\n>> +     else    {\n> Many ' ' between else and '{', one should be enough\n>> +             memset (&hints, '\\0', sizeof (hints));\n>> +             hints.ai_flags = AI_CANONNAME;\n>> +             if (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {\n\nWhy is 'gai' needed and assigned? It's value is never consulted thereafter.\n\n>> +                     strbuf_addstr(out, ai->ai_canonname);\n>> +                     freeaddrinfo(ai);\n\nAlso, aren't you leaking 'ai' when 'ai_canonname' doesn't contain a '.'?\n\n>> +             }\n>> +             else\n> Colud be written in one line as \"} else\"\n>> +                     strbuf_addf(out, \"%s.(none)\", buf);\n>> +     }\n>> +}\n>> +#else /* NO_IPV6 */\n"},{"id":"272550","messageId":"20151030182939.GA4729@sigill.intra.peff.net","threadId":"40677","inReplyTo":"1446216487-11503-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] ident.c: add support for IPv6","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-30T18:29:39Z","receivedAt":"2015-10-30T18:29:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2015 at 03:48:07PM +0100, Elia Pinto wrote:\n\n> Add IPv6 support by implementing name resolution with the\n> protocol agnostic getaddrinfo(3) API. The old gethostbyname(3)\n> code is still available when git is compiled with NO_IPV6.\n\nMakes sense. I'm not excited by the duplication in the early part of the\nfunction, though:\n\n> +#ifndef NO_IPV6\n> +\n> +static void add_domainname(struct strbuf *out)\n> +{\n> +\tchar buf[1024];\n> +\tstruct addrinfo hints, *ai;\n> +\tint gai;\n> +\n> +\tif (gethostname(buf, sizeof(buf))) {\n> +\t\twarning(\"cannot get host name: %s\", strerror(errno));\n> +\t\tstrbuf_addstr(out, \"(none)\");\n> +\t\treturn;\n> +\t}\n> +\tif (strchr(buf, '.'))\n> +\t\tstrbuf_addstr(out, buf);\n> +\telse\t{\n> +\t\tmemset (&hints, '\\0', sizeof (hints));\n> +\t\thints.ai_flags = AI_CANONNAME;\n> +\t\tif (!(gai = getaddrinfo(buf, NULL, &hints, &ai)) && ai && strchr(ai->ai_canonname, '.')) {\n> +\t\t\tstrbuf_addstr(out, ai->ai_canonname);\n> +\t\t\tfreeaddrinfo(ai);\n> +\t\t}\n> +\t\telse\n> +\t\t\tstrbuf_addf(out, \"%s.(none)\", buf);\n> +\t}\n> +}\n\nEspecially the \"(none)\" stuff is ugly enough as it is, without being\nduplicated in two spots. Can we factor out the else clause that calls\ngethostbyname(), and just override that part with the #ifdef?\n\nFor that matter, we have a few other spots that use getaddrinfo and\n#ifdef. I wonder if it would be possible to simply use getaddrinfo\neverywhere, and make a compatibility wrapper that uses gethostbyname for\nolder systems. The cut-and-paste duplication in connect.c, for example,\nis pretty egregious.\n\n-Peff\n"}]}