qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 04/21] block/commit: utilize job_exit shim


From: John Snow
Subject: Re: [Qemu-block] [PATCH 04/21] block/commit: utilize job_exit shim
Date: Wed, 8 Aug 2018 10:00:33 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0


On 08/07/2018 11:41 PM, Jeff Cody wrote:
> On Tue, Aug 07, 2018 at 12:33:32AM -0400, John Snow wrote:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/commit.c | 19 +++++--------------
>>  1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 620666161b..5bed098d5f 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend 
>> *bs, BlockBackend *base,
>>      return 0;
>>  }
>>  
>> -typedef struct {
>> -    int ret;
>> -} CommitCompleteData;
>> -
>> -static void commit_complete(Job *job, void *opaque)
>> +static void commit_exit(Job *job)
>>  {
>>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>>      BlockJob *bjob = &s->common;
>> -    CommitCompleteData *data = opaque;
>>      BlockDriverState *top = blk_bs(s->top);
>>      BlockDriverState *base = blk_bs(s->base);
>>      BlockDriverState *commit_top_bs = s->commit_top_bs;
>> -    int ret = data->ret;
>> +    int ret = job->ret;
>>      bool remove_commit_top_bs = false;
>>  
>>      /* Make sure commit_top_bs and top stay around until 
>> bdrv_replace_node() */
> 
> Since 'ret' is only assigned to 'job->ret' outside of this line, we could
> drop the local 'ret' and assign job->ret to the output of
> brdrv_drop_intermediate() in this code below:
> 
>     if (!job_is_cancelled(job) && ret == 0) {
>         /* success */
>         ret = bdrv_drop_intermediate(s->commit_top_bs, base,
>                                      s->backing_file_str);
>     }
> 
>> @@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
>>       * bdrv_set_backing_hd() to fail. */
>>      block_job_remove_all_bdrv(bjob);
>>  
>> -    job_completed(job, ret);
>> -    g_free(data);
>> -
>>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>>       * filter driver from the backing chain. Do this as the final step so 
>> that
>>       * the 'consistent read' permission can be granted.  */
>> @@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
>>  
>>      bdrv_unref(commit_top_bs);
>>      bdrv_unref(top);
>> +    job->ret = ret;
> 
> We could then drop this line as well.
> 
> (This may fall under bike shedding, so I don't feel strongly about it):

You're right; though most of it gets blown out of the water in #16
anyway, so it doesn't stay a problem for long.

--js

> 
> Reviewed-by: Jeff Cody <address@hidden>
> 

Thanks!

>>  }
>>  
>>  static void coroutine_fn commit_run(void *opaque)
>>  {
>>      CommitBlockJob *s = opaque;
>> -    CommitCompleteData *data;
>>      int64_t offset;
>>      uint64_t delay_ns = 0;
>>      int ret = 0;
>> @@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
>>  out:
>>      qemu_vfree(buf);
>>  
>> -    data = g_malloc(sizeof(*data));
>> -    data->ret = ret;
>> -    job_defer_to_main_loop(&s->common.job, commit_complete, data);
>> +    s->common.job.ret = ret;
>>  }
>>  
>>  static const BlockJobDriver commit_job_driver = {
>> @@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
>>          .user_resume   = block_job_user_resume,
>>          .drain         = block_job_drain,
>>          .start         = commit_run,
>> +        .exit          = commit_exit,
>>      },
>>  };
>>  
>> -- 
>> 2.14.4
>>





reply via email to

[Prev in Thread] Current Thread [Next in Thread]