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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html