[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_s
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock} |
Date: |
Thu, 13 Dec 2018 12:39:08 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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 :|
Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Daniel P . Berrangé, 2018/12/13
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Peter Maydell, 2018/12/13
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Markus Armbruster, 2018/12/13
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock},
Daniel P . Berrangé <=
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Peter Maydell, 2018/12/13
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Markus Armbruster, 2018/12/13
- Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in qemu_set_{block, noblock}, Markus Armbruster, 2018/12/13