On 8/9/18 5:31 PM, Max Reitz wrote:
For completeness' sake, add a function for accessing a node's storage
child, too. For filters, this is there filtered child; for non-filters,
s/there/their/
this is bs->file.
Some places are deliberately left unconverted:
- BDS opening/closing functions where bs->file is handled specially
(which is basically wrong, but at least simplifies probing)
- bdrv_co_block_status_from_file(), because its name implies that it
points to ->file
I'm wondering if we can clean up block_status to let filters have a NULL
callback and io.c do the right thing automatically, rather than the
current approach of filters assigning the callback to the common helper
routine. Maybe later in the series.
- bdrv_snapshot_goto() in one places unrefs bs->file. Such a
s/places/place/
modification is not covered by this patch and is therefore just
safeguarded by an additional assert(), but otherwise kept as-is.
Signed-off-by: Max Reitz <mre...@redhat.com>
---
+++ b/block/snapshot.c
@@ -204,37 +207,38 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
- if (bs->file) {
- BlockDriverState *file;
+ storage_bs = bdrv_storage_bs(bs);
+ if (storage_bs) {
QDict *options = qdict_clone_shallow(bs->options);
QDict *file_options;
Error *local_err = NULL;
- file = bs->file->bs;
/* Prevent it from getting deleted when detached from bs */
- bdrv_ref(file);
+ bdrv_ref(storage_bs);
qdict_extract_subqdict(options, &file_options, "file.");
qobject_unref(file_options);
- qdict_put_str(options, "file", bdrv_get_node_name(file));
+ qdict_put_str(options, "file", bdrv_get_node_name(storage_bs));
drv->bdrv_close(bs);
+
+ assert(bs->file->bs == storage_bs);
At first glance, this assertion...
bdrv_unref_child(bs, bs->file);
bs->file = NULL;
- ret = bdrv_snapshot_goto(file, snapshot_id, errp);
+ ret = bdrv_snapshot_goto(storage_bs, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
qobject_unref(options);
if (open_ret < 0) {
- bdrv_unref(file);
+ bdrv_unref(storage_bs);
bs->drv = NULL;
/* A bdrv_snapshot_goto() error takes precedence */
error_propagate(errp, local_err);
return ret < 0 ? ret : open_ret;
}
- assert(bs->file->bs == file);
- bdrv_unref(file);
+ assert(bs->file->bs == storage_bs);
...looks like a duplicate of this one. But looking closer, I see
bs->file = NULL followed by drv->bdrv_open() in between which should
reassign bs->file, so having the assertion on both ends makes sense.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
