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 <jsnow@redhat.com>
---
 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
@@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
     int max_iov;
     bool initial_zeroing_ongoing;
     int in_active_write_counter;
+    bool prepared;
 } MirrorBlockJob;
 
 typedef struct MirrorBDSOpaque {
@@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
     }
 }
 
-static void mirror_exit(Job *job)
+static int mirror_exit_common(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockJob *bjob = &s->common;
@@ -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;
+    int ret = 0;
+
+    if (s->prepared) {
+        return 0;
+    }
+    s->prepared = true;
 
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
@@ -648,7 +655,7 @@ static void mirror_exit(Job *job)
         aio_context_acquire(replace_aio_context);
     }
 
-    if (s->should_complete && ret == 0) {
+    if (s->should_complete && !abort) {
         BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
 
         if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
@@ -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);
 }
 
 static void mirror_throttle(MirrorBlockJob *s)
@@ -1133,7 +1150,8 @@ static const BlockJobDriver mirror_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -1150,7 +1168,8 @@ static const BlockJobDriver commit_active_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
-- 
2.14.4

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 <jsnow@redhat.com>
> ---
>  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.

> +    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

Reviewed-by: Max Reitz <mreitz@redhat.com>

>  }
>  
>  static void mirror_throttle(MirrorBlockJob *s)

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 <jsnow@redhat.com>
>> ---
>>  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 <mreitz@redhat.com>
> 
>>  }
>>  
>>  static void mirror_throttle(MirrorBlockJob *s)
>