Re: [Qemu-devel] [PATCH 4/7] block/commit: utilize job_exit shim

[Max Reitz]

On 2018-08-17 21:18, John Snow wrote:
> 
> 
> On 08/17/2018 03:04 PM, John Snow wrote:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit, renaming commit_complete to commit_exit.
>>
>> This conversion does change the timing of when job_completed is
>> called to after the bdrv_replace_node and bdrv_unref calls, which
>> could have implications for bjob->blk which will now be put down
>> after this cleanup.
>>
>> Kevin highlights that we did not take any permissions for that backend
>> at job creation time, so it is safe to reorder these operations.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/commit.c | 20 ++++----------------
>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 4a17bb73ec..93c3b6a39e 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -68,19 +68,13 @@ 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;
>>      bool remove_commit_top_bs = false;
>>  
>>      /* Make sure commit_top_bs and top stay around until 
>> bdrv_replace_node() */
>> @@ -93,8 +87,8 @@ static void commit_complete(Job *job, void *opaque)
>>  
>>      if (!job_is_cancelled(job) && ret == 0) {
> 
> forgot to actually squash the change in here that replaces `ret` with
> `job->ret`.

A better interface would be one function that is called when .run() was
successful, and one that is called when it was not.  (They can still
resolve into a single function in the job that is just called with a
boolean that's either true or false, but accessing *job to find out
whether .run() was successful or not seems kind of convoluted to me.)

Max

signature.asc
Description: OpenPGP digital signature