Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
>
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
>
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   block/commit.c |  8 +++++++-
>   qemu-img.c     | 19 ++++++++++++++-----
>   2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>       }
>   
>       if (drv->bdrv_make_empty) {

This 'if' is still a bit awkward. Do all filter drivers set this 
function, or will bdrv_make_empty() automatically take care of looking 
through filters?  Should this be a check of a supported_ variable 
instead (similar to how Kevin just added supported_truncate_flags)?

> -        ret = drv->bdrv_make_empty(bs);
> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>           if (ret < 0) {
>               goto ro_cleanup;
>           }
> +
> +        ret = blk_make_empty(src, NULL);

So, if the driver lacks the callback, you miss calling blk_make_empty() 
even if it would have done something.

> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +
>           blk_flush(src);
>       }
>   
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>           goto unref_backing;
>       }
>   
> -    if (!drop && bs->drv->bdrv_make_empty) {
> -        ret = bs->drv->bdrv_make_empty(bs);

Old code: if the driver had the callback, use it.

> -        if (ret) {
> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
> -                             filename);
> +    if (!drop) {
> +        BlockBackend *old_backing_blk;
> +
> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> +                                          &local_err);
> +        if (!old_backing_blk) {
> +            goto unref_backing;
> +        }
> +        ret = blk_make_empty(old_backing_blk, &local_err);

New code: Call blk_make_empty() whether or not driver has the callback, 
then deal with the fallout.

> +        blk_unref(old_backing_blk);
> +        if (ret == -ENOTSUP) {
> +            error_free(local_err);
> +            local_err = NULL;
> +        } else if (ret < 0) {
>               goto unref_backing;
>           }

But this actually looks smarter than the commit case.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org