[libvirt] [PATCH] build: fix building with '--enable-test-locking'
When test-locking is enabled, CFLAGS have -Dbool=char, which makes pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: I still can't build with '--enable-test-locking' and I'm not sure this is the proper way to fix it. Also, the code wasn't touched for months, so I'm not pushing it as a build-breaker. src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 631cd06..88674ec 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -262,7 +262,7 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) } VIR_FORCE_CLOSE(*fd); - *fd = pipefd[output]; + *fd = pipefd[!!output]; return ret; error: -- 1.8.5.1
On 12/18/2013 02:35 AM, Martin Kletzander wrote:
When test-locking is enabled, CFLAGS have -Dbool=char, which makes
Huh? bool should never be defined to char in CFLAGS; it should only be replaced if gnulib thinks the compiler is too old.
pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker.
I don't get how this could possibly make a difference. The 'output' variable is declared bool, and then only ever assigned 'false' or 'true', which if bool is replaced by gnulib are still guaranteed to be 0 and 1, so using it directly as an array index is still safe.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I still can't build with '--enable-test-locking' and I'm not sure this is the proper way to fix it. Also, the code wasn't touched for months, so I'm not pushing it as a build-breaker.
NACK. I need more details about the actual failure you are seeing, but this is not the right patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/18/2013 06:09 AM, Eric Blake wrote:
On 12/18/2013 02:35 AM, Martin Kletzander wrote:
When test-locking is enabled, CFLAGS have -Dbool=char, which makes
Huh? bool should never be defined to char in CFLAGS; it should only be replaced if gnulib thinks the compiler is too old.
I actually looked at configure.ac, and wow, we really ARE doing -Dbool=char when trying to use CIL. Yuck. I think --with-test-locking has bit-rotted for some time now, and is going to need a LOT of work before it can be considered usable. That option was added in commit 6962a2d, in 2009, which predates my time on libvirt, and I don't ever recall using it myself. I'm not familiar enough with CIL to speculate why it doesn't work with C99 bool (or maybe that was a limitation back in 2009 but has since been fixed). Dan? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 18, 2013 at 06:09:40AM -0700, Eric Blake wrote:
On 12/18/2013 02:35 AM, Martin Kletzander wrote:
When test-locking is enabled, CFLAGS have -Dbool=char, which makes
Huh? bool should never be defined to char in CFLAGS; it should only be replaced if gnulib thinks the compiler is too old.
Gnulib doesn't do that, it's our configure.ac thing (but I must say I don't know whether that's copied from somewhere or not) introduced in 6962a2dd7e: AC_ARG_ENABLE([test-locking], [AS_HELP_STRING([--enable-test-locking], [thread locking tests using CIL @<:@default=no@:>@])], [case "${enableval}" in yes|no) ;; *) AC_MSG_ERROR([bad value ${enableval} for test-locking option]) ;; esac], [enableval=no]) enable_locking=$enableval if test "$enable_locking" = "yes"; then LOCK_CHECKING_CFLAGS="-Dbool=char -D_Bool=char -save-temps" AC_SUBST([LOCK_CHECKING_CFLAGS]) fi AM_CONDITIONAL([WITH_CIL],[test "$enable_locking" = "yes"])
pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker.
I don't get how this could possibly make a difference. The 'output' variable is declared bool, and then only ever assigned 'false' or 'true', which if bool is replaced by gnulib are still guaranteed to be 0 and 1, so using it directly as an array index is still safe.
But the thing is that due to -Dbool=char the variable is declared char, not bool.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I still can't build with '--enable-test-locking' and I'm not sure this is the proper way to fix it. Also, the code wasn't touched for months, so I'm not pushing it as a build-breaker.
NACK. I need more details about the actual failure you are seeing, but this is not the right patch.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/18/2013 06:25 AM, Martin Kletzander wrote:
pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker.
I don't get how this could possibly make a difference. The 'output' variable is declared bool, and then only ever assigned 'false' or 'true', which if bool is replaced by gnulib are still guaranteed to be 0 and 1, so using it directly as an array index is still safe.
But the thing is that due to -Dbool=char the variable is declared char, not bool.
But if the variable contains exactly 0 or 1, it still makes no difference as an array index whether it was typed bool or char. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 18, 2013 at 06:34:43AM -0700, Eric Blake wrote:
On 12/18/2013 06:25 AM, Martin Kletzander wrote:
pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker.
I don't get how this could possibly make a difference. The 'output' variable is declared bool, and then only ever assigned 'false' or 'true', which if bool is replaced by gnulib are still guaranteed to be 0 and 1, so using it directly as an array index is still safe.
But the thing is that due to -Dbool=char the variable is declared char, not bool.
But if the variable contains exactly 0 or 1, it still makes no difference as an array index whether it was typed bool or char.
Yep, but '--enable-werror' is sad aboot it. Anyways, the code for test-locking is way too old and I don't need it in for anything right now, so I'm ok with keeping everything as-is. Thanks for having a look at it and sorry for wasting your time. Martin
On Wed, Dec 18, 2013 at 06:09:40AM -0700, Eric Blake wrote:
On 12/18/2013 02:35 AM, Martin Kletzander wrote:
When test-locking is enabled, CFLAGS have -Dbool=char, which makes
Huh? bool should never be defined to char in CFLAGS; it should only be replaced if gnulib thinks the compiler is too old.
pipefd[bool] fail (obviously). Forcing the subscript to be bool by double negation fixes the build breaker.
I don't get how this could possibly make a difference. The 'output' variable is declared bool, and then only ever assigned 'false' or 'true', which if bool is replaced by gnulib are still guaranteed to be 0 and 1, so using it directly as an array index is still safe.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I still can't build with '--enable-test-locking' and I'm not sure this is the proper way to fix it. Also, the code wasn't touched for months, so I'm not pushing it as a build-breaker.
NACK. I need more details about the actual failure you are seeing, but this is not the right patch.
The reason for this is that CIL didn't understand the bool type. GNULIB's automagic detection didn't help here because the build is still done by gcc which supports it just fine. We use -save-temps and then CIL analyses those intermediate files and threw up. I'll see if it is still a problem on F20 CIL. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Martin Kletzander