[PATCH] imap-send: add support for IPv6

[PATCH] imap-send: add support for IPv6

[Benjamin Kramer]

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: Benjamin Kramer <benny.kra@googlemail.com>
---
 imap-send.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 8154cb2..861268a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -982,9 +982,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	struct imap_store *ctx;
 	struct imap *imap;
 	char *arg, *rsp;
-	struct hostent *he;
-	struct sockaddr_in addr;
-	int s, a[2], preauth;
+	int s = -1, a[2], preauth;
 	pid_t pid;
 
 	ctx = xcalloc(sizeof(*ctx), 1);
@@ -1021,6 +1019,51 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 
 		imap_info("ok\n");
 	} else {
+#ifndef NO_IPV6
+		struct addrinfo hints, *ai0, *ai;
+		int gai;
+		char portstr[6];
+
+		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+
+		memset(&hints, 0, sizeof(hints));
+		hints.ai_socktype = SOCK_STREAM;
+		hints.ai_protocol = IPPROTO_TCP;
+
+		imap_info("Resolving %s... ", srvc->host);
+		gai = getaddrinfo(srvc->host, portstr, &hints, &ai);
+		if (gai) {
+			fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(gai));
+			goto bail;
+		}
+		imap_info("ok\n");
+
+		for (ai0 = ai; ai; ai = ai->ai_next) {
+			char addr[NI_MAXHOST];
+
+			s = socket(ai->ai_family, ai->ai_socktype,
+				   ai->ai_protocol);
+			if (s < 0)
+				continue;
+
+			getnameinfo(ai->ai_addr, ai->ai_addrlen, addr,
+				    sizeof(addr), NULL, 0, NI_NUMERICHOST);
+			imap_info("Connecting to [%s]:%s... ", addr, portstr);
+
+			if (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {
+				close(s);
+				s = -1;
+				perror("connect");
+				continue;
+			}
+
+			break;
+		}
+		freeaddrinfo(ai0);
+#else /* NO_IPV6 */
+		struct hostent *he;
+		struct sockaddr_in addr;
+
 		memset(&addr, 0, sizeof(addr));
 		addr.sin_port = htons(srvc->port);
 		addr.sin_family = AF_INET;
@@ -1040,7 +1083,12 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		imap_info("Connecting to %s:%hu... ", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));
 		if (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {
 			close(s);
+			s = -1;
 			perror("connect");
+		}
+#endif
+		if (s < 0) {
+			fputs("Error: unable to connect to server.\n", stderr);
 			goto bail;
 		}
 
-- 
1.6.3.1.153.g63801e

Re: [PATCH] imap-send: add support for IPv6

[Junio C Hamano]

Benjamin Kramer <benny.kra@googlemail.com> writes:

> 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.

> diff --git a/imap-send.c b/imap-send.c
> index 8154cb2..861268a 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1021,6 +1019,51 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
>  
>  		imap_info("ok\n");
>  	} else {
> +#ifndef NO_IPV6
> +		struct addrinfo hints, *ai0, *ai;
> +		int gai;
> +		char portstr[6];
> +
> +		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);

We already use %h length specifier to explicitly say the parameter is
a short in the IPV4 part of this program, so I am sure this won't regress
anything for people, but I wonder what the point of it is...  (I am not
asking nor even suggesting to change this, by the way).

> +		memset(&hints, 0, sizeof(hints));
> +		hints.ai_socktype = SOCK_STREAM;
> +		hints.ai_protocol = IPPROTO_TCP;
> +
> +		imap_info("Resolving %s... ", srvc->host);
> +		gai = getaddrinfo(srvc->host, portstr, &hints, &ai);
> +		if (gai) {
> +			fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(gai));
> +			goto bail;

It is Ok for now (as existing codepath liberally uses fprintf() and
fputs() to report errors), but ideally we should start converting these to
error() calls, I think, in a follow-up patch.

> +		}
> +		imap_info("ok\n");
> +
> +		for (ai0 = ai; ai; ai = ai->ai_next) {
> +			char addr[NI_MAXHOST];
> +
> +			s = socket(ai->ai_family, ai->ai_socktype,
> +				   ai->ai_protocol);
> +			if (s < 0)
> +				continue;
> +
> +			getnameinfo(ai->ai_addr, ai->ai_addrlen, addr,
> +				    sizeof(addr), NULL, 0, NI_NUMERICHOST);
> +			imap_info("Connecting to [%s]:%s... ", addr, portstr);

Is forcing to NUMERICHOST done to match IPV4 codepath that does
inet_ntoa()?  I guess that makes sense.

> +			if (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {
> +				close(s);
> +				s = -1;
> +				perror("connect");
> +				continue;
> +			}
> +
> +			break;
> +		}
> +		freeaddrinfo(ai0);
> +#else /* NO_IPV6 */
> +		struct hostent *he;
> +		struct sockaddr_in addr;
> +
>  		memset(&addr, 0, sizeof(addr));
>  		addr.sin_port = htons(srvc->port);
>  		addr.sin_family = AF_INET;
> @@ -1040,7 +1083,12 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
>  		imap_info("Connecting to %s:%hu... ", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));
>  		if (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {
>  			close(s);
> +			s = -1;
>  			perror("connect");
> +		}
> +#endif
> +		if (s < 0) {
> +			fputs("Error: unable to connect to server.\n", stderr);
>  			goto bail;
>  		}

I do not use imap-send myself, and I suspect not many people do so either,
so I originally guessed the best way to judge if this has any portability
(or other) issues is to directly apply to 'master' and see if anybody
screams.

Thanks.

Re: [PATCH] imap-send: add support for IPv6

[Benjamin Kramer]

Junio C Hamano wrote:
> We already use %h length specifier to explicitly say the parameter is
> a short in the IPV4 part of this program, so I am sure this won't regress
> anything for people, but I wonder what the point of it is...  (I am not
> asking nor even suggesting to change this, by the way).

getaddrinfo(3) takes the port as a string so we have to convert it. I tried
to match the style of the existing code with the format string. The portstr
buffer is 6 chars long so the highest possible unsigned short 65535 fits in
exactly.

> It is Ok for now (as existing codepath liberally uses fprintf() and
> fputs() to report errors), but ideally we should start converting these to
> error() calls, I think, in a follow-up patch.

Looking at the number of fprintfs in imap-send.c a simple search/replace
probably won't do the job here. I tried to contact the original author but
his mail address seems to be dead ...

> Is forcing to NUMERICHOST done to match IPV4 codepath that does
> inet_ntoa()?  I guess that makes sense.

We need to get the IP string, otherwise the output of imap-send would
make no sense. Here's what imap-send outputs when I try to connect to
localhost.

Without the patch:
    Resolving localhost... ok
    Connecting to 127.0.0.1:993... connect: Connection refused

With the patch:
    Resolving localhost... ok
    Connecting to [::1]:993... connect: Connection refused
    Connecting to [fe80::1%lo0]:993... connect: Connection refused
    Connecting to [127.0.0.1]:993... connect: Connection refused
    Error: unable to connect to server.

Using the hostname instead of the IP address here wouldn't be very useful.