| Message ID | 20190115200252.25911-8-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/49] pci/pcie: stop plug/unplug if the slot is locked | expand |
Hi, On 1/15/19 9:04 PM, Michael S. Tsirkin wrote: > From: Li Qiang <liq3ea@163.com> > > Assert that the return value is not an error. This is like commit > 7e6478e7d4f for qemu_set_cloexec. > > Signed-off-by: Li Qiang <liq3ea@163.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.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); > + 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); This commit breaks OpenBSD, when trying to start QEMU I get: assertion "f != -1" failed: file "util/oslib-posix.c", line 247, function "qemu_set_nonblock" Having a quick look at gdb, the last device opened is /dev/null, and when fcntl() fails we have errno = ENODEV. 19 ENODEV Operation not supported by device. An attempt was made to apply an inappropriate function to a device, for example, trying to read a write-only device such as a printer. Digging further I found a recent commit which could fix this problem: https://github.com/openbsd/src/commit/c2a35b387f9d3c "fcntl(F_SETFL) invokes the FIONBIO and FIOASYNC ioctls internally, so the memory devices (/dev/null, /dev/zero, etc) need to permit them." Brad: Do you think this might be the fix? If so, any idea what is the first release to contain this fix? I don't know OpenBSD and can't figure this out... Also, what would be the cleaner QEMU fix? Thanks, Phil. > } > > int socket_set_fast_reuse(int fd) >
On 25.01.2019 19:53, Philippe Mathieu-Daudé wrote: > Hi, > > On 1/15/19 9:04 PM, Michael S. Tsirkin wrote: >> From: Li Qiang <liq3ea@163.com> >> >> Assert that the return value is not an error. This is like commit >> 7e6478e7d4f for qemu_set_cloexec. >> >> Signed-off-by: Li Qiang <liq3ea@163.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.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); >> + 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); > > This commit breaks OpenBSD, when trying to start QEMU I get: > assertion "f != -1" failed: file "util/oslib-posix.c", line 247, > function "qemu_set_nonblock" > > Having a quick look at gdb, the last device opened is /dev/null, and > when fcntl() fails we have errno = ENODEV. > > 19 ENODEV Operation not supported by device. > An attempt was made to apply an inappropriate function to a device, > for example, trying to read a write-only device such as a printer. > > Digging further I found a recent commit which could fix this problem: > https://github.com/openbsd/src/commit/c2a35b387f9d3c > "fcntl(F_SETFL) invokes the FIONBIO and FIOASYNC ioctls internally, so > the memory devices (/dev/null, /dev/zero, etc) need to permit them." > > Brad: Do you think this might be the fix? If so, any idea what is the > first release to contain this fix? I don't know OpenBSD and can't figure > this out... Also, what would be the cleaner QEMU fix? > > Thanks, > I cannot speak for OpenBSD (never installed it myself), but if there is a critical patch to test on NetBSD - please let me know. > Phil. > >> } >> >> int socket_set_fast_reuse(int fd) >> >
On 1/25/2019 1:53 PM, Philippe Mathieu-Daudé wrote: > Hi, > > On 1/15/19 9:04 PM, Michael S. Tsirkin wrote: >> From: Li Qiang <liq3ea@163.com> >> >> Assert that the return value is not an error. This is like commit >> 7e6478e7d4f for qemu_set_cloexec. >> >> Signed-off-by: Li Qiang <liq3ea@163.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.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); >> + 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); > This commit breaks OpenBSD, when trying to start QEMU I get: > assertion "f != -1" failed: file "util/oslib-posix.c", line 247, > function "qemu_set_nonblock" > > Having a quick look at gdb, the last device opened is /dev/null, and > when fcntl() fails we have errno = ENODEV. > > 19 ENODEV Operation not supported by device. > An attempt was made to apply an inappropriate function to a device, > for example, trying to read a write-only device such as a printer. > > Digging further I found a recent commit which could fix this problem: > https://github.com/openbsd/src/commit/c2a35b387f9d3c > "fcntl(F_SETFL) invokes the FIONBIO and FIOASYNC ioctls internally, so > the memory devices (/dev/null, /dev/zero, etc) need to permit them." > > Brad: Do you think this might be the fix? If so, any idea what is the > first release to contain this fix? I don't know OpenBSD and can't figure > this out... Also, what would be the cleaner QEMU fix? I don't know. But that commit was included with 6.3 or newer.
On Fri, Jan 25, 2019 at 02:04:15PM -0500, Brad Smith wrote: > On 1/25/2019 1:53 PM, Philippe Mathieu-Daudé wrote: > > > Hi, > > > > On 1/15/19 9:04 PM, Michael S. Tsirkin wrote: > > > From: Li Qiang <liq3ea@163.com> > > > > > > Assert that the return value is not an error. This is like commit > > > 7e6478e7d4f for qemu_set_cloexec. > > > > > > Signed-off-by: Li Qiang <liq3ea@163.com> > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.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); > > > + 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); > > This commit breaks OpenBSD, when trying to start QEMU I get: > > assertion "f != -1" failed: file "util/oslib-posix.c", line 247, > > function "qemu_set_nonblock" > > > > Having a quick look at gdb, the last device opened is /dev/null, and > > when fcntl() fails we have errno = ENODEV. > > > > 19 ENODEV Operation not supported by device. > > An attempt was made to apply an inappropriate function to a device, > > for example, trying to read a write-only device such as a printer. > > > > Digging further I found a recent commit which could fix this problem: > > https://github.com/openbsd/src/commit/c2a35b387f9d3c > > "fcntl(F_SETFL) invokes the FIONBIO and FIOASYNC ioctls internally, so > > the memory devices (/dev/null, /dev/zero, etc) need to permit them." > > > > Brad: Do you think this might be the fix? If so, any idea what is the > > first release to contain this fix? I don't know OpenBSD and can't figure > > this out... Also, what would be the cleaner QEMU fix? > > I don't know. But that commit was included with 6.3 or newer. I'm quite prepared to revert that - the value of spreading asserts around is marginal and if we don't set fd even on a real file to non-blocking qemu doesn't explode - it just hangs which isn't much worse than an assert. Let me know.
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)