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

[Max Reitz]

On 2018-08-22 23:55, John Snow wrote:
> 
> 
> On 08/22/2018 07:58 AM, Max Reitz wrote:
>> 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
>>
> 
> Sorry, I need a better cover letter.

My mistake, I need to read more than the first few lines of a cover letter.

> .exit() is going away, and either .prepare() or .abort() will be called
> after .run(), so what you're asking for will be true, just not all at
> once in this series.

Yay!

Max

signature.asc
Description: OpenPGP digital signature