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

From: Junio C Hamano <gitster@pobox.com>
Date: May 27, 2009, 06:28 UTC
Message-ID: <7v7i03kqjq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4A1AEDF2.7060307@googlemail.com>

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.