[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points |
Date: |
Thu, 16 Jun 2016 11:19:12 +0100 |
User-agent: |
Mutt/1.6.1 (2016-04-27) |
On Wed, Jun 15, 2016 at 04:57:41PM +0800, Fam Zheng wrote:
> On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -247,6 +247,30 @@ void block_job_complete(BlockJob *job, Error **errp)
> > job->driver->complete(job, errp);
> > }
> >
> > +void block_job_pause_point(BlockJob *job)
> > +{
> > + if (!block_job_is_paused(job)) {
>
> I find this check ...
>
> > + return;
> > + }
> > + if (block_job_is_cancelled(job)) {
> > + return;
> > + }
> > +
> > + if (job->driver->pause) {
> > + job->driver->pause(job);
> > + }
> > +
> > + job->paused = true;
>
> ... and this assignment confusing. After reading more, I think we ought to
> rename block_job_is_paused to block_job_should_pause and mark it static, in a
> separate patch.
>
> > + job->busy = false;
> > + qemu_coroutine_yield(); /* wait for block_job_resume() */
> > + job->busy = true;
> > + job->paused = false;
>
> Worth to "assert(!job->pause_count)" (or
> "assert(!block_job_should_pause(job))")?
>
> Regardless,
>
> Reviewed-by: Fam Zheng <address@hidden>
Nice solution! I hesitated a bit with job->paused vs
block_job_is_paused() because the naming is indeed confusing.
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup, Stefan Hajnoczi, 2016/06/14
- [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points, Stefan Hajnoczi, 2016/06/14
- [Qemu-devel] [PATCH v4 1/5] blockjob: move iostatus reset out of block_job_enter(), Stefan Hajnoczi, 2016/06/14
- [Qemu-devel] [PATCH v4 5/5] backup: follow AioContext change gracefully, Stefan Hajnoczi, 2016/06/14
- [Qemu-devel] [PATCH v4 3/5] blockjob: add AioContext attached callback, Stefan Hajnoczi, 2016/06/14
- Re: [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup, Jason J. Herne, 2016/06/14
- Re: [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup, Paolo Bonzini, 2016/06/15