Re: [PATCH 4/5] tcp: unify ipv4 and ipv6 code paths

[Jonathan Nieder <jrnieder@gmail.com>]

Erik Faye-Lund wrote:

> I'm not entirely sure I understand the motivation here. We already had
> well-tested, implementations of IPv4 and IPv6 tcp-socket setup. Here
> you unify the code by adding abstraction, but it ends up amounting to
> more lines of code, with the details scattered around in different
> source files.

Thanks.  It's true: I didn't convey the motivation well at all.

Before this patch, git_tcp_connect_sock() looks roughly like this:

	#ifndef NO_IPV6

	static int git_tcp_connect_sock(char *host, int flags)
	{
		get_host_and_port(&host, &port);
		memcpy(&hints, tcp_stream_hints, sizeof(hints));
		gai = getaddrinfo(host, port, &hints, &ai);
		for (ai0 = ai; ai; ai = ai->ai_next, cnt++) {
			... socket(), connect(), etc ...
		}
		freeaddrinfo(ai0);
		enable_keepalive(sockfd);
		return sockfd;
	}

	#else

	static int git_tcp_connect_sock(char *host, int flags)
	{
		get_host_and_port(&host, &port);
		he = gethostbyname(host);
		for (ap = he->h_addr_list; *ap; ap++, cnt++) {
			... socket(), connect(), etc ...
		}
		enable_keepalive(sockfd);
		return sockfd;
	}

	#endif

Any change to this procedure has to be made in both places, and in the
past, that has caused some minor bugs that were unnoticed for a while,
but no big deal --- it's not that complicated, so the code duplication
is tolerable.

After this patch and the next two, it looks like so:

	static int git_tcp_connect_sock(char *host, int flags)
	{
		get_host_and_port(&host, &port);
		gai = dns_resolve(host, port, 0, &ai);
		for_each_address(i, ai) {
			... socket(), connect(), etc ...
		}
		dns_free(ai0);
		enable_keepalive(sockfd);
		return sockfd;
	}

That is, it is almost identical to the getaddrinfo version.

So it is not about making the code shorter, but about not repeating
ourselves.

I did this for my sanity while implementing the SRV code: if my
changes work with the gethostbyname API, this way it is much more
likely this way that they will still work with getaddrinfo, too.  So
basically, the point is that I did not want to have to think about the
!NO_IPV6 codepath while writing patches (and others might not want to
have to think about the NO_IPV6 codepath at all --- the same
advantages apply for them, too).

	API dictionary in the !NO_IPV6 case
	(see dns-ipv6.h and dns-ipv6.c for details):

	resolver_result = struct addrinfo *
	resolved_address = const struct addrinfo *
	dns_resolve = getaddrinfo
	dns_name = getnameinfo, use in error messages
	dns_ip_address = getnameinfo, git-daemon uses for %IP substitution
	for_each_address = for (ai0 = ai; ai; ai = ai->ai_next)

	dns_family = ->ai_family
	dns_socktype = ->ai_socktype
	etc

	dns_strerror = gai_strerror
	dns_free = freeaddrinfo

The dns_name()/dns_ip_address() pair is a little silly.  The former
returns its result in a static buffer and the latter uses malloc.
Improvements welcome.

Of course, git_tcp_connect_sock() is not the only function that uses
host resolution facilities.  The benefit scales as more functions get
converted.

Hope that helps,
Jonathan