We should not try to assign a not yet opened node as the backing file,
because as soon as the permission system is added it will fail.  The
just added bdrv_new_open_driver() function is the right tool to open a
file with an internal driver, use it.

In case anyone wonders whether that magic fake backing file to trigger a
special action on 'commit' actually works today: No, not for me. One
reason is that we've been adding a raw format driver on top for several
years now and raw doesn't support commit. Other reasons include that the
backing file isn't writable and the driver doesn't support reopen, and
it's also size 0 and the driver doesn't support bdrv_truncate. All of
these are easily fixable, but then 'commit' ended up in an infinite loop
deep in the vvfat code for me, so I thought I'd best leave it alone. I'm
not really sure what it was supposed to do anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index c6bf67e..7f230be 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2968,6 +2968,7 @@ static void write_target_close(BlockDriverState *bs) {
 
 static BlockDriver vvfat_write_target = {
     .format_name        = "vvfat_write_target",
+    .instance_size      = sizeof(void*),
     .bdrv_co_pwritev    = write_target_commit,
     .bdrv_close         = write_target_close,
 };
@@ -3036,14 +3037,13 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
     unlink(s->qcow_filename);
 #endif
 
-    backing = bdrv_new();
+    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);
     bdrv_unref(backing);
 
-    s->bs->backing->bs->drv = &vvfat_write_target;
-    s->bs->backing->bs->opaque = g_new(void *, 1);
-    *(void**)s->bs->backing->bs->opaque = s;
-
     return 0;
 
 err:
-- 
1.8.3.1

On 21.02.2017 15:58, Kevin Wolf wrote:
> We should not try to assign a not yet opened node as the backing file,
> because as soon as the permission system is added it will fail.  The
> just added bdrv_new_open_driver() function is the right tool to open a
> file with an internal driver, use it.
> 
> In case anyone wonders whether that magic fake backing file to trigger a
> special action on 'commit' actually works today: No, not for me. One
> reason is that we've been adding a raw format driver on top for several
> years now and raw doesn't support commit. Other reasons include that the
> backing file isn't writable and the driver doesn't support reopen, and
> it's also size 0 and the driver doesn't support bdrv_truncate. All of
> these are easily fixable, but then 'commit' ended up in an infinite loop
> deep in the vvfat code for me, so I thought I'd best leave it alone. I'm
> not really sure what it was supposed to do anyway.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

On Tue, 02/21 15:58, Kevin Wolf wrote:
> -    backing = bdrv_new();
> +    backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
> +                                   &error_abort);
> +    *(void**) backing->opaque = s;

Could be simply "backing->opaque = s".

> +
>      bdrv_set_backing_hd(s->bs, backing);
>      bdrv_unref(backing);
>  
> -    s->bs->backing->bs->drv = &vvfat_write_target;
> -    s->bs->backing->bs->opaque = g_new(void *, 1);
> -    *(void**)s->bs->backing->bs->opaque = s;
> -
>      return 0;
>  
>  err:
> -- 
> 1.8.3.1
> 

Fam

Am 23.02.2017 um 12:49 hat Fam Zheng geschrieben:
> On Tue, 02/21 15:58, Kevin Wolf wrote:
> > -    backing = bdrv_new();
> > +    backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
> > +                                   &error_abort);
> > +    *(void**) backing->opaque = s;
> 
> Could be simply "backing->opaque = s".

That's not semantically the same.

Or if you just mean that vvfat shouldn't be using an implicit
BdrvVVFATState with just one pointer element rather than just directly
assigning the pointer to bs->opaque, that might have been an option, but
vvfat isn't written that way and I don't intend to change it at least in
this specific patch. (If I were to change it, I would probably just use
an explicit BdrvVVFATState, though.)

Kevin

On Thu, 02/23 13:25, Kevin Wolf wrote:
> Am 23.02.2017 um 12:49 hat Fam Zheng geschrieben:
> > On Tue, 02/21 15:58, Kevin Wolf wrote:
> > > -    backing = bdrv_new();
> > > +    backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
> > > +                                   &error_abort);
> > > +    *(void**) backing->opaque = s;
> > 
> > Could be simply "backing->opaque = s".
> 
> That's not semantically the same.
> 
> Or if you just mean that vvfat shouldn't be using an implicit
> BdrvVVFATState with just one pointer element rather than just directly
> assigning the pointer to bs->opaque, that might have been an option, but
> vvfat isn't written that way and I don't intend to change it at least in
> this specific patch. (If I were to change it, I would probably just use
> an explicit BdrvVVFATState, though.)

OK, I see it now. Thanks!

Fam