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

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

[Li Qiang]

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 = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+    assert(f != -1);
 }
 
 void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
-    fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFL, f | O_NONBLOCK);
+    assert(f != -1);
 }
 
 int socket_set_fast_reuse(int fd)
-- 
2.11.0

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/1544666977-4816-1-git-send-email-liq3ea@gmail.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp


The full log is available at
http://patchew.org/logs/1544666977-4816-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Peter Maydell]

On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote:
>
> Patchew URL: https://patchew.org/QEMU/1544666977-4816-1-git-send-email-liq3ea@gmail.com/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> time make docker-test-quick@centos7 SHOW_ENV=1 J=8
> === TEST SCRIPT END ===
>
> libpmem support   no
> libudev           no
>
> WARNING: Use of SDL 1.2 is deprecated and will be removed in
> WARNING: future releases. Please switch to using SDL 2.0
>
> NOTE: cross-compilers enabled:  'cc'
>   GEN     x86_64-softmmu/config-devices.mak.tmp
>
>
> The full log is available at
> http://patchew.org/logs/1544666977-4816-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message.

Patchew's attempt to limit the log to only the section with
the errors/warnings seems to have misfired here -- it looks
like it's picked the first bit of the log with a warning/error
rather than extracting all of them, which in this case happens
to be the harmless complaint that this build setup doesn't
have SDL2 installed.

The actual cause of the failure is much lower down:

  GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S61a1e35369394d7efb0a0e96d8af615d
  GTESTER tests/test-qht-par
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sbf8c21ef5f216840e073ff3e487dedbc
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sfe8cd276fddf3a92891cf274bf88e888
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S7487fc065e22541ba4c78c0db6c61d3c
Could not access KVM kernel module: No such file or directory

thanks
-- PMM

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

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道：

> On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote:
> >
> > Patchew URL:
> https://patchew.org/QEMU/1544666977-4816-1-git-send-email-liq3ea@gmail.com/
> >
> >
> >
> > Hi,
> >
> > This series failed the docker-quick@centos7 build test. Please find the
> testing commands and
> > their output below. If you have Docker installed, you can probably
> reproduce it
> > locally.
> >
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > time make docker-test-quick@centos7 SHOW_ENV=1 J=8
> > === TEST SCRIPT END ===
> >
> > libpmem support   no
> > libudev           no
> >
> > WARNING: Use of SDL 1.2 is deprecated and will be removed in
> > WARNING: future releases. Please switch to using SDL 2.0
> >
> > NOTE: cross-compilers enabled:  'cc'
> >   GEN     x86_64-softmmu/config-devices.mak.tmp
> >
> >
> > The full log is available at
> >
> http://patchew.org/logs/1544666977-4816-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message
> .
>
> Patchew's attempt to limit the log to only the section with
> the errors/warnings seems to have misfired here -- it looks
> like it's picked the first bit of the log with a warning/error
> rather than extracting all of them, which in this case happens
> to be the harmless complaint that this build setup doesn't
> have SDL2 installed.
>
> The actual cause of the failure is much lower down:
>
>
Indeed.


