From: Markus Armbruster <armbru@redhat.com>
References: <1544666977-4816-1-git-send-email-liq3ea@gmail.com>
	<20181213101940.GC5171@redhat.com>
	<CAFEAcA_rBakXJZuPGgcsvC9m75oeTbg=q-=mOCFJuungF-VR8g@mail.gmail.com>
Date: Thu, 13 Dec 2018 13:28:23 +0100
In-Reply-To: <CAFEAcA_rBakXJZuPGgcsvC9m75oeTbg=q-=mOCFJuungF-VR8g@mail.gmail.com>
	(Peter Maydell's message of "Thu, 13 Dec 2018 11:27:35 +0000")
Message-ID: <87o99pa9w8.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH] util: check the return value of fcntl in
 qemu_set_{block, noblock}
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Daniel P. Berrange" <berrange@redhat.com>, =?utf-8?Q?Marc-Andr=C3=A9?= Lureau <marcandre.lureau@redhat.com>, Li Qiang <liq3ea@gmail.com>, QEMU Developers <qemu-devel@nongnu.org>, Paolo Bonzini <pbonzini@redhat.com>

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrang=C3=A9 <berrange@redhat.co=
m> 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 <liq3ea@gmail.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 =3D fcntl(fd, F_GETFL);
>> > -    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
>> > +    assert(f !=3D -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.

When you feel you have to print something fancy when an assertion fails,
either your feelings are misguided, or the assertion is wrong.