Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}

[Daniel P . Berrangé]

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