>   GTESTER check-qtest-aarch64
> vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> qemu_set_nonblock: Assertion `f != -1' failed.
>

So here means the fcntl call returns '-1'.
Seems this test have some bugs?

Thanks,
Li Qiang


> Broken pipe
> GTester: last random seed: R02S61a1e35369394d7efb0a0e96d8af615d
>   GTESTER tests/test-qht-par
> vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> qemu_set_nonblock: Assertion `f != -1' failed.
> Broken pipe
> GTester: last random seed: R02Sbf8c21ef5f216840e073ff3e487dedbc
> vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> qemu_set_nonblock: Assertion `f != -1' failed.
> Broken pipe
> GTester: last random seed: R02Sfe8cd276fddf3a92891cf274bf88e888
> vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> qemu_set_nonblock: Assertion `f != -1' failed.
> Broken pipe
> GTester: last random seed: R02S7487fc065e22541ba4c78c0db6c61d3c
> Could not access KVM kernel module: No such file or directory
>
> thanks
> -- PMM
>

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 05:56:24PM +0800, Li Qiang wrote:
> Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道：
> 
> > On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote:
> > >
> > > Patchew URL:
> > https://patchew.org/QEMU/1544666977-4816-1-git-send-email-liq3ea@gmail.com/
> > >
> > >
> > >
> > > Hi,
> > >
> > > This series failed the docker-quick@centos7 build test. Please find the
> > testing commands and
> > > their output below. If you have Docker installed, you can probably
> > reproduce it
> > > locally.
> > >
> > > === TEST SCRIPT BEGIN ===
> > > #!/bin/bash
> > > time make docker-test-quick@centos7 SHOW_ENV=1 J=8
> > > === TEST SCRIPT END ===
> > >
> > > libpmem support   no
> > > libudev           no
> > >
> > > WARNING: Use of SDL 1.2 is deprecated and will be removed in
> > > WARNING: future releases. Please switch to using SDL 2.0
> > >
> > > NOTE: cross-compilers enabled:  'cc'
> > >   GEN     x86_64-softmmu/config-devices.mak.tmp
> > >
> > >
> > > The full log is available at
> > >
> > http://patchew.org/logs/1544666977-4816-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message
> > .
> >
> > Patchew's attempt to limit the log to only the section with
> > the errors/warnings seems to have misfired here -- it looks
> > like it's picked the first bit of the log with a warning/error
> > rather than extracting all of them, which in this case happens
> > to be the harmless complaint that this build setup doesn't
> > have SDL2 installed.
> >
> > The actual cause of the failure is much lower down:
> >
> >
> Indeed.
> 
> 
> >   GTESTER check-qtest-aarch64
> > vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> > qemu_set_nonblock: Assertion `f != -1' failed.
> >
> 
> So here means the fcntl call returns '-1'.
> Seems this test have some bugs?

Not neccessarily. It means that some code is caller qemu_set_nonblock
with a file descriptor for this which is not valid. You'll have to
debug what caller is triggering this to understand why

It might be as simple as something passing in an FD == -1, and indeed
I fear this is quite likely as we've been ignoring errors from
qemu_set_nonblock forever.   IOW, your change may well break existing
code that is in fact working just fine today.


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é]

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

   if (fcntl(fd, F_SETFL, f & ~O_NONBLOCK) < 0) {
       error_report("Unable to set blocking flag on fd %d: %s (errno=%d)",
                    fd, strerror(errno), errno));
       abort();
   }


> +    f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
> +    assert(f != -1);
>  }
>  
>  void qemu_set_nonblock(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFL);
> -    fcntl(fd, F_SETFL, f | O_NONBLOCK);
> +    assert(f != -1);
> +    f = fcntl(fd, F_SETFL, f | O_NONBLOCK);
> +    assert(f != -1);
>  }
>  
>  int socket_set_fast_reuse(int fd)
> -- 
> 2.11.0
> 
> 

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}

[Li Qiang]

Daniel P. Berrangé <berrange@redhat.com> 于2018年12月13日周四 下午6:17写道：

> On Thu, Dec 13, 2018 at 05:56:24PM +0800, Li Qiang wrote:
> > Peter Maydell <peter.maydell@linaro.org> 于2018年12月13日周四 下午5:31写道：
> >
> > > On Thu, 13 Dec 2018 at 06:58, <no-reply@patchew.org> wrote:
> > > >
> > > > Patchew URL:
> > >
> https://patchew.org/QEMU/1544666977-4816-1-git-send-email-liq3ea@gmail.com/
> > > >
> > > >
> > > >
> > > > Hi,
> > > >
> > > > This series failed the docker-quick@centos7 build test. Please find
> the
> > > testing commands and
> > > > their output below. If you have Docker installed, you can probably
> > > reproduce it
> > > > locally.
> > > >
> > > > === TEST SCRIPT BEGIN ===
> > > > #!/bin/bash
> > > > time make docker-test-quick@centos7 SHOW_ENV=1 J=8
> > > > === TEST SCRIPT END ===
> > > >
> > > > libpmem support   no
> > > > libudev           no
> > > >
> > > > WARNING: Use of SDL 1.2 is deprecated and will be removed in
> > > > WARNING: future releases. Please switch to using SDL 2.0
> > > >
> > > > NOTE: cross-compilers enabled:  'cc'
> > > >   GEN     x86_64-softmmu/config-devices.mak.tmp
> > > >
> > > >
> > > > The full log is available at
> > > >
> > >
> http://patchew.org/logs/1544666977-4816-1-git-send-email-liq3ea@gmail.com/testing.docker-quick@centos7/?type=message
> > > .
> > >
> > > Patchew's attempt to limit the log to only the section with
> > > the errors/warnings seems to have misfired here -- it looks
> > > like it's picked the first bit of the log with a warning/error
> > > rather than extracting all of them, which in this case happens
> > > to be the harmless complaint that this build setup doesn't
> > > have SDL2 installed.
> > >
> > > The actual cause of the failure is much lower down:
> > >
> > >
> > Indeed.
> >
> >
> > >   GTESTER check-qtest-aarch64
> > > vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
> > > qemu_set_nonblock: Assertion `f != -1' failed.
> > >
> >
> > So here means the fcntl call returns '-1'.
> > Seems this test have some bugs?
>
> Not neccessarily. It means that some code is caller qemu_set_nonblock
> with a file descriptor for this which is not valid. You'll have to
> debug what caller is triggering this to understand why
>

