| Message ID | 1544666977-4816-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | util: check the return value of fcntl in qemu_set_{block, noblock} | expand |
Patchew URL: https://patchew.org/QEMU/1544666977-4816-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/1544666977-4816-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 Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote: > > Patchew URL: https://patchew.org/QEMU/1544666977-4816-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/1544666977-4816-1-git-send-email-liq3ea@gmail.com/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
Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道： > On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote: > > > > Patchew URL: > https://patchew.org/QEMU/1544666977-4816-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/1544666977-4816-1-git-send-email-liq3ea@gmail.com/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: > > Indeed. > 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? Thanks, Li Qiang > 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 >
On Thu, Dec 13, 2018 at 05:56:24PM +0800, Li Qiang wrote: > Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道： > > > On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote: > > > > > > Patchew URL: > > https://patchew.org/QEMU/1544666977-4816-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/1544666977-4816-1-git-send-email-liq3ea@gmail.com/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: > > > > > Indeed. > > > > 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 It might be as simple as something passing in an FD == -1, and indeed I fear this is quite likely as we've been ignoring errors from qemu_set_nonblock forever. IOW, your change may well break existing code that is in fact working just fine today. Regards, Daniel
On Wed, Dec 12, 2018 at 06:09:37PM -0800, Li Qiang wrote: > Assert that the return value is not an error. This is like commit > 7e6478e7d4f for qemu_set_cloexec. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > util/oslib-posix.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index c1bee2a581..4ce1ba9ca4 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -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. 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(); } > + 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) > -- > 2.11.0 > > Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> 于2018年12月13日周四 下午6:17写道： > On Thu, Dec 13, 2018 at 05:56:24PM +0800, Li Qiang wrote: > > Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道： > > > > > On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote: > > > > > > > > Patchew URL: > > > > https://patchew.org/QEMU/1544666977-4816-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/1544666977-4816-1-git-send-email-liq3ea@gmail.com/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: > > > > > > > > Indeed. > > > > > > > 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 x86_64, no this error). 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 > > It might be as simple as something passing in an FD == -1, and indeed I fear this is quite likely as we've been ignoring errors from > qemu_set_nonblock forever. Maybe, but if so, we need fix this(check 'fd' before calling 'qemu_set_{block，nonblock, cloexec}'). I think check 'fd' should be done before calling 'qemu_set_{block，nonblock, cloexec}', the caller of these function should be responsible for the valid of 'fd'. I will try to add some diagnostics info in the revised patch so that the bot can give a more detailed information. Thanks, Li Qiang > 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 :| >
On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Dec 12, 2018 at 06:09:37PM -0800, Li Qiang wrote: > > Assert that the return value is not an error. This is like commit > > 7e6478e7d4f for qemu_set_cloexec. > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > util/oslib-posix.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > > index c1bee2a581..4ce1ba9ca4 100644 > > --- a/util/oslib-posix.c > > +++ b/util/oslib-posix.c > > @@ -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... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> On Wed, Dec 12, 2018 at 06:09:37PM -0800, Li Qiang wrote: >> > Assert that the return value is not an error. This is like commit >> > 7e6478e7d4f for qemu_set_cloexec. >> > >> > Signed-off-by: Li Qiang <liq3ea@gmail.com> >> > --- >> > util/oslib-posix.c | 8 ++++++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> > index c1bee2a581..4ce1ba9ca4 100644 >> > --- a/util/oslib-posix.c >> > +++ b/util/oslib-posix.c >> > @@ -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... 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.
On Thu, Dec 13, 2018 at 01:28:23PM +0100, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> wrote: > >> > >> On Wed, Dec 12, 2018 at 06:09:37PM -0800, Li Qiang wrote: > >> > Assert that the return value is not an error. This is like commit > >> > 7e6478e7d4f for qemu_set_cloexec. > >> > > >> > Signed-off-by: Li Qiang <liq3ea@gmail.com> > >> > --- > >> > util/oslib-posix.c | 8 ++++++-- > >> > 1 file changed, 6 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > >> > index c1bee2a581..4ce1ba9ca4 100644 > >> > --- a/util/oslib-posix.c > >> > +++ b/util/oslib-posix.c > >> > @@ -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... > > 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. 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. > When you feel you have to print something fancy when an assertion fails, > either your feelings are misguided, or the assertion is wrong. Honestly I'd probably prefer these methods to take an "Error **errp" and propagate to the caller but that's alot more work. Regards, Daniel
On Thu, 13 Dec 2018 at 12:39, Daniel P. Berrangé <berrange@redhat.com> wrote: > 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 :-) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 13 Dec 2018 at 12:39, Daniel P. Berrangé <berrange@redhat.com> wrote: >> 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 :-) That feature would make even me make peace with g_assert() ;)
Daniel P. Berrangé <berrange@redhat.com> writes: [...] > Honestly I'd probably prefer these methods to take an "Error **errp" > and propagate to the caller but that's alot more work. I'd rather return 0 on success, -errno on failure in these cases.
diff --git a/util/oslib-posix.c b/util/oslib-posix.c index c1bee2a581..4ce1ba9ca4 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -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); + 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)
Assert that the return value is not an error. This is like commit 7e6478e7d4f for qemu_set_cloexec. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- util/oslib-posix.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)