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

[Erik Faye-Lund <kusmabite@gmail.com>]

On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> 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.
>>
>
> 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?
>

Just for reference, this is the patch that fixes it. What do you think?

diff --git a/daemon.c b/daemon.c
index 941c095..8162f10 100644
--- a/daemon.c
+++ b/daemon.c
@@ -902,9 +903,15 @@ static int service_loop(struct socketlist *socklist)

 		for (i = 0; i < socklist->nr; i++) {
 			if (pfd[i].revents & POLLIN) {
-				struct sockaddr_storage ss;
+				union {
+					struct sockaddr sa;
+					struct sockaddr_in sai;
+#ifndef NO_IPV6
+					struct sockaddr_in6 sai6;
+#endif
+				} ss;
 				unsigned int sslen = sizeof(ss);
-				int incoming = accept(pfd[i].fd, (struct sockaddr *)&ss, &sslen);
+				int incoming = accept(pfd[i].fd, &ss.sa, &sslen);
 				if (incoming < 0) {
 					switch (errno) {
 					case EAGAIN:
@@ -915,7 +922,7 @@ static int service_loop(struct socketlist *socklist)
 						die_errno("accept returned");
 					}
 				}
-				handle(incoming, (struct sockaddr *)&ss, sslen);
+				handle(incoming, &ss.sa, sslen);
 			}
 		}
 	}