util: check the return value of fcntl in qemu_set_{block, noblock}

Patchew URL: https://patchew.org/QEMU/[email protected]/

Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END ===

libpmem support no
libudev no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled: 'cc'
GEN x86_64-softmmu/config-devices.mak.tmp

The full log is available at
http://patchew.org/logs/[email protected]/testing.docker-quick@centos7/?type=message.

Quoted 2 lines · On Thu, 13 Dec 2018 at 06:58, <[email protected]> wrote:

Patchew URL:

https://patchew.org/QEMU/[email protected]/

Quoted 6 lines

Hi,

This series failed the docker-quick@centos7 build test. Please find the

testing commands and

Quoted 1 line

their output below. If you have Docker installed, you can probably

reproduce it

Quoted 10 lines

locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END ===

libpmem support no
libudev no

Quoted 9 lines

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled: 'cc'
GEN x86_64-softmmu/config-devices.mak.tmp

The full log is available at

http://patchew.org/logs/[email protected]/testing.docker-quick@centos7/?type=message
.

Patchew's attempt to limit the log to only the section with the errors/warnings seems to have misfired here -- it looks like it's picked the first bit of the log with a warning/error rather than extracting all of them, which in this case happens to be the harmless complaint that this build setup doesn't have SDL2 installed.

The actual cause of the failure is much lower down:

GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.

Broken pipe
GTester: last random seed: R02S61a1e35369394d7efb0a0e96d8af615d
GTESTER tests/test-qht-par
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sbf8c21ef5f216840e073ff3e487dedbc
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sfe8cd276fddf3a92891cf274bf88e888
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S7487fc065e22541ba4c78c0db6c61d3c Could not access KVM kernel module: No such file or directory

thanks
-- PMM

Quoted 35 lines · Peter Maydell <[email protected]> 于2018年12月13日周四 下午5:31写道：

Quoted 2 lines · On Thu, 13 Dec 2018 at 06:58, <[email protected]> wrote:

Patchew URL:

https://patchew.org/QEMU/[email protected]/

Quoted 6 lines

Hi,

This series failed the docker-quick@centos7 build test. Please find the

testing commands and

Quoted 1 line

their output below. If you have Docker installed, you can probably

reproduce it

Quoted 10 lines

locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END ===

libpmem support no
libudev no

Quoted 9 lines

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled: 'cc'
GEN x86_64-softmmu/config-devices.mak.tmp

The full log is available at

http://patchew.org/logs/[email protected]/testing.docker-quick@centos7/?type=message
.

Quoted 10 lines

Patchew's attempt to limit the log to only the section with the errors/warnings seems to have misfired here -- it looks like it's picked the first bit of the log with a warning/error rather than extracting all of them, which in this case happens to be the harmless complaint that this build setup doesn't have SDL2 installed.

The actual cause of the failure is much lower down:

Indeed.

Quoted 4 lines

GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.

So here means the fcntl call returns '-1'.
Seems this test have some bugs?

Quoted 6 lines · Li Qiang

Quoted 4 lines · Peter Maydell <[email protected]> 于2018年12月13日周四 下午5:31写道：

Quoted 2 lines · On Thu, 13 Dec 2018 at 06:58, <[email protected]> wrote:

Patchew URL:

https://patchew.org/QEMU/[email protected]/

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Hi,

This series failed the docker-quick@centos7 build test. Please find

the

Quoted 16 lines

Quoted 16 lines

testing commands and

Quoted 1 line

their output below. If you have Docker installed, you can probably

reproduce it

Quoted 13 lines

locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END ===

libpmem support no
libudev no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

NOTE: cross-compilers enabled: 'cc'
GEN x86_64-softmmu/config-devices.mak.tmp

The full log is available at

http://patchew.org/logs/[email protected]/testing.docker-quick@centos7/?type=message

Quoted 15 lines

Quoted 12 lines

.

Patchew's attempt to limit the log to only the section with the errors/warnings seems to have misfired here -- it looks like it's picked the first bit of the log with a warning/error rather than extracting all of them, which in this case happens to be the harmless complaint that this build setup doesn't have SDL2 installed.

The actual cause of the failure is much lower down:

Indeed.

Quoted 7 lines

Quoted 4 lines

GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.

So here means the fcntl call returns '-1'.
Seems this test have some bugs?

