Re: [Qemu-devel] [PATCH 3/3] block: Fix snapshot=on for protocol parsed from filename

[Kevin Wolf <kwolf@redhat.com>]

Am 04.04.2014 um 16:38 hat Max Reitz geschrieben:
> On 04.04.2014 14:03, Kevin Wolf wrote:
> >Since commit 9fd3171a, BDRV_O_SNAPSHOT uses an option QDict to specify
> >the originally requested image as the backing file of the newly created
> >temporary snapshot. This means that the filename is stored in
> >"file.filename", which is an option that is not parsed for protocol
> >names. Therefore things like -drive file=nbd:localhost:10809 were
> >broken because it looked for a local file with the literal name
> >'nbd:localhost:10809'.
> >
> >This patch changes the way BDRV_O_SNAPSHOT works once again. We now open
> >the originally requested image as normal, and then do a similar
> >operation as for live snapshots to put the temporary snapshot on top.
> >This way, both driver specific options and parsed filenames work.
> >
> >As a nice side effect, this results in code movement to factor
> >bdrv_append_temp_snapshot() out. This is a good preparation for moving
> >its call to drive_init() and friends eventually.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block.c                    | 144 +++++++++++++++++++++++----------------------
> >  include/block/block.h      |   1 +
> >  tests/qemu-iotests/051     |   2 +
> >  tests/qemu-iotests/051.out |  14 +++++
> >  4 files changed, 91 insertions(+), 70 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index df2b8d1..7817fea 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -1162,6 +1162,69 @@ done:
> >      return ret;
> >  }
> >+void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
> >+{
> >+    /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> >+    char tmp_filename[PATH_MAX + 1];
> >+
> >+    int64_t total_size;
> >+    BlockDriver *bdrv_qcow2;
> >+    QEMUOptionParameter *create_options;
> >+    QDict *snapshot_options;
> >+    BlockDriverState *bs_snapshot;
> >+    Error *local_err;
> >+    int ret;
> >+
> >+    /* if snapshot, we create a temporary backing file and open it
> >+       instead of opening 'filename' directly */
> >+
> >+    /* Get the required size from the image */
> >+    total_size = bdrv_getlength(bs) & BDRV_SECTOR_MASK;
> 
> bdrv_getlength() may fail.

Aye. This is just code motion, but I'll send a follow-up patch for that.

> >+
> >+    /* Create the temporary image */
> >+    ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename));
> >+    if (ret < 0) {
> >+        error_setg_errno(errp, -ret, "Could not get temporary filename");
> >+        return;
> >+    }
> >+
> >+    bdrv_qcow2 = bdrv_find_format("qcow2");
> >+    create_options = parse_option_parameters("", bdrv_qcow2->create_options,
> >+                                             NULL);
> >+
> >+    set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
> >+
> >+    ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err);
> >+    free_option_parameters(create_options);
> >+    if (ret < 0) {
> >+        error_setg_errno(errp, -ret, "Could not create temporary overlay "
> >+                         "'%s': %s", tmp_filename,
> >+                         error_get_pretty(local_err));
> >+        error_free(local_err);
> >+        return;
> >+    }
> >+
> >+    /* Prepare a new options QDict for the temporary file */
> >+    snapshot_options = qdict_new();
> >+    qdict_put(snapshot_options, "file.driver",
> >+              qstring_from_str("file"));
> >+    qdict_put(snapshot_options, "file.filename",
> >+              qstring_from_str(tmp_filename));
> >+
> >+    bs_snapshot = bdrv_new("");
> >+    bs_snapshot->is_temporary = 1;
> >+
> >+    ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
> >+                    bs->open_flags & ~BDRV_O_SNAPSHOT, bdrv_qcow2, &local_err);
> >+    if (ret < 0) {
> >+        error_propagate(errp, local_err);
> >+        return;
> >+    }
> >+
> >+    bdrv_append(bs_snapshot, bs);
> >+    bdrv_reopen(bs_snapshot, bs_snapshot->open_flags & ~BDRV_O_RDWR, NULL);
> 
> This may fail as well. I don't know how likely that is, but it might
> (for qcow2 at least after your patch "qcow2: Flush metadata during
> read-only reopen").

Right. My v2 that fixes a real bug gets rid of this line, but otherwise
checking and returning failure would indeed have made sense here. I
looked at things like live commit for the reopen and there the best
option was to leave the backing file just r/w if the reopen fails.

Kevin