Re: [Qemu-devel] [PATCH v3 2/8] block: set snapshot option for block devices in blkreplay module

[Kevin Wolf]

[ 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 <address@hidden>
> ---
>  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