[1/2] block: Remove 'backing': null from bs->{explicit_, }options
diff mbox series

Message ID 20191108085312.27049-2-kwolf@redhat.com
State New
Headers show
Series
  • block: Fix multiple blockdev-snapshot calls
Related show

Commit Message

Kevin Wolf Nov. 8, 2019, 8:53 a.m. UTC
bs->options and bs->explicit_options shouldn't contain any options for
child nodes. bdrv_open_inherited() takes care to remove any options that
match a child name after opening the image and the same is done when
reopening.

However, we miss the case of 'backing': null, which is a child option,
but results in no child being created. This means that a 'backing': null
remains in bs->options and bs->explicit_options.

A typical use for 'backing': null is in live snapshots: blockdev-add for
the qcow2 overlay makes sure not to open the backing file (because it is
already opened and blockdev-snapshot will attach it). After doing a
blockdev-snapshot, bs->options and bs->explicit_options become
inconsistent with the actual state (bs has a backing file now, but the
options still say null). On the next occasion that the image is
reopened, e.g. switching it from read-write to read-only when another
snapshot is taken, the option will take effect again and the node
incorrectly loses its backing file.

Fix bdrv_open_inherited() to remove the 'backing' option from
bs->options and bs->explicit_options even for the case where it
specifies that no backing file is wanted.

Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Alberto Garcia Nov. 8, 2019, 2:30 p.m. UTC | #1
On Fri 08 Nov 2019 09:53:11 AM CET, Kevin Wolf wrote:
> bs->options and bs->explicit_options shouldn't contain any options for
> child nodes. bdrv_open_inherited() takes care to remove any options that
> match a child name after opening the image and the same is done when
> reopening.
>
> However, we miss the case of 'backing': null, which is a child option,
> but results in no child being created. This means that a 'backing': null
> remains in bs->options and bs->explicit_options.
>
> A typical use for 'backing': null is in live snapshots: blockdev-add for
> the qcow2 overlay makes sure not to open the backing file (because it is
> already opened and blockdev-snapshot will attach it). After doing a
> blockdev-snapshot, bs->options and bs->explicit_options become
> inconsistent with the actual state (bs has a backing file now, but the
> options still say null). On the next occasion that the image is
> reopened, e.g. switching it from read-write to read-only when another
> snapshot is taken, the option will take effect again and the node
> incorrectly loses its backing file.
>
> Fix bdrv_open_inherited() to remove the 'backing' option from
> bs->options and bs->explicit_options even for the case where it
> specifies that no backing file is wanted.
>
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Peter Krempa Nov. 12, 2019, 4:01 p.m. UTC | #2
On Fri, Nov 08, 2019 at 09:53:11 +0100, Kevin Wolf wrote:
> bs->options and bs->explicit_options shouldn't contain any options for
> child nodes. bdrv_open_inherited() takes care to remove any options that
> match a child name after opening the image and the same is done when
> reopening.
> 
> However, we miss the case of 'backing': null, which is a child option,
> but results in no child being created. This means that a 'backing': null
> remains in bs->options and bs->explicit_options.
> 
> A typical use for 'backing': null is in live snapshots: blockdev-add for
> the qcow2 overlay makes sure not to open the backing file (because it is

Note that we also use '"backing": null' as a terminator for the last
image in the chain if the user configures the chain manually.

This is kind-of a protection from opening the backing file from the
header if it was misconfigured somehow. I think this functionality
should be kept despite probably not making practical sense.

In my testing this scenario worked properly.

> already opened and blockdev-snapshot will attach it). After doing a
> blockdev-snapshot, bs->options and bs->explicit_options become
> inconsistent with the actual state (bs has a backing file now, but the
> options still say null). On the next occasion that the image is
> reopened, e.g. switching it from read-write to read-only when another
> snapshot is taken, the option will take effect again and the node
> incorrectly loses its backing file.
> 
> Fix bdrv_open_inherited() to remove the 'backing' option from
> bs->options and bs->explicit_options even for the case where it
> specifies that no backing file is wanted.
> 
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

The fix looks sane-enough to me and works as expected, but since I'm not
familiar enough with this code I'm comfortable only with a:

Tested-by: Peter Krempa <pkrempa@redhat.com>

> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

Patch
diff mbox series

diff --git a/block.c b/block.c
index dad5a3d8e0..74ba9acb08 100644
--- a/block.c
+++ b/block.c
@@ -3019,6 +3019,8 @@  static BlockDriverState *bdrv_open_inherit(const char *filename,
                         "use \"backing\": null instead");
         }
         flags |= BDRV_O_NO_BACKING;
+        qdict_del(bs->explicit_options, "backing");
+        qdict_del(bs->options, "backing");
         qdict_del(options, "backing");
     }