On Thu, Dec 13, 2018 at 01:28:23PM +0100, Markus Armbruster wrote: > Peter Maydell <[email protected]> writes: > > > On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <[email protected]> > > 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 <[email protected]> > >> > --- > >> > 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 -- |: 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 :|
