> From: Kevin Wolf [mailto:kw...@redhat.com] > Am 20.09.2016 um 14:31 hat Pavel Dovgalyuk geschrieben: > > This patch adds overlay option for blkreplay filter. > > It allows creating persistent overlay file for saving and reloading > > VM snapshots in record/replay modes. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru> > > --- > > block/blkreplay.c | 119 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > docs/replay.txt | 8 ++++ > > vl.c | 2 - > > 3 files changed, 128 insertions(+), 1 deletion(-) > > > > diff --git a/block/blkreplay.c b/block/blkreplay.c > > index 30f9d5f..e90d2c6 100644 > > --- a/block/blkreplay.c > > +++ b/block/blkreplay.c > > @@ -14,6 +14,7 @@ > > #include "block/block_int.h" > > #include "sysemu/replay.h" > > #include "qapi/error.h" > > +#include "qapi/qmp/qstring.h" > > > > typedef struct Request { > > Coroutine *co; > > @@ -25,11 +26,82 @@ typedef struct Request { > > block devices should not get overlapping ids. */ > > static uint64_t request_id; > > > > +static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs, > > + int flags, > > + QDict *snapshot_options, > > + Error **errp) > > +{ > > + int ret; > > + BlockDriverState *bs_snapshot; > > + > > + /* Create temporary file, if needed */ > > + if ((flags & BDRV_O_TEMPORARY) || replay_mode == REPLAY_MODE_RECORD) { > > + int64_t total_size; > > + QemuOpts *opts = NULL; > > + const char *tmp_filename = qdict_get_str(snapshot_options, > > + "file.filename"); > > + > > + /* Get the required size from the image */ > > + total_size = bdrv_getlength(bs); > > + if (total_size < 0) { > > + error_setg_errno(errp, -total_size, "Could not get image > > size"); > > + goto out; > > + } > > + > > + opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, > > + &error_abort); > > + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, > > &error_abort); > > + ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp); > > + qemu_opts_del(opts); > > + if (ret < 0) { > > + error_prepend(errp, "Could not create temporary overlay '%s': > > ", > > + tmp_filename); > > + goto out; > > + } > > + } > > + > > + bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp); > > + snapshot_options = NULL; > > + if (!bs_snapshot) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it > > will > > + * call bdrv_unref() on it), so in order to be able to return one, we > > have > > + * to increase bs_snapshot's refcount here */ > > + bdrv_ref(bs_snapshot); > > + bdrv_append(bs_snapshot, bs); > > + > > + return bs_snapshot; > > + > > +out: > > + QDECREF(snapshot_options); > > + return NULL; > > +} > > + > > +static QemuOptsList blkreplay_runtime_opts = { > > + .name = "blkreplay", > > + .head = QTAILQ_HEAD_INITIALIZER(blkreplay_runtime_opts.head), > > + .desc = { > > + { > > + .name = "overlay", > > + .type = QEMU_OPT_STRING, > > + .help = "Optional overlay file for snapshots", > > + }, > > + { /* end of list */ } > > + }, > > +}; > > + > > static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, > > Error **errp) > > { > > Error *local_err = NULL; > > int ret; > > + QDict *snapshot_options = qdict_new(); > > + int snapshot_flags = BDRV_O_RDWR; > > + const char *snapshot; > > + QemuOpts *opts = NULL; > > > > /* Open the image file */ > > bs->file = bdrv_open_child(NULL, options, "image", > > @@ -40,6 +112,43 @@ static int blkreplay_open(BlockDriverState *bs, QDict > > *options, int > flags, > > goto fail; > > } > > > > + opts = qemu_opts_create(&blkreplay_runtime_opts, NULL, 0, > > &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &local_err); > > + if (local_err) { > > + ret = -EINVAL; > > + goto fail; > > + } > > Starting from here... > > > + /* Prepare options QDict for the overlay file */ > > + qdict_put(snapshot_options, "file.driver", > > + qstring_from_str("file")); > > + qdict_put(snapshot_options, "driver", > > + qstring_from_str("qcow2")); > > + > > + snapshot = qemu_opt_get(opts, "overlay"); > > + if (snapshot) { > > + qdict_put(snapshot_options, "file.filename", > > + qstring_from_str(snapshot)); > > + } else { > > + char tmp_filename[PATH_MAX + 1]; > > + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > > + if (ret < 0) { > > + error_setg_errno(errp, -ret, "Could not get temporary > > filename"); > > + goto fail; > > + } > > + qdict_put(snapshot_options, "file.filename", > > + qstring_from_str(tmp_filename)); > > + snapshot_flags |= BDRV_O_TEMPORARY; > > + } > > + > > + /* Add temporary snapshot to preserve the image */ > > + if (!blkreplay_append_snapshot(bs->file->bs, snapshot_flags, > > + snapshot_options, &local_err)) { > > + ret = -EINVAL; > > + error_propagate(errp, local_err); > > + goto fail; > > + } > > ...to here, this is code that is written according to a fundamentally > wrong design. > > The problem here is that you're hard-coding the options that are used > for the overlay image. This restricts users to using only qcow2 (there > are many other formats that support backing files), only on local files > (there are quite a few more protocols over which qcow2 works), only with > default options (e.g. cache mode, discard behaviour, lazy refcounts). > > The correct way would be to get not a filename option, but what is > called a BlockdevRef in QAPI: Either a node-name of a separately created > BDS or an inline definition of a new BDS. Have a look at other filter > drivers for how to do this. The thing to look for is bdrv_open_child() > (though of course an overlay may look a bit different from this).
Thanks. Then I'll better apply Paolo's approach to creating overlays. Pavel Dovgalyuk