[libvirt] Dealing with GCC 6.0
So we recently started compiling with gcc 5 and there's a new thing now and we don't build on gcc 6. I know it's just a development build, but it finds new things, like: 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) { ^~ and rpc/virnetsshsession.c: In function 'virNetSSHAuthenticateAgent': rpc/virnetsshsession.c:548:56: error: logical 'and' of equal expressions [-Werror=logical-op] if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ^~ rpc/virnetsshsession.c: In function 'virNetSSHAuthenticatePrivkey': rpc/virnetsshsession.c:676:57: error: logical 'or' of equal expressions [-Werror=logical-op] if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ^~ The first one is prety easy to fix, that's the only place where we use EWOULDBLOCK, we are using only EAGAIN everywhere else. However, the second one is not that easy to change for me as I'm not a libssh expert and moreover, I would like to know other's opinions on how to tackle that. We can disable the logical-op warning, but it might show something that makes sense (really?). Or we can do bunch of very very ugly "#if"s. Anyone care to share an idea? Have a nice day, Martin
On 02/01/2016 07:37 AM, Martin Kletzander wrote:
So we recently started compiling with gcc 5 and there's a new thing now and we don't build on gcc 6. I know it's just a development build, but it finds new things, like:
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) { ^~
POSIX allows EAGAIN and EWOULDBLOCK to be separate values; probably the best fix for this would be some sort of macro or inline function that does something like: static int is_restartable(int err) { if (err == EAGAIN) return true; if (err == EWOULDBLOCK) return true; return false; } then using is_restartable(errno) rather than open-coding everywhere else that we compare against EAGAIN and where POSIX says EWOULDBLOCK may be returned.
and
rpc/virnetsshsession.c: In function 'virNetSSHAuthenticateAgent': rpc/virnetsshsession.c:548:56: error: logical 'and' of equal expressions [-Werror=logical-op] if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ^~ rpc/virnetsshsession.c: In function 'virNetSSHAuthenticatePrivkey': rpc/virnetsshsession.c:676:57: error: logical 'or' of equal expressions [-Werror=logical-op] if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ^~
More context: if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ret != LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED && ret != LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) so depending on the libssh2 build, PUBLICKEY_UNRECOGNIZED and AUTHENTICATION_FAILED must be the same value. Again, something where we could write an inline wrapper to split up the checks to bypass the -Wlogical-op stupidity without making the code too unreadable.
The first one is prety easy to fix, that's the only place where we use EWOULDBLOCK, we are using only EAGAIN everywhere else.
Dropping the 'errno == EWOULDBLOCK' branch would be a bug.
However, the second one is not that easy to change for me as I'm not a libssh expert and moreover, I would like to know other's opinions on how to tackle that. We can disable the logical-op warning, but it might show something that makes sense (really?). Or we can do bunch of very very ugly "#if"s.
Anyone care to share an idea?
I've filed a gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 If it's only rawhide that is failing, the next question is whether rawhide is using unreleased gcc on purpose to find problems like this before gcc releases, or whether anyone using stock gcc 6.0 tarball will face the same problem. Disabling -Wlogical-op feels like too heavy of a hammer, especially if my idea to break the checks up into multiple 'if's in an inline wrapper function works. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 01, 2016 at 03:14:04PM -0700, Eric Blake wrote:
On 02/01/2016 07:37 AM, Martin Kletzander wrote:
So we recently started compiling with gcc 5 and there's a new thing now and we don't build on gcc 6. I know it's just a development build, but it finds new things, like:
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) { ^~
POSIX allows EAGAIN and EWOULDBLOCK to be separate values; probably the best fix for this would be some sort of macro or inline function that does something like:
static int is_restartable(int err) { if (err == EAGAIN) return true; if (err == EWOULDBLOCK) return true; return false; }
then using is_restartable(errno) rather than open-coding everywhere else that we compare against EAGAIN and where POSIX says EWOULDBLOCK may be returned.
and
rpc/virnetsshsession.c: In function 'virNetSSHAuthenticateAgent': rpc/virnetsshsession.c:548:56: error: logical 'and' of equal expressions [-Werror=logical-op] if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ^~ rpc/virnetsshsession.c: In function 'virNetSSHAuthenticatePrivkey': rpc/virnetsshsession.c:676:57: error: logical 'or' of equal expressions [-Werror=logical-op] if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ^~
More context:
if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED && ret != LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED && ret != LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) {
if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
so depending on the libssh2 build, PUBLICKEY_UNRECOGNIZED and AUTHENTICATION_FAILED must be the same value. Again, something where we could write an inline wrapper to split up the checks to bypass the -Wlogical-op stupidity without making the code too unreadable.
The first one is prety easy to fix, that's the only place where we use EWOULDBLOCK, we are using only EAGAIN everywhere else.
Dropping the 'errno == EWOULDBLOCK' branch would be a bug.
That means we should use EWOULDBLOCK in other places where we check for EAGAIN only as well, right? Or only where the read/write can be done through sockets.
However, the second one is not that easy to change for me as I'm not a libssh expert and moreover, I would like to know other's opinions on how to tackle that. We can disable the logical-op warning, but it might show something that makes sense (really?). Or we can do bunch of very very ugly "#if"s.
Anyone care to share an idea?
I've filed a gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
If it's only rawhide that is failing, the next question is whether rawhide is using unreleased gcc on purpose to find problems like this before gcc releases, or whether anyone using stock gcc 6.0 tarball will face the same problem. Disabling -Wlogical-op feels like too heavy of a hammer, especially if my idea to break the checks up into multiple 'if's in an inline wrapper function works.
I should've dug more into this, thanks for doing it instead of me. Although I wouldn't describe the behaviour so nicely in the bug as you did. Let's hope this will be fixed in GCC as that is, as I said, only a rawhide devel build, there is no release yet, at least not to my knowledge. Have a nice day, Martin
On Mon, Feb 01, 2016 at 03:37:07PM +0100, Martin Kletzander wrote:
So we recently started compiling with gcc 5 and there's a new thing now and we don't build on gcc 6. I know it's just a development build, but it finds new things, like:
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) { ^~
FWIW this was fixed in libguestfs by rewriting the expression as: + if (errno == EWOULDBLOCK || + (EWOULDBLOCK != EAGAIN && errno == EAGAIN)) (commit b12f0a809f7d1aef9215d25bda2f0a2fd955d0f8). I think the warning itself is pretty useless. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 02/02/2016 03:12 AM, Richard W.M. Jones wrote:
On Mon, Feb 01, 2016 at 03:37:07PM +0100, Martin Kletzander wrote:
So we recently started compiling with gcc 5 and there's a new thing now and we don't build on gcc 6. I know it's just a development build, but it finds new things, like:
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) { ^~
FWIW this was fixed in libguestfs by rewriting the expression as:
+ if (errno == EWOULDBLOCK || + (EWOULDBLOCK != EAGAIN && errno == EAGAIN))
Yep, this works. I'll probably push something like this under the build-breaker rule, so that our pending release builds out of the box on current rawhide. I'm also seeing failures on rawhide due to the wireshark problems (should be fixed within 24 hours once http://koji.fedoraproject.org/koji/taskinfo?taskID=13106963 lands) and due to this: ../../tests/securityselinuxhelper.c:307:24: error: conflicting types for 'selabel_open' struct selabel_handle *selabel_open(unsigned int backend, ^~~~~~~~~~~~ In file included from ../../tests/securityselinuxhelper.c:32:0: /usr/include/selinux/label.h:73:24: note: previous declaration of 'selabel_open' was here struct selabel_handle *selabel_open(unsigned int backend, ^~~~~~~~~~~~ which doesn't say which parameter type changed in the latest selinux headers, but also something we'll want to patch sooner rather than later. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Martin Kletzander -
Richard W.M. Jones