05.05.2021 19:25, Max Reitz wrote:
On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 12:54, Max Reitz wrote:
In the bdrv_snapshot_goto() fallback code, we work with a pointer to
either bs->file or bs->backing.
We close that child,
Do we?
We *detach it.
close the node
(with .bdrv_close()), apply the snapshot on the child node, and then
re-open the node (with .bdrv_open()).
In order for .bdrv_open() to attach the same child node that we had
before, we pass "file={child-node}" or "backing={child-node}" to it.
Therefore, when .bdrv_open() has returned success, we can assume that
bs->file or bs->backing (respectively) points to our original child
again. This is verified by an assertion.
All of this is not immediately clear from a quick glance at the code,
so add a comment to the assertion what it is for, and why it is valid.
It certainly confused Coverity.
Reported-by: Coverity (CID 1452774)
Signed-off-by: Max Reitz <mre...@redhat.com>
---
block/snapshot.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index e8ae9a28c1..cce5e35b35 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
qobject_unref(file_options);
g_free(subqdict_prefix);
+ /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr
*/
qdict_put_str(options, (*fallback_ptr)->name,
bdrv_get_node_name(fallback_bs));
+ /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
if (drv->bdrv_close) {
drv->bdrv_close(bs);
}
+ /* .bdrv_open() will re-attach it */
bdrv_unref_child(bs, *fallback_ptr);
*fallback_ptr = NULL;
@@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret < 0 ? ret : open_ret;
}
- assert(fallback_bs == (*fallback_ptr)->bs);
+ /*
+ * fallback_ptr is &bs->file or &bs->backing. *fallback_ptr
+ * was closed above and set to NULL, but the .bdrv_open() call
+ * has opened it again, because we set the respective option
+ * (with the qdict_put_str() call above).
+ * Assert that .bdrv_open() has attached some child on
+ * *fallback_ptr, and that it has attached the one we wanted
+ * it to (i.e., fallback_bs).
+ */
+ assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
bdrv_unref(fallback_bs);
return ret;
}
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
===
I still think that this fallback has more problems.. Nothing guarantee that all
format drivers (even restricted to those who have only one child) support such
logic.
For example, .bdrv_open() may rely on state structure were zeroed and not
initialize some fields. And .bdrv_close() may just g_free() some things, not
setting them to zero.. So we probably should call bdrv_open()/bdrv_close()
generic functions. But we can't: at least bdrv_close() requires that node has
no parents.
Not saying about that we lose everything on failure (when actually, it's better
to restore original state on failure).
This feature should instead be done with help of bdrv_reopen_multiple(), and
even with it it's not obvious how to do it properly.
The feature were done long ago in 2010 with commit
7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
Note also, that the only protocol driver that support snapshots is rbd, and
snapshot support was added to it in 2012 with commit
bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
Other two drivers with support are sheepdog (which is deprecated) and qcow2 (I
doubt that it should be used as protocol driver for some other format).
Do we really need this fallback? Is it used somewhere? May be, we can just
deprecate it, and look will someone complain?
Maybe? :)
:) I'll send a patch later.
--
Best regards,
Vladimir