[libvirt] [PATCH] build: work around gcc 6.0 warnings
gcc 6.0 added an annoying warning: fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 So work around it by further complicating the logic to thwart the compiler. Signed-off-by: Eric Blake <eblake@redhat.com> --- This is a build-breaker fix for rawhide; but I'll wait for a day for any reasons why I should not push it during freeze. src/fdstream.c | 8 +++++--- src/rpc/virnetsshsession.c | 10 +++++++--- src/security/security_selinux.c | 5 +++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..1c34321 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,7 +1,7 @@ /* * fdstream.c: generic streams impl for file descriptors * - * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. + * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -387,7 +387,8 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) retry: ret = write(fdst->fd, bytes, nbytes); if (ret < 0) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { + if (errno == EAGAIN || + (EAGAIN != EWOULDBLOCK && errno == EWOULDBLOCK)) { ret = -2; } else if (errno == EINTR) { goto retry; @@ -437,7 +438,8 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) retry: ret = read(fdst->fd, bytes, nbytes); if (ret < 0) { - if (errno == EAGAIN || errno == EWOULDBLOCK) { + if (errno == EAGAIN || + (EAGAIN != EWOULDBLOCK && errno == EWOULDBLOCK)) { ret = -2; } else if (errno == EINTR) { goto retry; diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 406a831..d7d1c1a 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -1,7 +1,7 @@ /* * virnetsshsession.c: ssh network transport provider based on libssh2 * - * Copyright (C) 2012-2013 Red Hat, Inc. + * Copyright (C) 2012-2013, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -546,7 +546,9 @@ virNetSSHAuthenticateAgent(virNetSSHSessionPtr sess, return 0; /* key accepted */ if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && - ret != LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED && + (LIBSSH2_ERROR_AUTHENTICATION_FAILED != + LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED && + ret != LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED) && ret != LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { libssh2_session_last_error(sess->session, &errmsg, NULL, 0); virReportError(VIR_ERR_AUTH_FAILED, @@ -674,7 +676,9 @@ virNetSSHAuthenticatePrivkey(virNetSSHSessionPtr sess, priv->filename, errmsg); if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || - ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) + (LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED != + LIBSSH2_ERROR_AUTHENTICATION_FAILED && + ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)) return 1; else return -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 26d95d1..25f0bdf 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2014 Red Hat, Inc. + * Copyright (C) 2008-2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -911,7 +911,8 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, * hopefully sets one of the necessary SELinux virt_use_{nfs,usb,pci} * boolean tunables to allow it ... */ - if (setfilecon_errno != EOPNOTSUPP && setfilecon_errno != ENOTSUP && + if (setfilecon_errno != EOPNOTSUPP && + (EOPNOTSUPP != ENOTSUP && setfilecon_errno != ENOTSUP) && setfilecon_errno != EROFS) { virReportSystemError(setfilecon_errno, _("unable to set security context '%s' on '%s'"), -- 2.5.0
On 24.02.2016 22:29, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
This is a build-breaker fix for rawhide; but I'll wait for a day for any reasons why I should not push it during freeze.
src/fdstream.c | 8 +++++--- src/rpc/virnetsshsession.c | 10 +++++++--- src/security/security_selinux.c | 5 +++-- 3 files changed, 15 insertions(+), 8 deletions(-)
I'm having some difficulties applying this: Applying: build: work around gcc 6.0 warnings Using index info to reconstruct a base tree... error: patch failed: src/fdstream.c:1 error: src/fdstream.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 build: work around gcc 6.0 warnings Could it be that my git is 'only' 2.4.10 while yours is 2.5.0? Michal
On Thu, 2016-02-25 at 09:22 +0100, Michal Privoznik wrote:
I'm having some difficulties applying this:
Applying: build: work around gcc 6.0 warnings Using index info to reconstruct a base tree... error: patch failed: src/fdstream.c:1 error: src/fdstream.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 build: work around gcc 6.0 warnings
Could it be that my git is 'only' 2.4.10 while yours is 2.5.0?
That's probably not it, I have 2.5.0 and I get the same error when trying to apply it with 'git am'. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Feb 25, 2016 at 09:52:12 +0100, Andrea Bolognani wrote:
On Thu, 2016-02-25 at 09:22 +0100, Michal Privoznik wrote:
I'm having some difficulties applying this:
Applying: build: work around gcc 6.0 warnings Using index info to reconstruct a base tree... error: patch failed: src/fdstream.c:1 error: src/fdstream.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 build: work around gcc 6.0 warnings
Could it be that my git is 'only' 2.4.10 while yours is 2.5.0?
That's probably not it, I have 2.5.0 and I get the same error when trying to apply it with 'git am'.
Well, simply the patch context is broken in the first hunk: The first hunk in the mail: diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..1c34321 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,7 +1,7 @@ /* * fdstream.c: generic streams impl for file descriptors * - * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. + * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public While the top of src/fdstream.c: /* * fdstream.c: generic streams impl for file descriptors * * Copyright (C) 2009-2012, 2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * So the first hunk in the patch either changes a whitespace that is not visible or was edited manually and is wrong. Additionally it does not match the context in the file.
On 02/25/2016 02:37 AM, Peter Krempa wrote:
On Thu, Feb 25, 2016 at 09:52:12 +0100, Andrea Bolognani wrote:
On Thu, 2016-02-25 at 09:22 +0100, Michal Privoznik wrote:
I'm having some difficulties applying this:
Well, simply the patch context is broken in the first hunk:
The first hunk in the mail:
diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..1c34321 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,7 +1,7 @@ /* * fdstream.c: generic streams impl for file descriptors * - * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. + * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc.
D'oh. I blame emacs - I have it set to remind me to automatically update copyright, and must have accidentally said yes to updating the diff. I'll need to teach my hook to do nothing on diff files. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Feb 25, 2016 at 06:46:55AM -0700, Eric Blake wrote:
On 02/25/2016 02:37 AM, Peter Krempa wrote:
On Thu, Feb 25, 2016 at 09:52:12 +0100, Andrea Bolognani wrote:
On Thu, 2016-02-25 at 09:22 +0100, Michal Privoznik wrote:
I'm having some difficulties applying this:
Well, simply the patch context is broken in the first hunk:
The first hunk in the mail:
diff --git a/src/fdstream.c b/src/fdstream.c index a85cf9d..1c34321 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,7 +1,7 @@ /* * fdstream.c: generic streams impl for file descriptors * - * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc. + * Copyright (C) 2009-2012, 2014, 2016 Red Hat, Inc.
D'oh. I blame emacs - I have it set to remind me to automatically update copyright, and must have accidentally said yes to updating the diff. I'll need to teach my hook to do nothing on diff files.
s/on diff files// ;) Jan
On Thu, Feb 25, 2016 at 14:52:16 +0100, Ján Tomko wrote:
On Thu, Feb 25, 2016 at 06:46:55AM -0700, Eric Blake wrote:
D'oh. I blame emacs - I have it set to remind me to automatically update copyright, and must have accidentally said yes to updating the diff. I'll need to teach my hook to do nothing on diff files.
s/on diff files// ;)
+1
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Personally I don't like this approach. Why don't we take that 6 line reproducer from GCC BZ and create a m4 macro that will detect that bug in GCC and turn off the logical-op check? For me this solution is much cleaner than introducing some ugly workaround in our code just because GCC has a bug. Pavel
On 02/25/2016 04:15 AM, Pavel Hrdina wrote:
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Personally I don't like this approach. Why don't we take that 6 line reproducer from GCC BZ and create a m4 macro that will detect that bug in GCC and turn off the logical-op check? For me this solution is much cleaner than introducing some ugly workaround in our code just because GCC has a bug.
Killing it for the entire tree seems like a rather heavy hammer. I'd rather just add #pragma push/pop to ignore the warning around the few places that it incorrectly triggers, leaving the rest of the tree still protected (as it does catch real bugs, when we aren't dealing with sometimes-aliased values coming from headers). I'll work up a v2 along those lines, so we can compare the two approaches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 25.02.2016 14:48, Eric Blake wrote:
On 02/25/2016 04:15 AM, Pavel Hrdina wrote:
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Personally I don't like this approach. Why don't we take that 6 line reproducer from GCC BZ and create a m4 macro that will detect that bug in GCC and turn off the logical-op check? For me this solution is much cleaner than introducing some ugly workaround in our code just because GCC has a bug.
Killing it for the entire tree seems like a rather heavy hammer. I'd rather just add #pragma push/pop to ignore the warning around the few places that it incorrectly triggers, leaving the rest of the tree still protected (as it does catch real bugs, when we aren't dealing with sometimes-aliased values coming from headers).
Since not everybody is switching to gcc 6.0 (I'm stuck with 4.9.3 for a while now) we are still going to be able to catch those errors. Moreover, if the gcc bug is ever fixed we don't need to undo those pragmas. Michal
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
This is a build-breaker fix for rawhide; but I'll wait for a day for any reasons why I should not push it during freeze.
It looks like you're still talking this over and thinking about approaches. But could we push the fix for the time being so that the release is nicely buildable and then work on making it nicer later? I'd vote ACK for that. Martin
On Mon, 2016-02-29 at 15:41 +0100, Martin Kletzander wrote:
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning: fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 So work around it by further complicating the logic to thwart the compiler. Signed-off-by: Eric Blake <eblake@redhat.com> --- This is a build-breaker fix for rawhide; but I'll wait for a day for any reasons why I should not push it during freeze. It looks like you're still talking this over and thinking about approaches. But could we push the fix for the time being so that the release is nicely buildable and then work on making it nicer later? I'd vote ACK for that.
Update: the libvirt-fedora-rawhide build host on CentOS CI has been updated and it's now running gcc 6.0. The immediate result is that build jobs have started to fail (see [1] for an example). Looking at the gcc bug, it doesn't look like there's been much movement in that area, so I think we should take some action on our side... Eric, you mentioned proposing a v2 that would use #pragma instead of uglifying the current checks. Do you still intend to work on such alternative approach? Failing that, I'm personally okay with pushing this. Cheers. [1] https://ci.centos.org/view/libvirt-project/job/libvirt-daemon-build/1131/ -- Andrea Bolognani Software Engineer - Virtualization Team
On 04/08/2016 12:27 PM, Andrea Bolognani wrote:
On Mon, 2016-02-29 at 15:41 +0100, Martin Kletzander wrote:
On Wed, Feb 24, 2016 at 02:29:58PM -0700, Eric Blake wrote:
gcc 6.0 added an annoying warning:
fdstream.c: In function 'virFDStreamWrite': fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~ fdstream.c: In function 'virFDStreamRead': fdstream.c:440:29: error: logical 'or' of equal expressions [-Werror=logical-op] if (errno == EAGAIN || errno == EWOULDBLOCK) { ^~
This makes it impossible to build out-of-the-box on rawhide, and we aren't guaranteed that the gcc bug will be fixed in a timely manner: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
So work around it by further complicating the logic to thwart the compiler.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
This is a build-breaker fix for rawhide; but I'll wait for a day for any reasons why I should not push it during freeze.
It looks like you're still talking this over and thinking about approaches. But could we push the fix for the time being so that the release is nicely buildable and then work on making it nicer later? I'd vote ACK for that.
Update: the libvirt-fedora-rawhide build host on CentOS CI has been updated and it's now running gcc 6.0. The immediate result is that build jobs have started to fail (see [1] for an example).
Looking at the gcc bug, it doesn't look like there's been much movement in that area, so I think we should take some action on our side...
Eric, you mentioned proposing a v2 that would use #pragma instead of uglifying the current checks. Do you still intend to work on such alternative approach?
I've attached a patch with the pragma push/pop approach Eric mentioned. Unfortunately it doesn't work as is on RHEL6 vintage gcc so it's probably not a realistic option.
Failing that, I'm personally okay with pushing this.
I agree - Cole
participants (8)
-
Andrea Bolognani -
Cole Robinson -
Eric Blake -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa