threads / patch / 19501

patchimap-send: add support for IPv6

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

## tl;dr

3 messages between May 25, 2009 and May 27, 2009. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Benjamin Kramer· May 25, 2009, 19:13 UTC · lore

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(-)

Show changes to imap-send.c +51 −3

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

Junio C Hamano· May 27, 2009, 06:28 UTC · re: Benjamin Kramer · lore

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

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.

Show 14 quoted lines

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

Show 9 quoted lines

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

Show 14 quoted lines

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

Show 29 quoted lines

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

Benjamin Kramer· May 27, 2009, 10:50 UTC · re: Junio C Hamano · lore

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

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.