Re: [PATCH v6 00/16] daemon-win32

On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund [off-list ref] wrote:

On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:

quoted

On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:

quoted

Erik Faye-Lund [off-list ref] writes:

quoted

Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.

I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:

   CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here

Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.

OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.

Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.

I also of course need to update "daemon: get remote host address from
root-process" as well, as it introduces a new such code-path (which is
actually the one complained about here). And I guess I should use
sockaddr_in instead of sockaddr.

But my luck stops there. The resulting git-daemon.exe leaves me with a
very bizarre error:

error: unable to make a socket file descriptor: Bad file descriptor
fatal: accept returned: Bad file descriptor

This is triggered by the call to accept() in mingw_accept returning -1.

What is even stranger is that if I change the code at the error-point like this
-				struct sockaddr_in sa;
+				struct sockaddr_in sa[2];
 				socklen_t salen = sizeof(sa);
-				int incoming = accept(pfd[i].fd, (struct sockaddr *)&sa, &salen);
+				int incoming = accept(pfd[i].fd, (struct sockaddr *)sa, &salen);

the error goes away. Similarly, if I change mingw_accept's call to
Winsock's accept(), like this:
-	SOCKET s2 = accept(s1, sa, sz);
+	SOCKET s2 = accept(s1, sa, NULL);

So it seems accept() somehow reacts to the value of the variable
pointed at by sz, which is 16. Strange, huh?

Perhaps it isn't -- the sockaddr_storage change seems to have been
introduced for IPv6 reasons. I'm trying to connect over IPv6, and IPv6
has a new sockaddr_in6 struct. So yeah.

Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?