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

On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:

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

quoted

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.

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);
 			}
 		}
 	}