Re: [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

03.05.2021 14:25, Paolo Bonzini wrote:
> Simplify the code by removing conditionals.  qemu_co_sleep_ns
> can simply use point the argument to an on-stack temporary.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qemu/coroutine.h    |  5 +++--
>   util/qemu-coroutine-sleep.c | 18 +++++-------------
>   2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index ce5b9c6851..c5d7742989 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -295,7 +295,7 @@ typedef struct QemuCoSleepState QemuCoSleepState;
>   
>   /**
>    * Yield the coroutine for a given duration. During this yield, @sleep_state
> - * (if not NULL) is set to an opaque pointer, which may be used for
> + * is set to an opaque pointer, which may be used for
>    * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
>    * timer fires. Don't save the obtained value to other variables and don't call
>    * qemu_co_sleep_wake from another aio context.
> @@ -304,7 +304,8 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>                                               QemuCoSleepState **sleep_state);
>   static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
>   {
> -    qemu_co_sleep_ns_wakeable(type, ns, NULL);
> +    QemuCoSleepState *unused = NULL;
> +    qemu_co_sleep_ns_wakeable(type, ns, &unused);
>   }
>   
>   /**
> diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
> index eec6e81f3f..3f6f637e81 100644
> --- a/util/qemu-coroutine-sleep.c
> +++ b/util/qemu-coroutine-sleep.c
> @@ -32,9 +32,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
>                                              qemu_co_sleep_ns__scheduled, NULL);
>   
>       assert(scheduled == qemu_co_sleep_ns__scheduled);
> -    if (sleep_state->user_state_pointer) {
> -        *sleep_state->user_state_pointer = NULL;
> -    }
> +    *sleep_state->user_state_pointer = NULL;
>       timer_del(&sleep_state->ts);
>       aio_co_wake(sleep_state->co);
>   }
> @@ -63,16 +61,10 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>       }
>   
>       aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
> -    if (sleep_state) {
> -        *sleep_state = &state;
> -    }
> +    *sleep_state = &state;
>       timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
>       qemu_coroutine_yield();
> -    if (sleep_state) {
> -        /*
> -         * Note that *sleep_state is cleared during qemu_co_sleep_wake
> -         * before resuming this coroutine.
> -         */
> -        assert(*sleep_state == NULL);
> -    }
> +
> +    /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */

over-80 line

> +    assert(*sleep_state == NULL);
>   }
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir