On 11/8/19 12:42 PM, Peter Maydell wrote:
On Wed, 23 Oct 2019 at 03:04, Eric Blake <ebl...@redhat.com> wrote:
From: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Introduce a function to gracefully wake a coroutine sleeping in
qemu_co_sleep_ns().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Reviewed-by: Kevin Wolf <kw...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Message-Id: <20191009084158.15614-2-vsement...@virtuozzo.com>
Signed-off-by: Eric Blake <ebl...@redhat.com>
Hi; Coverity reports an issue in this patch (CID 1406474):
- * Yield the coroutine for a given duration
+ * 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
+ * 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.
*/
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
Here, we document the rules on what will happen to *sleep_state (in
particular, since we store a stack local variable into it, the caller
must not leak it further, and it will be reclaimed back to zero before
this function finally finishes).
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
+void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
+ QemuCoSleepState **sleep_state)
{
AioContext *ctx = qemu_get_current_aio_context();
- QEMUTimer *ts;
- Coroutine *co = qemu_coroutine_self();
+ QemuCoSleepState state = {
+ .co = qemu_coroutine_self(),
+ .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state),
+ .user_state_pointer = sleep_state,
+ };
Here 'state' is a variable on the stack...
- const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
+ const char *scheduled = atomic_cmpxchg(&state.co->scheduled, NULL,
+ qemu_co_sleep_ns__scheduled);
if (scheduled) {
fprintf(stderr,
"%s: Co-routine was already scheduled in '%s'\n",
__func__, scheduled);
abort();
}
- ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
- timer_mod(ts, qemu_clock_get_ns(type) + ns);
+
+ if (sleep_state) {
+ *sleep_state = &state;
...here we save a pointer to it into *sleep_state which was
passed to us by the caller...
+ }
+ timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
qemu_coroutine_yield();
And here is where we yield, which is the only point at which the caller
will see a non-zero value in *sleep_state in the first place, at which
point the caller must follow the rules we document.
- timer_del(ts);
- timer_free(ts);
+ timer_free(state.ts);
...and here we return from this function, which means 'state'
is no longer in valid memory, but the caller has still been
given a pointer to it.
}
Is this just Coverity getting confused by our coroutine code?
(I certainly find it confusing...)
Yes, Coverity is unable to see that we require that the caller MUST obey
rules with the use of it's access to sleep_state. However, it might be
possible after the yield to assert(!sleep_state || *sleep_state == NULL)
to prove that the caller's use of our temporary leak of a stack variable
was solely to get this coroutine to resume from yield, and that the
resumption cleared it, so that the end of the function is not leaking
anything.
I guess it's worth experimenting to see if such a patch would silence
Coverity without breaking the code...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org