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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]