On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
> On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> > On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> >> On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> >>> On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
> >>> <patthoyts@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>>> Erik Faye-Lund <kusmabite@xxxxxxxxx> 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);
> }
> }
> }
As you say yourself, it's not elegant at all - sockaddr_storage is
intended to be just that, an struct large enough to fit all the sockaddrs
you'll encounter on this platform, with all fields aligned in the same way
as all the other sockaddr structs. You're supposed to be able to cast the
sockaddr struct pointers like currently is done, although I'm not familiar
with the strict aliasing stuff well enough to know if anything else would
be required somewhere.
I didn't see any of these hacks in the v7 patchset - did the warning go
away by itself there?
FWIW, the actual warning itself isn't directly related to any of the code
worked on here, gcc just happens to realize it after some of these
changes. I'm able to trigger the same warnings on the current master, by
simply doing this change:
diff --git a/daemon.c b/daemon.c
index 5783e24..467cea2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -670,7 +670,6 @@ static void handle(int incoming, struct sockaddr
*addr, int
dup2(incoming, 1);
close(incoming);
- exit(execute(addr));
}
static void child_handler(int signo)
// Martin