From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBh0i-0003SQ-HV for qemu-devel@nongnu.org; Wed, 23 Dec 2015 05:51:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBh0h-0000az-Gw for qemu-devel@nongnu.org; Wed, 23 Dec 2015 05:51:12 -0500 Sender: Paolo Bonzini References: <1450867706-19860-4-git-send-email-pbonzini@redhat.com> From: Paolo Bonzini Message-ID: <567A7C95.5000800@redhat.com> Date: Wed, 23 Dec 2015 11:51:01 +0100 MIME-Version: 1.0 In-Reply-To: <1450867706-19860-4-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] blockjob: prevent double enter (with dangling access on the second) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: qemu-stable@nongnu.org, qemu-block@nongnu.org On 23/12/2015 11:48, Paolo Bonzini wrote: > Because block_job_sleep_ns marks the job as non-busy, it is > possible to enter it again from block_job_enter. However, the > same coroutine may then be re-entered from co_sleep_cb, and this > time the CoSleepCB is not on the stack anymore. > > Fix this by open coding the sleep in block_job_sleep_ns, and > deleting the timer immediately after the coroutine is re-entered. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Paolo Bonzini Forget about this, the current code does exactly the same as what I wrote, and is correct. /me needs vacation. :) Paolo > --- > blockjob.c | 27 +++++++++++++++++++++++---- > include/block/blockjob.h | 3 +-- > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index 80adb9d..5e59a8f 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -329,8 +329,21 @@ int block_job_complete_sync(BlockJob *job, Error **errp) > return block_job_finish_sync(job, &block_job_complete, errp); > } > > +typedef struct CoSleepCB { > + QEMUTimer *ts; > + Coroutine *co; > +} CoSleepCB; > + > +static void co_sleep_cb(void *opaque) > +{ > + CoSleepCB *sleep_cb = opaque; > + > + qemu_coroutine_enter(sleep_cb->co, NULL); > +} > + > void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) > { > + CoSleepCB sleep_cb = { .ts = NULL }; > assert(job->busy); > > /* Check cancellation *before* setting busy = false, too! */ > @@ -339,10 +352,16 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) > } > > job->busy = false; > - if (block_job_is_paused(job)) { > - qemu_coroutine_yield(); > - } else { > - co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns); > + if (!block_job_is_paused(job)) { > + sleep_cb.co = qemu_coroutine_self(), > + sleep_cb.ts = aio_timer_new(bdrv_get_aio_context(job->bs), type, > + SCALE_NS, co_sleep_cb, &sleep_cb); > + timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns); > + } > + qemu_coroutine_yield(); > + if (sleep_cb.ts) { > + timer_del(sleep_cb.ts); > + timer_free(sleep_cb.ts); > } > job->busy = true; > } > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index d84ccd8..82063f2 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -120,8 +120,7 @@ struct BlockJob { > > /** > * Set to false by the job while it is in a quiescent state, where > - * no I/O is pending and the job has yielded on any condition > - * that is not detected by #aio_poll, such as a timer. > + * no I/O is pending. The job can be reentered with qemu_coroutine_enter. > */ > bool busy; > >