>From the bot, seems the error occurs in aarch64 qtest. (I have checked for
x86_64, no this error).

  GTESTER check-qtest-x86_64
  GTESTER check-qtest-aarch64
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S61a1e35369394d7efb0a0e96d8af615d
  GTESTER tests/test-qht-par
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sbf8c21ef5f216840e073ff3e487dedbc
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02Sfe8cd276fddf3a92891cf274bf88e888
vhost-user-test: /tmp/qemu-test/src/util/oslib-posix.c:245:
qemu_set_nonblock: Assertion `f != -1' failed.
Broken pipe
GTester: last random seed: R02S7487fc065e22541ba4c78c0db6c61d3c
Could not access KVM kernel module: No such file or directory




>
> It might be as simple as something passing in an FD == -1, and indeed

I fear this is quite likely as we've been ignoring errors from
> qemu_set_nonblock forever.


Maybe, but if so, we need fix this(check 'fd' before calling
'qemu_set_{block，nonblock, cloexec}').
I think check 'fd' should be done before calling
'qemu_set_{block，nonblock, cloexec}', the caller of these function should
be responsible for the valid of 'fd'.

I will try to add some diagnostics info in the revised patch so that the
bot can give a more detailed information.


Thanks,
Li Qiang


> IOW, your change may well break existing
> code that is in fact working just fine today.
>
>
> 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}

[Peter Maydell]

On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> 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 = 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

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

[Markus Armbruster]

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

> On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> 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 = 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.

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

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 <peter.maydell@linaro.org> writes:
> 
> > On Thu, 13 Dec 2018 at 10:19, Daniel P. Berrangé <berrange@redhat.com> 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 = 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}

[Peter Maydell]

On Thu, 13 Dec 2018 at 12:39, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 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.

In an ideal world assert would print a stack backtrace :-)

thanks
-- PMM

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

[Markus Armbruster]

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

> On Thu, 13 Dec 2018 at 12:39, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> 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.
>
> In an ideal world assert would print a stack backtrace :-)

That feature would make even me make peace with g_assert() ;)

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

[...]
> Honestly I'd probably prefer these methods to take an "Error **errp"
> and propagate to the caller but that's alot more work.

I'd rather return 0 on success, -errno on failure in these cases.