[libvirt] [PATCH] Cygwin's GCC doesn't like this .sa_handler initialization for some reason
--- examples/domain-events/events-c/event-test.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 53a3195..74eabba 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -380,10 +380,11 @@ int main(int argc, char **argv) int callback5ret = -1; int callback6ret = -1; int callback7ret = -1; + struct sigaction action_stop; - struct sigaction action_stop = { - .sa_handler = stop - }; + memset(&action_stop, 0, sizeof action_stop); + + action_stop.sa_handler = stop; if(argc > 1 && STREQ(argv[1],"--help")) { usage(argv[0]); -- 1.6.3.3
On 04/25/2010 05:32 AM, Matthias Bolte wrote:
--- examples/domain-events/events-c/event-test.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 53a3195..74eabba 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -380,10 +380,11 @@ int main(int argc, char **argv) int callback5ret = -1; int callback6ret = -1; int callback7ret = -1; + struct sigaction action_stop;
- struct sigaction action_stop = { - .sa_handler = stop - }; + memset(&action_stop, 0, sizeof action_stop); + + action_stop.sa_handler = stop;
ACK. By the way, the "for some reason" boils down to how sa_handler is declared in <signal.h> on the two platforms. On cygwin, sa_handler happens to be a member of an anonymous union (exploiting a gcc extension); whereas on Linux, it is a macro that accesses a member of a named union. If I read POSIX correctly, both behaviors for the declaration of sa_handler are permitted. But dotted assignment only works in the case of a named union. Whether it is a gcc bug that you can't use dotted assignment to access a member of an anonymous union, or a cygwin bug for having a header that does not allow compilation like Linux (when cygwin's stated goal is to be Linux-compatible) is debatable. By the way, this use of memset() to initialize action_stop assumes that the null pointer is all 0 bits (which is probably okay for all platforms that libvirt will ever run on). There is currently a discussion on gnulib on the fact that to be portable to C99, where a null pointer might not be all 0 bits, you would instead have to use the more drawn-out sequence: static struct sigaction zero_sigaction = {0}; struct sigaction action_stop = zero_sigaction; action_stop.sa_handler = stop; But don't go changing this commit just for that theoretical platform. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/4/26 Eric Blake <eblake@redhat.com>:
On 04/25/2010 05:32 AM, Matthias Bolte wrote:
--- examples/domain-events/events-c/event-test.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 53a3195..74eabba 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -380,10 +380,11 @@ int main(int argc, char **argv) int callback5ret = -1; int callback6ret = -1; int callback7ret = -1; + struct sigaction action_stop;
- struct sigaction action_stop = { - .sa_handler = stop - }; + memset(&action_stop, 0, sizeof action_stop); + + action_stop.sa_handler = stop;
ACK. By the way, the "for some reason" boils down to how sa_handler is declared in <signal.h> on the two platforms. On cygwin, sa_handler happens to be a member of an anonymous union (exploiting a gcc extension); whereas on Linux, it is a macro that accesses a member of a named union. If I read POSIX correctly, both behaviors for the declaration of sa_handler are permitted. But dotted assignment only works in the case of a named union. Whether it is a gcc bug that you can't use dotted assignment to access a member of an anonymous union, or a cygwin bug for having a header that does not allow compilation like Linux (when cygwin's stated goal is to be Linux-compatible) is debatable.
By the way, this use of memset() to initialize action_stop assumes that the null pointer is all 0 bits (which is probably okay for all platforms that libvirt will ever run on). There is currently a discussion on gnulib on the fact that to be portable to C99, where a null pointer might not be all 0 bits, you would instead have to use the more drawn-out sequence:
You say null pointer, but where's the pointer in this example? Do you really refer to pointers or to the initialization of memory? I wonder what's the use case for this.
static struct sigaction zero_sigaction = {0}; struct sigaction action_stop = zero_sigaction; action_stop.sa_handler = stop;
But don't go changing this commit just for that theoretical platform.
Thanks, pushed. Matthias
On 04/26/2010 01:20 PM, Matthias Bolte wrote:
ACK. By the way, the "for some reason" boils down to how sa_handler is declared in <signal.h> on the two platforms. On cygwin, sa_handler happens to be a member of an anonymous union (exploiting a gcc extension); whereas on Linux, it is a macro that accesses a member of a named union. If I read POSIX correctly, both behaviors for the declaration of sa_handler are permitted. But dotted assignment only works in the case of a named union. Whether it is a gcc bug that you can't use dotted assignment to access a member of an anonymous union, or a cygwin bug for having a header that does not allow compilation like Linux (when cygwin's stated goal is to be Linux-compatible) is debatable.
GCC bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676 But cygwin will (probably) be changing to match Linux anyways: http://cygwin.com/ml/cygwin/2010-04/msg00947.html
By the way, this use of memset() to initialize action_stop assumes that the null pointer is all 0 bits (which is probably okay for all platforms that libvirt will ever run on). There is currently a discussion on gnulib on the fact that to be portable to C99, where a null pointer might not be all 0 bits, you would instead have to use the more drawn-out sequence:
You say null pointer, but where's the pointer in this example?
As an app writer, you have no idea what struct sigaction includes. You know what POSIX requires that it contains (sa_flags is integral, sa_handler _is_ a pointer but we happen to be initializing it, and sa_sigaction is a pointer but is allowed to overlap with sa_handler), but even then, you don't know is whether sa_mask is a pointer (POSIX allows that, even though it is integral on Linux). Then the fact remains that you don't know whether the implementation has other fields as extensions (on Linux, sa_restorer is an extension, and _is_ a pointer; fortunately, Linux only runs on hardware where the null pointer is all 0 bits). Therefore, a strictly C99-compliant app must rely on compiler initialization to ensure that all contained pointers are initialized to their proper bit value, which might not be all 0s on weird architectures; the runtime initialization to all 0-bits using memset() would assign invalid values to any such pointer contained in the struct. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/4/26 Eric Blake <eblake@redhat.com>:
On 04/26/2010 01:20 PM, Matthias Bolte wrote:
ACK. By the way, the "for some reason" boils down to how sa_handler is declared in <signal.h> on the two platforms. On cygwin, sa_handler happens to be a member of an anonymous union (exploiting a gcc extension); whereas on Linux, it is a macro that accesses a member of a named union. If I read POSIX correctly, both behaviors for the declaration of sa_handler are permitted. But dotted assignment only works in the case of a named union. Whether it is a gcc bug that you can't use dotted assignment to access a member of an anonymous union, or a cygwin bug for having a header that does not allow compilation like Linux (when cygwin's stated goal is to be Linux-compatible) is debatable.
GCC bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676 But cygwin will (probably) be changing to match Linux anyways: http://cygwin.com/ml/cygwin/2010-04/msg00947.html
By the way, this use of memset() to initialize action_stop assumes that the null pointer is all 0 bits (which is probably okay for all platforms that libvirt will ever run on). There is currently a discussion on gnulib on the fact that to be portable to C99, where a null pointer might not be all 0 bits, you would instead have to use the more drawn-out sequence:
You say null pointer, but where's the pointer in this example?
As an app writer, you have no idea what struct sigaction includes. You know what POSIX requires that it contains (sa_flags is integral, sa_handler _is_ a pointer but we happen to be initializing it, and sa_sigaction is a pointer but is allowed to overlap with sa_handler), but even then, you don't know is whether sa_mask is a pointer (POSIX allows that, even though it is integral on Linux). Then the fact remains that you don't know whether the implementation has other fields as extensions (on Linux, sa_restorer is an extension, and _is_ a pointer; fortunately, Linux only runs on hardware where the null pointer is all 0 bits). Therefore, a strictly C99-compliant app must rely on compiler initialization to ensure that all contained pointers are initialized to their proper bit value, which might not be all 0s on weird architectures; the runtime initialization to all 0-bits using memset() would assign invalid values to any such pointer contained in the struct.
Ah, okay. I didn't get that you referred to the struct members. Now I understand what you meant, thanks for the detailed explanation. Matthias
libvir-list-bounces@redhat.com wrote on 04/26/2010 03:20:47 PM:
static struct sigaction zero_sigaction = {0}; struct sigaction action_stop = zero_sigaction; action_stop.sa_handler = stop;
But don't go changing this commit just for that theoretical platform.
Thanks, pushed.
Does it compile for you on cygwin? I need to make the following changes just to get it to link properly: diff --git a/src/Makefile.am b/src/Makefile.am index fc64927..723d221 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -928,7 +928,7 @@ libvirt.def: libvirt.syms libvirt_la_SOURCES = libvirt_la_LIBADD += \ $(CYGWIN_EXTRA_LIBADD) ../gnulib/lib/libgnu.la -libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_SYMBOL_FILE) \ +libvirt_la_LDFLAGS = \ -version-info $(LIBVIRT_VERSION_INFO) \ $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ $(LIBXML_LIBS) \ diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index c9816dd..854b0fd 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -413,12 +413,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - (void)IXDR_PUT_INT32(buf, objp->cpus); - (void)IXDR_PUT_INT32(buf, objp->mhz); - (void)IXDR_PUT_INT32(buf, objp->nodes); - (void)IXDR_PUT_INT32(buf, objp->sockets); - (void)IXDR_PUT_INT32(buf, objp->cores); - (void)IXDR_PUT_INT32(buf, objp->threads); + (void)IXDR_PUT_LONG(buf, objp->cpus); + (void)IXDR_PUT_LONG(buf, objp->mhz); + (void)IXDR_PUT_LONG(buf, objp->nodes); + (void)IXDR_PUT_LONG(buf, objp->sockets); + (void)IXDR_PUT_LONG(buf, objp->cores); + (void)IXDR_PUT_LONG(buf, objp->threads); } return TRUE; } else if (xdrs->x_op == XDR_DECODE) { @@ -442,12 +442,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - objp->cpus = IXDR_GET_INT32(buf); - objp->mhz = IXDR_GET_INT32(buf); - objp->nodes = IXDR_GET_INT32(buf); - objp->sockets = IXDR_GET_INT32(buf); - objp->cores = IXDR_GET_INT32(buf); - objp->threads = IXDR_GET_INT32(buf); + objp->cpus = IXDR_GET_LONG(buf); + objp->mhz = IXDR_GET_LONG(buf); + objp->nodes = IXDR_GET_LONG(buf); + objp->sockets = IXDR_GET_LONG(buf); + objp->cores = IXDR_GET_LONG(buf); + objp->threads = IXDR_GET_LONG(buf); } return TRUE; } Virsh doesn't work then, but at least it links after these changes. Stefan
2010/4/27 Stefan Berger <stefanb@us.ibm.com>:
libvir-list-bounces@redhat.com wrote on 04/26/2010 03:20:47 PM:
static struct sigaction zero_sigaction = {0}; struct sigaction action_stop = zero_sigaction; action_stop.sa_handler = stop;
But don't go changing this commit just for that theoretical platform.
Thanks, pushed.
Does it compile for you on cygwin?
It does, but not all my patches have reached the GIT repo yet.
I need to make the following changes just to get it to link properly:
diff --git a/src/Makefile.am b/src/Makefile.am index fc64927..723d221 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -928,7 +928,7 @@ libvirt.def: libvirt.syms libvirt_la_SOURCES = libvirt_la_LIBADD += \ $(CYGWIN_EXTRA_LIBADD) ../gnulib/lib/libgnu.la -libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_SYMBOL_FILE) \ +libvirt_la_LDFLAGS = \ -version-info $(LIBVIRT_VERSION_INFO) \ $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ $(LIBXML_LIBS) \
Ah, I forgot to post a patch that fixes version script handling for Cygwin. In contrast to MinGW's ld, Cygwin's ld handles version scripts properly: https://www.redhat.com/archives/libvir-list/2010-April/msg01202.html
diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index c9816dd..854b0fd 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -413,12 +413,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - (void)IXDR_PUT_INT32(buf, objp->cpus); - (void)IXDR_PUT_INT32(buf, objp->mhz); - (void)IXDR_PUT_INT32(buf, objp->nodes); - (void)IXDR_PUT_INT32(buf, objp->sockets); - (void)IXDR_PUT_INT32(buf, objp->cores); - (void)IXDR_PUT_INT32(buf, objp->threads); + (void)IXDR_PUT_LONG(buf, objp->cpus); + (void)IXDR_PUT_LONG(buf, objp->mhz); + (void)IXDR_PUT_LONG(buf, objp->nodes); + (void)IXDR_PUT_LONG(buf, objp->sockets); + (void)IXDR_PUT_LONG(buf, objp->cores); + (void)IXDR_PUT_LONG(buf, objp->threads); } return TRUE; } else if (xdrs->x_op == XDR_DECODE) { @@ -442,12 +442,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - objp->cpus = IXDR_GET_INT32(buf); - objp->mhz = IXDR_GET_INT32(buf); - objp->nodes = IXDR_GET_INT32(buf); - objp->sockets = IXDR_GET_INT32(buf); - objp->cores = IXDR_GET_INT32(buf); - objp->threads = IXDR_GET_INT32(buf); + objp->cpus = IXDR_GET_LONG(buf); + objp->mhz = IXDR_GET_LONG(buf); + objp->nodes = IXDR_GET_LONG(buf); + objp->sockets = IXDR_GET_LONG(buf); + objp->cores = IXDR_GET_LONG(buf); + objp->threads = IXDR_GET_LONG(buf); } return TRUE; }
Virsh doesn't work then, but at least it links after these changes.
Stefan
I just pushed a patch to fix this. virsh works for me. I tested it with a connection to a remote libvirtd using a plain unencrypted TCP connection: virsh -c qemu+tcp://<server>/system Matthias
Matthias Bolte <matthias.bolte@googlemail.com> wrote on 04/27/2010 04:03:54 AM:
2010/4/27 Stefan Berger <stefanb@us.ibm.com>:
[...]
I just pushed a patch to fix this.
Got the git update now and your build fix.
virsh works for me. I tested it with a connection to a remote libvirtd using a plain unencrypted TCP connection: virsh -c qemu+tcp://<server>/system
Not the same luck here. I still see it aborting. Also the 'make check' test-poll doesn't succeed here. Stefan
On 04/27/2010 07:00 AM, Stefan Berger wrote:
virsh works for me. I tested it with a connection to a remote libvirtd using a plain unencrypted TCP connection: virsh -c qemu+tcp://<server>/system
Not the same luck here. I still see it aborting. Also the 'make check' test-poll doesn't succeed here.
Gnulib's test-poll is tickling a known upstream cygwin bug; you can probably ignore that now while it is still being tracked down between gnulib and cygwin on how best to work around it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Apr 26, 2010 at 06:46:31PM -0400, Stefan Berger wrote:
libvir-list-bounces@redhat.com wrote on 04/26/2010 03:20:47 PM:
static struct sigaction zero_sigaction = {0}; struct sigaction action_stop = zero_sigaction; action_stop.sa_handler = stop;
But don't go changing this commit just for that theoretical platform.
Thanks, pushed.
Does it compile for you on cygwin?
I need to make the following changes just to get it to link properly:
diff --git a/src/Makefile.am b/src/Makefile.am index fc64927..723d221 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -928,7 +928,7 @@ libvirt.def: libvirt.syms libvirt_la_SOURCES = libvirt_la_LIBADD += \ $(CYGWIN_EXTRA_LIBADD) ../gnulib/lib/libgnu.la -libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_SYMBOL_FILE) \ +libvirt_la_LDFLAGS = \ -version-info $(LIBVIRT_VERSION_INFO) \ $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ $(LIBXML_LIBS) \ diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index c9816dd..854b0fd 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -413,12 +413,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - (void)IXDR_PUT_INT32(buf, objp->cpus); - (void)IXDR_PUT_INT32(buf, objp->mhz); - (void)IXDR_PUT_INT32(buf, objp->nodes); - (void)IXDR_PUT_INT32(buf, objp->sockets); - (void)IXDR_PUT_INT32(buf, objp->cores); - (void)IXDR_PUT_INT32(buf, objp->threads); + (void)IXDR_PUT_LONG(buf, objp->cpus); + (void)IXDR_PUT_LONG(buf, objp->mhz); + (void)IXDR_PUT_LONG(buf, objp->nodes); + (void)IXDR_PUT_LONG(buf, objp->sockets); + (void)IXDR_PUT_LONG(buf, objp->cores); + (void)IXDR_PUT_LONG(buf, objp->threads); } return TRUE; } else if (xdrs->x_op == XDR_DECODE) { @@ -442,12 +442,12 @@ xdr_remote_node_get_info_ret (XDR *xdrs, remote_node_get_info_ret *objp) if (!xdr_int (xdrs, &objp->threads)) return FALSE; } else { - objp->cpus = IXDR_GET_INT32(buf); - objp->mhz = IXDR_GET_INT32(buf); - objp->nodes = IXDR_GET_INT32(buf); - objp->sockets = IXDR_GET_INT32(buf); - objp->cores = IXDR_GET_INT32(buf); - objp->threads = IXDR_GET_INT32(buf); + objp->cpus = IXDR_GET_LONG(buf); + objp->mhz = IXDR_GET_LONG(buf); + objp->nodes = IXDR_GET_LONG(buf); + objp->sockets = IXDR_GET_LONG(buf); + objp->cores = IXDR_GET_LONG(buf); + objp->threads = IXDR_GET_LONG(buf); } return TRUE; }
NACK to this change. It is reverting commit 9322b2e8617bf7f4a4d9b8a686dcf130efb2d652 Author: Daniel P. Berrange <berrange@redhat.com> Date: Wed Jan 28 21:33:56 2009 +0000 Solaris portability for RPC code data types If the Cygwin XDR doesn't support this, then just build the portablexdr library & use that instead, since that is 100% compatible with glibc. http://people.redhat.com/~rjones/portablexdr/ Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Sun, Apr 25, 2010 at 01:32:32PM +0200, Matthias Bolte wrote:
--- examples/domain-events/events-c/event-test.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 53a3195..74eabba 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -380,10 +380,11 @@ int main(int argc, char **argv) int callback5ret = -1; int callback6ret = -1; int callback7ret = -1; + struct sigaction action_stop;
- struct sigaction action_stop = { - .sa_handler = stop - }; + memset(&action_stop, 0, sizeof action_stop); + + action_stop.sa_handler = stop;
if(argc > 1 && STREQ(argv[1],"--help")) { usage(argv[0]);
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Matthias Bolte -
Stefan Berger