| Message ID | 20191108085312.27049-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix multiple blockdev-snapshot calls | expand |
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
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(+)
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"); }
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(+)