[Qemu-devel] [PATCH] main_loop: Make main_loop_wait() return void

[Qemu-devel] [PATCH] main_loop: Make main_loop_wait() return void

[Peter Maydell]

In commit e330c118f2a5a the last usage of main_loop_wait() that cared
about the return value was changed to no longer use it. Drop the
now-useless return value and make the function return void.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Coverity complains (CID 1372464) about main_loop() calling
main_loop_wait() and ignoring its return value. I suspect
this change will just displace that to within main_loop_wait()
itself since the underlying issue there is "the ppoll() that
gets called to poll fds can return an error code, but what
do we do if it does?". Suggestions on that point welcome.

I guess this will make the compiler warn about ret being
set and never used if CONFIG_SLIRP is not defined, which
is irritating. I'm postponing messing about with fixing
that in favour of seeing whether anybody has a good answer
to the question above (which might make it moot).
---
 include/qemu/main-loop.h | 2 +-
 util/main-loop.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index d7e24af..6b4b60b 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -79,7 +79,7 @@ int qemu_init_main_loop(Error **errp);
  *
  * @nonblocking: Whether the caller should block until an event occurs.
  */
-int main_loop_wait(int nonblocking);
+void main_loop_wait(int nonblocking);
 
 /**
  * qemu_get_aio_context: Return the main loop's AioContext
diff --git a/util/main-loop.c b/util/main-loop.c
index 4534c89..d0e27f3 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -476,7 +476,7 @@ static int os_host_main_loop_wait(int64_t timeout)
 }
 #endif
 
-int main_loop_wait(int nonblocking)
+void main_loop_wait(int nonblocking)
 {
     int ret;
     uint32_t timeout = UINT32_MAX;
@@ -513,7 +513,7 @@ int main_loop_wait(int nonblocking)
     qemu_start_warp_timer();
     qemu_clock_run_all_timers();
 
-    return ret;
+    return;
 }
 
 /* Functions to operate on the main QEMU AioContext.  */
-- 
2.7.4

Re: [Qemu-devel] [PATCH] main_loop: Make main_loop_wait() return void

[Peter Maydell]

On 31 March 2017 at 14:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> In commit e330c118f2a5a the last usage of main_loop_wait() that cared
> about the return value was changed to no longer use it. Drop the
> now-useless return value and make the function return void.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Coverity complains (CID 1372464) about main_loop() calling
> main_loop_wait() and ignoring its return value. I suspect
> this change will just displace that to within main_loop_wait()
> itself since the underlying issue there is "the ppoll() that
> gets called to poll fds can return an error code, but what
> do we do if it does?". Suggestions on that point welcome.
>
> I guess this will make the compiler warn about ret being
> set and never used if CONFIG_SLIRP is not defined, which
> is irritating. I'm postponing messing about with fixing
> that in favour of seeing whether anybody has a good answer
> to the question above (which might make it moot).
> ---

...oops, I meant to tag this RFC; never mind.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] main_loop: Make main_loop_wait() return void

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

On 03/31/2017 08:51 AM, Peter Maydell wrote:
> In commit e330c118f2a5a the last usage of main_loop_wait() that cared
> about the return value was changed to no longer use it. Drop the
> now-useless return value and make the function return void.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Coverity complains (CID 1372464) about main_loop() calling
> main_loop_wait() and ignoring its return value. I suspect
> this change will just displace that to within main_loop_wait()
> itself since the underlying issue there is "the ppoll() that
> gets called to poll fds can return an error code, but what
> do we do if it does?". Suggestions on that point welcome.

At one point, there was a suggestion to introduce an ignore_value()
macro that ignores values that the compiler/coverity would otherwise
complain about, in contexts where we really are okay ignoring the value.
 If making main_loop_wait() return void shifts where Coverity blames,
then ignore_value(ppoll()) seems like it might help.

> 
> I guess this will make the compiler warn about ret being
> set and never used if CONFIG_SLIRP is not defined, which
> is irritating. I'm postponing messing about with fixing
> that in favour of seeing whether anybody has a good answer
> to the question above (which might make it moot).
> ---

> -int main_loop_wait(int nonblocking)
> +void main_loop_wait(int nonblocking)
>  {
>      int ret;
>      uint32_t timeout = UINT32_MAX;
> @@ -513,7 +513,7 @@ int main_loop_wait(int nonblocking)
>      qemu_start_warp_timer();
>      qemu_clock_run_all_timers();
>  
> -    return ret;
> +    return;
>  }

We generally avoid lone return; at the end of a void function.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] main_loop: Make main_loop_wait() return void

[Peter Maydell]

On 31 March 2017 at 15:10, Eric Blake <eblake@redhat.com> wrote:
> On 03/31/2017 08:51 AM, Peter Maydell wrote:
>> In commit e330c118f2a5a the last usage of main_loop_wait() that cared
>> about the return value was changed to no longer use it. Drop the
>> now-useless return value and make the function return void.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Coverity complains (CID 1372464) about main_loop() calling
>> main_loop_wait() and ignoring its return value. I suspect
>> this change will just displace that to within main_loop_wait()
>> itself since the underlying issue there is "the ppoll() that
>> gets called to poll fds can return an error code, but what
>> do we do if it does?". Suggestions on that point welcome.
>
> At one point, there was a suggestion to introduce an ignore_value()
> macro that ignores values that the compiler/coverity would otherwise
> complain about, in contexts where we really are okay ignoring the value.
>  If making main_loop_wait() return void shifts where Coverity blames,
> then ignore_value(ppoll()) seems like it might help.

Mmm, but my question really is "is ignoring the error code
from the syscall the correct thing to do?"... though I'm
not sure what else we would want to do (log a warning?
abort?).

thanks
-- PMM