Not neccessarily. It means that some code is caller qemu_set_nonblock with a file descriptor for this which is not valid. You'll have to debug what caller is triggering this to understand why

From the bot, seems the error occurs in aarch64 qtest. (I have checked for

It might be as simple as something passing in an FD == -1, and indeed

qemu_set_nonblock forever.

IOW, your change may well break existing
code that is in fact working just fine today.

Regards,
Daniel
--
|: https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-
https://fstop138.berrange.com :|
|: https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|

Assert that the return value is not an error. This is like commit 7e6478e7d4f for qemu_set_cloexec.

Signed-off-by Li Qiang <[email protected]>

---
 util/oslib-posix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

util/oslib-posix.c +0 -0

index c1bee2a581..4ce1ba9ca4 100644

util/oslib-posix.c +5 -1

+    f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);
 }
 
 void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
 }
 
 int socket_set_fast_reuse(int fd)

Signature · 1 line

2.11.0

Quoted 8 lines · Li Qiang

Assert that the return value is not an error. This is like commit 7e6478e7d4f for qemu_set_cloexec.

Signed-off-by Li Qiang <[email protected]>

---
 util/oslib-posix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

util/oslib-posix.c void qemu_set_block(int fd) · line 236

Quoted 10 lines

util/oslib-posix.c +1 -1

index c1bee2a581..4ce1ba9ca4 100644
@@ -233,14 +233,18 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);

This leads to *awful* diagnostics. We need to print something useful when it fails so we stand a chance of understanding what is wrong.

Quoted 10 lines · Daniel P. Berrangé

Quoted 8 lines · Li Qiang

Assert that the return value is not an error. This is like commit 7e6478e7d4f for qemu_set_cloexec.

Signed-off-by Li Qiang <[email protected]>

---
 util/oslib-posix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Quoted 14 lines

util/oslib-posix.c void qemu_set_block(int fd) · line 236

Quoted 10 lines

util/oslib-posix.c +1 -1

index c1bee2a581..4ce1ba9ca4 100644
@@ -233,14 +233,18 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);

This leads to *awful* diagnostics. We need to print something useful when it fails so we stand a chance of understanding what is wrong.

It's the same thing we do in qemu_set_cloexec(), though, and nobody's complained about that that I know of. I think we need to understand whether we're getting asserts in vhost_user_test because of something silly like passing -1 as the fd, or because the fcntl() can legitimately fail. If the former, the assert isn't a big deal because when we hit it in newly developed code the problem is going to be obvious when run under a debugger. If the latter, we need to actually pass out the error status and fix all the callers to check it...

Quoted 26 lines · Peter Maydell

Quoted 10 lines · Daniel P. Berrangé

Quoted 8 lines · Li Qiang

Assert that the return value is not an error. This is like commit 7e6478e7d4f for qemu_set_cloexec.

Signed-off-by Li Qiang <[email protected]>

---
 util/oslib-posix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Quoted 14 lines

util/oslib-posix.c void qemu_set_block(int fd) · line 236

Quoted 10 lines

util/oslib-posix.c +1 -1

index c1bee2a581..4ce1ba9ca4 100644
@@ -233,14 +233,18 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);

This leads to *awful* diagnostics. We need to print something useful when it fails so we stand a chance of understanding what is wrong.

Quoted 10 lines

It's the same thing we do in qemu_set_cloexec(), though, and nobody's complained about that that I know of. I think we need to understand whether we're getting asserts in vhost_user_test because of something silly like passing -1 as the fd, or because the fcntl() can legitimately fail. If the former, the assert isn't a big deal because when we hit it in newly developed code the problem is going to be obvious when run under a debugger. If the latter, we need to actually pass out the error status and fix all the callers to check it...

Yes.

Assertions are not expected to fail *by definition*. When they do, there's a bug in the code, and having to look at the code to see what's wrong is totally fine.

When you feel you have to print something fancy when an assertion fails, either your feelings are misguided, or the assertion is wrong.

The problem with this assertion is that there's many places which call qemu_set_nonblock, so you don't know which code to look at, as we don't know the caller.

Quoted 3 lines · Daniel P. Berrangé

The problem with this assertion is that there's many places which call qemu_set_nonblock, so you don't know which code to look at, as we don't know the caller.

In an ideal world assert would print a stack backtrace :-)

Honestly I'd probably prefer these methods to take an "Error **errp" and propagate to the caller but that's alot more work.