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

PATCH In review Li Qiang 5 participants 9 messages Dec 13, 2018 – Dec 14, 2018

lore

Share & tools

lore

Li Qiang

author

2018-12-13 11:38 UTC #0 lore raw

PATCH v2 1/1 util: check the return value of fcntl in qemu_set_{block, nonblock}

Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs.

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

---
Change since v1: add diagnostics info

 util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

util/oslib-posix.c +32 -5

index c1bee2a581..14cbef1e35 100644
@@ -38,6 +38,7 @@
 #include <libgen.h>
 #include <sys/signal.h>
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>

@@ -233,14 +234,32 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    if (f < 0) {
+        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFL, f & ~O_NONBLOCK) < 0) {
+        error_report("Unable to set blocking flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    if (f < 0) {
+        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFL, f | O_NONBLOCK) < 0) {
+        error_report("Unable to set nonblocking flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 int socket_set_fast_reuse(int fd)

@@ -259,9 +278,17 @@ void qemu_set_cloexec(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFD);
-    assert(f != -1);
-    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
-    assert(f != -1);
+    if (f < 0) {
+        error_report("Unable to get fd flags on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFD, f | FD_CLOEXEC) < 0) {
+        error_report("Unable to set fd close-on-exec flag on fd %d:"
+                    "%s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }
 
 /*

Signature · 1 line

2.11.0

Peter Maydell

2018-12-13 11:45 UTC #1 lore raw

Quoted 7 lines · Li Qiang

Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs.
Signed-off-by Li Qiang <[email protected]>
---
Change since v1: add diagnostics info

We need to fix the assert in the test suite first...

thanks
-- PMM

Daniel P. Berrangé

2018-12-13 11:48 UTC #2 lore raw

Quoted 10 lines · Peter Maydell

Quoted 7 lines · Li Qiang
Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs.
Signed-off-by Li Qiang <[email protected]>
---
Change since v1: add diagnostics info
We need to fix the assert in the test suite first...

Hopefully the improved diagnostics in this v2 will give us a better idea of what the test suite failure is likely to be when patchew reports in....

Regards,
Daniel

Signature · 3 lines

|: 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 :|

Li Qiang

author

2018-12-13 11:50 UTC #3 lore raw

In reply to Peter Maydell

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

Quoted 7 lines · Li Qiang
Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs.
Signed-off-by Li Qiang <[email protected]>
---
Change since v1: add diagnostics info
We need to fix the assert in the test suite first...

Agree, as I mentioned in the last email, this assert seems occurs in aarch64.
But I have no this environment. So this patch add diagnostics info to let the bot tell us
what happen.

Thanks,
Li Qiang

Quoted 3 lines

thanks
-- PMM

Markus Armbruster

2018-12-13 12:38 UTC #4 lore raw

In reply to Li Qiang

Quoted 28 lines · Li Qiang

Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs.

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

---
Change since v1: add diagnostics info

 util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

util/oslib-posix.c +2 -1

index c1bee2a581..14cbef1e35 100644
@@ -38,6 +38,7 @@
 #include <libgen.h>
 #include <sys/signal.h>
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>

@@ -233,14 +234,32 @@ void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    if (f < 0) {

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

Quoted 10 lines

util/oslib-posix.c +9 -0

+        error_report("Unable to get file status flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
+    if (fcntl(fd, F_SETFL, f & ~O_NONBLOCK) < 0) {
+        error_report("Unable to set blocking flag on fd %d: %s(errno=%d)",
+                    fd, strerror(errno), errno);
+        abort();
+    }
 }

If this is a programming error, i.e. one that can happen only when the function gets misused, then abort() is appropriate, but error_report() is putting lipstick on the pig.

If it's not a programming error, but something outside QEMU misbehaves (user error, network outage, ...), then error_report() is appropriate, but abort() is not. exit(1) then.

[...]

[email protected]

bot

2018-12-13 16:02 UTC #5 lore raw

In reply to Li Qiang

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.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]

Li Qiang

author

2018-12-14 01:47 UTC #6 lore raw

Hi all,

Here is the error.

GTESTER check-qtest-x86_64
Unable to get file status flag on fd 21860: Bad file descriptor(errno=9)
GTESTER check-qtest-aarch64
Broken pipe
GTester: last random seed: R02S3f0d6981dd97231d06e0b2966baf94b9 Unable to get file status flag on fd 21965: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S29fde958e7ee4c26c4f295ff4dbd47d4 Unable to get file status flag on fd 21890: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S6d074187e5c8501255c96b247f5c8e3f Unable to get file status flag on fd 21923: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S446127f38eb9e8b4f181e6fc95026ba0
GTESTER tests/test-qht-par
Could not access KVM kernel module: No such file or directory

The fd '21860' '21965', '21890', '21923' is a little strange. Following is my guess.

21280 --> 0x5564
21965 --> 0x55CD
21890 --> 0x5582
21923 --> 0x55A3

Seems they are stack uninitialized value which 'fd's memory holds. Seems 'qemu_chr_fe_get_msgfds' first failed, then the 'fd' is an uninitialized value
cause my first patch 'assert' fails.

Thanks,
Li Qiang

Quoted 35 lines · <[email protected]> 于2018年12月14日周五上午12:01写道：

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
.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]

Li Qiang

author

2018-12-14 09:16 UTC #7 lore raw

Quoted 30 lines · Li Qiang <[email protected]> 于2018年12月14日周五上午9:46写道：

Hi all,
Here is the error.
GTESTER check-qtest-x86_64
Unable to get file status flag on fd 21860: Bad file descriptor(errno=9)
GTESTER check-qtest-aarch64
Broken pipe
GTester: last random seed: R02S3f0d6981dd97231d06e0b2966baf94b9 Unable to get file status flag on fd 21965: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S29fde958e7ee4c26c4f295ff4dbd47d4 Unable to get file status flag on fd 21890: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S6d074187e5c8501255c96b247f5c8e3f Unable to get file status flag on fd 21923: Bad file descriptor(errno=9) Broken pipe
GTester: last random seed: R02S446127f38eb9e8b4f181e6fc95026ba0
GTESTER tests/test-qht-par
Could not access KVM kernel module: No such file or directory
The fd '21860' '21965', '21890', '21923' is a little strange. Following is my guess.
21280 --> 0x5564
21965 --> 0x55CD
21890 --> 0x5582
21923 --> 0x55A3

Quoted 6 lines

Seems they are stack uninitialized value which 'fd's memory holds. Seems 'qemu_chr_fe_get_msgfds' first failed, then the 'fd' is an uninitialized value
cause my first patch 'assert' fails.

First of all I want to know does the following error means? doesn't it mean "the x86 qtest is ok and aarch64 is not ok"?

GTESTER check-qtest-x86_64
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

This issue is caused by 'chr_read' in vhost-user-test.c processes 'VHOST_USER_SET_VRING_CALL'. qemu_chr_fe_get_msgfds->tcp_get_msgfds

static int tcp_get_msgfds(Chardev *chr, int *fds, int num)

{
    SocketChardev *s = SOCKET_CHARDEV(chr);

    int to_copy = (s->read_msgfds_num < num) ? s->read_msgfds_num : num;

    assert(num <= TCP_MAX_FDS);

    if (to_copy) {
        int i;

        memcpy(fds, s->read_msgfds, to_copy * sizeof(int));

        /* Close unused fds */
        for (i = to_copy; i < s->read_msgfds_num; i++) {
            close(s->read_msgfds[i]);
        }

        g_free(s->read_msgfds);
        s->read_msgfds = 0;
        s->read_msgfds_num = 0;
    }

    return to_copy;
}

The 's->read_msgfds_num' is 0, so qemu_chr_fe_get_msgfds returns 0, and 'fd' be an uninitialized stack value.

The reason why 's->read_msgfds_num' is 0 is that in 'vhost_set_vring_file'. the 'ioeventfd_enabled()' returns 0 and 'kvm_eventfds_allowed' is 0 which means the
kernel doesn't support eventfd.

static int vhost_set_vring_file(struct vhost_dev *dev,
VhostUserRequest request, struct vhost_vring_file *file)
{

    if (ioeventfd_enabled() && file->fd > 0) {
        fds[fd_num++] = file->fd;
    } else {
        msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
    }
  ...
    return 0;
}

After I look at the kernel, the arm platform seems doesn't support eventfd.

So if my understanding is correct in the first, '"the x86 qtest is ok and aarch64 is not ok".
This can be explainable. But if not, there maybe another issue.

Since this is related with vhost-user-test, Cc Jason and Michael .

Thanks,
Li Qiang

Quoted 44 lines

Thanks,
Li Qiang
Quoted 35 lines · <[email protected]> 于2018年12月14日周五上午12:01写道：
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
.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]

Peter Maydell

2018-12-14 09:49 UTC #8 lore raw

Quoted 4 lines · Li Qiang

First of all I want to know does the following error means? doesn't it mean "the x86 qtest is ok and aarch64 is not ok"?

Quoted 16 lines

GTESTER check-qtest-x86_64
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

I think maybe the make output is confusing you here, because it's running both qtests in parallel. We don't ever run the vhost-user-test for arm guests: tests/Makefile adds it only to check-qtest-i386 and check-qtest-x86_64. So I think all the warnings/errors here are coming from the x86 test.

Quoted 7 lines

the 'ioeventfd_enabled()' returns 0 and 'kvm_eventfds_allowed' is 0 which means the kernel doesn't support eventfd.
After I look at the kernel, the arm platform seems doesn't support eventfd.
So if my understanding is correct in the first, '"the x86 qtest is ok and aarch64 is not ok". This can be explainable. But if not, there maybe another issue.

It looks to me from the investigation you did that the vhost-user-test is assuming that we are using KVM and ioeventfds, and is misbehaving when that is not true. We should fix the test so that either it works in that configuration, or correctly skips doing the test if it is not applicable in a different config.

thanks
-- PMM