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 <f...@redhat.com>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
