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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Nov 4, 2010, 00:06 UTC
Message-ID: <AANLkTin6_RLBJZKDaDViZ2_v7_LKpyoyvofowArx9sas@mail.gmail.com>
In-Reply-To: <AANLkTim4_5dgPGcw5B3U-3UH63REL4QWsWPV=mDSJJ6D@mail.gmail.com>

On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Show 39 quoted lines

> On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
>> <patthoyts@users.sourceforge.net> wrote:
>>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>>
>>>>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?