From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51916) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmM2A-00015x-HB for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:28:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmM27-0000A2-8P for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:28:30 -0400 Date: Tue, 20 Sep 2016 16:28:14 +0200 From: Kevin Wolf Message-ID: <20160920142814.GE5807@noname.redhat.com> References: <20160920123126.5400.29283.stgit@PASHA-ISP.def.inno> <20160920123138.5400.67174.stgit@PASHA-ISP.def.inno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160920123138.5400.67174.stgit@PASHA-ISP.def.inno> Subject: Re: [Qemu-devel] [PATCH v3 2/8] block: set snapshot option for block devices in blkreplay module List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgalyuk Cc: qemu-devel@nongnu.org, peter.maydell@linaro.org, jasowang@redhat.com, pbonzini@redhat.com, mst@redhat.com, quintela@redhat.com, qemu-block@nongnu.org [ Cc: qemu-block ] 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 > --- > 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). > + > ret = 0; > fail: > if (ret < 0) { Kevin