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

[Peter Maydell]

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...

thanks
-- PMM