Most probably this fake backing child doesn't work anyway (see notes
about it in a8a4d15c1c34d).

Still, since 25f78d9e2de528473d52 drivers are required to set
.supports_backing if they want to call bdrv_set_backing_hd, so now
vvfat just doesn't work because of this check.

Let's finally drop this fake backing file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Honestly, I don't know, which scenarios may break after this patch.
So, that's just my idea that it's more correct to drop this thing than
set .supports_backing in vvfat driver.

 block/vvfat.c | 43 ++++---------------------------------------
 1 file changed, 4 insertions(+), 39 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ae9d387da7..34bf1e3a86 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3098,26 +3098,6 @@ static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs,
     return BDRV_BLOCK_DATA;
 }
 
-static int coroutine_fn
-write_target_commit(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
-                    QEMUIOVector *qiov, int flags)
-{
-    int ret;
-
-    BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
-    qemu_co_mutex_lock(&s->lock);
-    ret = try_commit(s);
-    qemu_co_mutex_unlock(&s->lock);
-
-    return ret;
-}
-
-static BlockDriver vvfat_write_target = {
-    .format_name        = "vvfat_write_target",
-    .instance_size      = sizeof(void*),
-    .bdrv_co_pwritev    = write_target_commit,
-};
-
 static void vvfat_qcow_options(BdrvChildRole role, bool parent_is_format,
                                int *child_flags, QDict *child_options,
                                int parent_flags, QDict *parent_options)
@@ -3133,7 +3113,6 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
     BlockDriver *bdrv_qcow = NULL;
-    BlockDriverState *backing;
     QemuOpts *opts = NULL;
     int ret;
     int size = sector2cluster(s, s->sector_count);
@@ -3184,13 +3163,6 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
     unlink(s->qcow_filename);
 #endif
 
-    backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
-                                   &error_abort);
-    *(void**) backing->opaque = s;
-
-    bdrv_set_backing_hd(s->bs, backing, &error_abort);
-    bdrv_unref(backing);
-
     return 0;
 
 err:
@@ -3205,17 +3177,10 @@ static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,
                              uint64_t perm, uint64_t shared,
                              uint64_t *nperm, uint64_t *nshared)
 {
-    if (role & BDRV_CHILD_DATA) {
-        /* This is a private node, nobody should try to attach to it */
-        *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
-        *nshared = BLK_PERM_WRITE_UNCHANGED;
-    } else {
-        assert(role & BDRV_CHILD_COW);
-        /* The backing file is there so 'commit' can use it. vvfat doesn't
-         * access it in any way. */
-        *nperm = 0;
-        *nshared = BLK_PERM_ALL;
-    }
+    assert(role & BDRV_CHILD_DATA);
+    /* This is a private node, nobody should try to attach to it */
+    *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
+    *nshared = BLK_PERM_WRITE_UNCHANGED;
 }
 
 static void vvfat_close(BlockDriverState *bs)
-- 
2.29.2

> On Jul 15, 2021, at 8:48 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> Most probably this fake backing child doesn't work anyway (see notes
> about it in a8a4d15c1c34d).
> 
> Still, since 25f78d9e2de528473d52 drivers are required to set
> .supports_backing if they want to call bdrv_set_backing_hd, so now
> vvfat just doesn't work because of this check.
> 
> Let's finally drop this fake backing file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Honestly, I don't know, which scenarios may break after this patch.
> So, that's just my idea that it's more correct to drop this thing than
> set .supports_backing in vvfat driver.
> 
> block/vvfat.c | 43 ++++---------------------------------------
> 1 file changed, 4 insertions(+), 39 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index ae9d387da7..34bf1e3a86 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3098,26 +3098,6 @@ static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs,
>     return BDRV_BLOCK_DATA;
> }
> 
> -static int coroutine_fn
> -write_target_commit(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> -                    QEMUIOVector *qiov, int flags)
> -{
> -    int ret;
> -
> -    BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
> -    qemu_co_mutex_lock(&s->lock);
> -    ret = try_commit(s);
> -    qemu_co_mutex_unlock(&s->lock);
> -
> -    return ret;
> -}
> -
> -static BlockDriver vvfat_write_target = {
> -    .format_name        = "vvfat_write_target",
> -    .instance_size      = sizeof(void*),
> -    .bdrv_co_pwritev    = write_target_commit,
> -};
> -
> static void vvfat_qcow_options(BdrvChildRole role, bool parent_is_format,
>                                int *child_flags, QDict *child_options,
>                                int parent_flags, QDict *parent_options)
> @@ -3133,7 +3113,6 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
> {
>     BDRVVVFATState *s = bs->opaque;
>     BlockDriver *bdrv_qcow = NULL;
> -    BlockDriverState *backing;
>     QemuOpts *opts = NULL;
>     int ret;
>     int size = sector2cluster(s, s->sector_count);
> @@ -3184,13 +3163,6 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
>     unlink(s->qcow_filename);
> #endif
> 
> -    backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
> -                                   &error_abort);
> -    *(void**) backing->opaque = s;
> -
> -    bdrv_set_backing_hd(s->bs, backing, &error_abort);
> -    bdrv_unref(backing);
> -
>     return 0;
> 
> err:
> @@ -3205,17 +3177,10 @@ static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,
>                              uint64_t perm, uint64_t shared,
>                              uint64_t *nperm, uint64_t *nshared)
> {
> -    if (role & BDRV_CHILD_DATA) {
> -        /* This is a private node, nobody should try to attach to it */
> -        *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
> -        *nshared = BLK_PERM_WRITE_UNCHANGED;
> -    } else {
> -        assert(role & BDRV_CHILD_COW);
> -        /* The backing file is there so 'commit' can use it. vvfat doesn't
> -         * access it in any way. */
> -        *nperm = 0;
> -        *nshared = BLK_PERM_ALL;
> -    }
> +    assert(role & BDRV_CHILD_DATA);
> +    /* This is a private node, nobody should try to attach to it */
> +    *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
> +    *nshared = BLK_PERM_WRITE_UNCHANGED;
> }
> 
> static void vvfat_close(BlockDriverState *bs)
> -- 
> 2.29.2
> 

Tested-by: John Arbuckle <programmingkidx@gmail.com>

Am 15.07.2021 um 14:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Most probably this fake backing child doesn't work anyway (see notes
> about it in a8a4d15c1c34d).
> 
> Still, since 25f78d9e2de528473d52 drivers are required to set
> .supports_backing if they want to call bdrv_set_backing_hd, so now
> vvfat just doesn't work because of this check.
> 
> Let's finally drop this fake backing file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks, applied to the block branch.

Kevin