Re: [Qemu-devel] [PATCH v3 08/15] blockjob: Simplify block_job_finish_sync

[John Snow <jsnow@redhat.com>]

On 07/09/2015 11:46 PM, Fam Zheng wrote:
> With job->completed and job->ret to replace BlockFinishData.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockjob.c | 23 ++---------------------
>  1 file changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index fb1d9e7..11b48f5 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -179,43 +179,24 @@ struct BlockFinishData {
>      int ret;
>  };
>  
> -static void block_job_finish_cb(void *opaque, int ret)
> -{
> -    struct BlockFinishData *data = opaque;
> -
> -    data->cancelled = block_job_is_cancelled(data->job);
> -    data->ret = ret;
> -    data->cb(data->opaque, ret);
> -}
> -
>  static int block_job_finish_sync(BlockJob *job,
>                                   void (*finish)(BlockJob *, Error **errp),
>                                   Error **errp)
>  {
> -    struct BlockFinishData data;
>      BlockDriverState *bs = job->bs;
>      Error *local_err = NULL;
>  
>      assert(bs->job == job);
>  
> -    /* Set up our own callback to store the result and chain to
> -     * the original callback.
> -     */
> -    data.job = job;
> -    data.cb = job->cb;
> -    data.opaque = job->opaque;
> -    data.ret = -EINPROGRESS;
> -    job->cb = block_job_finish_cb;
> -    job->opaque = &data;
>      finish(job, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return -EBUSY;
>      }
> -    while (data.ret == -EINPROGRESS) {
> +    while (!job->completed) {
>          aio_poll(bdrv_get_aio_context(bs), true);
>      }
> -    return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret;
> +    return (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>  }
>  
>  /* A wrapper around block_job_cancel() taking an Error ** parameter so it may be
> 

Hmm ... I'm less sure of this one because I can't imagine why it's split
out the way it is to begin with.

Looks sane to me, though...

Reviewed-by: John Snow <jsnow@redhat.com>