Portability patches vs 1.7.0.2 [5/6]

Gary V. Vaughan· Mar 9, 2010, 16:19 UTC · lore

## sockaddr_t->ss_family is not portable

Many of our supported platforms do not have this declaration, for example solaris2.6 thru 2.8 (I can find a complete list of which of our supported platforms have this problem).

Normally, I would solve this by importing the relevant modules from gnulib and linking in the portable equivalent (hence the name of the patch: gnulib.patch). But in this case, since git is not built using Automake, I decided that it would be easier to simply revert the part of the code that was triggering this problem back to what was used in git-1.6.6. So that is what this patch represents.

Actually, I'd like some advice on whether this was a good idea? Was the change to the code in git-1.7.0 introduced in response to a bug (latent or reported) that is fixed by the code I reverted in this patch? If so, what is the right way to fix it (assuming you don't want to introduce a dependency on gnulib and Automake into git)?

Cheers,

-- 
Gary V. Vaughan (gary@thewrittenword.com)


Index: daemon.c
===================================================================
--- daemon.c.orig
+++ daemon.c
@@ -588,24 +588,6 @@ static int execute(struct sockaddr *addr
 	return -1;
 }
 
-static int addrcmp(const struct sockaddr_storage *s1,
-    const struct sockaddr_storage *s2)
-{
-	if (s1->ss_family != s2->ss_family)
-		return s1->ss_family - s2->ss_family;
-	if (s1->ss_family == AF_INET)
-		return memcmp(&((struct sockaddr_in *)s1)->sin_addr,
-		    &((struct sockaddr_in *)s2)->sin_addr,
-		    sizeof(struct in_addr));
-#ifndef NO_IPV6
-	if (s1->ss_family == AF_INET6)
-		return memcmp(&((struct sockaddr_in6 *)s1)->sin6_addr,
-		    &((struct sockaddr_in6 *)s2)->sin6_addr,
-		    sizeof(struct in6_addr));
-#endif
-	return 0;
-}
-
 static int max_connections = 32;
 
 static unsigned int live_children;
@@ -625,7 +607,7 @@ static void add_child(pid_t pid, struct 
 	newborn->pid = pid;
 	memcpy(&newborn->address, addr, addrlen);
 	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
-		if (!addrcmp(&(*cradle)->address, &newborn->address))
+		if (!memcmp(&(*cradle)->address, &newborn->address, sizeof(newborn->address)))
 			break;
 	newborn->next = *cradle;
 	*cradle = newborn;
@@ -658,7 +640,7 @@ static void kill_some_child(void)
 		return;
 
 	for (; (next = blanket->next); blanket = next)
-		if (!addrcmp(&blanket->address, &next->address)) {
+		if (!memcmp(&blanket->address, &next->address, sizeof(next->address))) {
 			kill(blanket->pid, SIGTERM);
 			break;
 		}

Erik Faye-Lund· Mar 9, 2010, 17:08 UTC · re: Gary V. Vaughan · lore

Re: Portability patches vs 1.7.0.2 [5/6]

In the future, please read Documentation/SubmittingPatches before sending patches. Patches should be inlined, for one.

On Tue, Mar 9, 2010 at 5:19 PM, Gary V. Vaughan <git@mlists.thewrittenword.com> wrote:

> Actually, I'd like some advice on whether this was a good idea?  Was
> the change to the code in git-1.7.0 introduced in response to a bug
> (latent or reported) that is fixed by the code I reverted in this
> patch?

I'm the author of commit 15515b73, who introduced that change (due to a bug). This is something that git-blame could have told you, and you could have CC'ed me. It's pure luck that I had a look at this patch.

Simply doing memcmp will not work, since sockaddr_storage contains both the IP-address and the port, but the port should not be compared.

>  If so, what is the right way to fix it (assuming you don't
> want to introduce a dependency on gnulib and Automake into git)?
>

If ss_family doesn't exist, does that mean that the platform only supports IPv4?

-- 
Erik "kusma" Faye-Lund

Jeff King· Mar 9, 2010, 23:26 UTC · re: Gary V. Vaughan · lore

Re: Portability patches vs 1.7.0.2 [5/6]

On Tue, Mar 09, 2010 at 04:19:06PM +0000, Gary V. Vaughan wrote:

> ## sockaddr_t->ss_family is not portable

I assume you mean sockaddr_storage here?

> Many of our supported platforms do not have this declaration, for
> example solaris2.6 thru 2.8 (I can find a complete list of which of
> our supported platforms have this problem).

Did you mean that list to be inclusive? My Solaris 2.8 definitely has sockaddr_storage, as it is necessary for IPv6 handling. I don't remember when IPv6 support was added, though...if it was in a minor release, then presumably earlier versions of 2.8 did not.

At any rate, we should only need it for IPv6 support, so I think the right solution would be to include all uses inside an "#ifndef NO_IPV6", and for the NO_IPV6 case assume it's a sockaddr_in.

-Peff