Re: [Qemu-block] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor

[John Snow]

On 09/03/2018 05:32 AM, Max Reitz wrote:
> On 2018-09-01 00:28, John Snow wrote:
>> For purposes of minimum code movement, refactor the mirror_exit
>> callback to use the post-finalization callbacks in a trivial way.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/mirror.c | 31 +++++++++++++++++++++++++------
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index c164fee883..5067f1764d 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
> 
> [...]
> 
>> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>>      BlockDriverState *target_bs = blk_bs(s->target);
>>      BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>>      Error *local_err = NULL;
>> -    int ret = job->ret;
>> +    bool abort = !!job->ret;
> 
> I think "job->ret < 0" could be read more easily.

Hm, OK. We shouldn't be using > 0 retcodes anyway.

> 
>> +    int ret = 0;
>> +
>> +    if (s->prepared) {
>> +        return 0;
>> +    }
>> +    s->prepared = true;
>>  
>>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>  
> 
> [...]
> 
>> @@ -712,7 +719,17 @@ static void mirror_exit(Job *job)
>>      bdrv_unref(mirror_top_bs);
>>      bdrv_unref(src);
>>  
>> -    job->ret = ret;
>> +    return ret;
>> +}
>> +
>> +static int mirror_prepare(Job *job)
>> +{
>> +    return mirror_exit_common(job);
>> +}
>> +
>> +static void mirror_abort(Job *job)
>> +{
>> +    assert(mirror_exit_common(job) == 0);
> 
> You shouldn't execute vital code in assert(), as NDEBUG would make that
> code disappear.  As far as I know, we have decided (at some point) not
> to ever enable NDEBUG in qemu, but, you know.  Doing it right only costs
> one more line, and it would get you a
> 

d'oh.

> Reviewed-by: Max Reitz <address@hidden>
> 
>>  }
>>  
>>  static void mirror_throttle(MirrorBlockJob *s)
>