Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Stefan Hajnoczi]

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