| Message ID | 1544701071-2922-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] util: check the return value of fcntl in qemu_set_{block, nonblock} | expand |
On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote: > > Also add diagnostics info in 'qemu_set_cloexec' so that we can know > what happen when error occurs. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > Change since v1: add diagnostics info We need to fix the assert in the test suite first... thanks -- PMM
On Thu, Dec 13, 2018 at 11:45:22AM +0000, Peter Maydell wrote: > On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote: > > > > Also add diagnostics info in 'qemu_set_cloexec' so that we can know > > what happen when error occurs. > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > 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
Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午7:45写道： > On Thu, 13 Dec 2018 at 11:37, Li Qiang <liq3ea@gmail.com> wrote: > > > > Also add diagnostics info in 'qemu_set_cloexec' so that we can know > > what happen when error occurs. > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > 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 > thanks > -- PMM >
Li Qiang <liq3ea@gmail.com> writes: > Also add diagnostics info in 'qemu_set_cloexec' so that we can know > what happen when error occurs. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > Change since v1: add diagnostics info > > util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 5 deletions(-) > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index c1bee2a581..14cbef1e35 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -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(); > + } > } 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. [...]
Patchew URL: https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/ 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
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 <no-reply@patchew.org> 于2018年12月14日周五 上午12:01写道： > Patchew URL: > https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/ > > > > 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message > . > --- > Email generated automatically by Patchew [http://patchew.org/]. > Please send your feedback to patchew-devel@redhat.com
Li Qiang <liq3ea@gmail.com> 于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 > > 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 > Thanks, > Li Qiang > > > > <no-reply@patchew.org> 于2018年12月14日周五 上午12:01写道： > >> Patchew URL: >> https://patchew.org/QEMU/1544701071-2922-1-git-send-email-liq3ea@gmail.com/ >> >> >> >> 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/1544701071-2922-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message >> . >> --- >> Email generated automatically by Patchew [http://patchew.org/]. >> Please send your feedback to patchew-devel@redhat.com > >
On Fri, 14 Dec 2018 at 09:16, Li Qiang <liq3ea@gmail.com> wrote: > 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 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. > 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
diff --git a/util/oslib-posix.c b/util/oslib-posix.c index c1bee2a581..14cbef1e35 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -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(); + } } /*
Also add diagnostics info in 'qemu_set_cloexec' so that we can know what happen when error occurs. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- Change since v1: add diagnostics info util/oslib-posix.c | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-)