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 <mreitz@redhat.com>
---
 block/commit.c | 16 +++++++++-------
 qemu-img.c     | 19 ++++++++++++++-----
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 87f6096d90..ba60fb7955 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -414,7 +414,9 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     ctx = bdrv_get_aio_context(bs);
-    src = blk_new(ctx, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
+    /* WRITE_UNCHANGED is required for bdrv_make_empty() */
+    src = blk_new(ctx, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED,
+                  BLK_PERM_ALL);
     backing = blk_new(ctx, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
 
     ret = blk_insert_bs(src, bs, &local_err);
@@ -492,14 +494,14 @@ int bdrv_commit(BlockDriverState *bs)
         }
     }
 
-    if (drv->bdrv_make_empty) {
-        ret = drv->bdrv_make_empty(bs);
-        if (ret < 0) {
-            goto ro_cleanup;
-        }
-        blk_flush(src);
+    ret = blk_make_empty(src, NULL);
+    /* Ignore -ENOTSUP */
+    if (ret < 0 && ret != -ENOTSUP) {
+        goto ro_cleanup;
     }
 
+    blk_flush(src);
+
     /*
      * Make sure all data we wrote to the backing device is actually
      * stable on disk.
diff --git a/qemu-img.c b/qemu-img.c
index 7f52742ef2..77f3575538 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);
-        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);
+        blk_unref(old_backing_blk);
+        if (ret == -ENOTSUP) {
+            error_free(local_err);
+            local_err = NULL;
+        } else if (ret < 0) {
             goto unref_backing;
         }
     }
-- 
2.25.4

On 29.04.20 16:11, 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 <mreitz@redhat.com>
> ---
>  block/commit.c | 16 +++++++++-------
>  qemu-img.c     | 19 ++++++++++++++-----
>  2 files changed, 23 insertions(+), 12 deletions(-)

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 7f52742ef2..77f3575538 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);
> -        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);
> +        blk_unref(old_backing_blk);
> +        if (ret == -ENOTSUP) {
> +            error_free(local_err);
> +            local_err = NULL;
> +        } else if (ret < 0) {
>              goto unref_backing;
>          }
>      }

This patch should also include a %s/Could not empty/Failed to empty/ on
tests/qemu-iotests/098.out...

Max

On 4/29/20 9:11 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 <mreitz@redhat.com>
> ---
>   block/commit.c | 16 +++++++++-------
>   qemu-img.c     | 19 ++++++++++++++-----
>   2 files changed, 23 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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