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

[Max Reitz]

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

> +
> +    /* 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").

> +}
> +
>   /*
>    * Opens a disk image (raw, qcow2, vmdk, ...)
>    *
> @@ -1182,8 +1245,6 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>                 BlockDriver *drv, Error **errp)
>   {
>       int ret;
> -    /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> -    char tmp_filename[PATH_MAX + 1];
>       BlockDriverState *file = NULL, *bs;
>       const char *drvname;
>       Error *local_err = NULL;
> @@ -1243,74 +1304,6 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>           }
>       }
>   
> -    /* For snapshot=on, create a temporary qcow2 overlay */
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        BlockDriverState *bs1;
> -        int64_t total_size;
> -        BlockDriver *bdrv_qcow2;
> -        QEMUOptionParameter *create_options;
> -        QDict *snapshot_options;
> -
> -        /* if snapshot, we create a temporary backing file and open it
> -           instead of opening 'filename' directly */
> -
> -        /* Get the required size from the image */
> -        QINCREF(options);
> -        bs1 = NULL;
> -        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
> -                        drv, &local_err);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> -        total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
> -
> -        bdrv_unref(bs1);
> -
> -        /* 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");
> -            goto fail;
> -        }
> -
> -        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);
> -            local_err = NULL;
> -            goto fail;
> -        }
> -
> -        /* Prepare a new options QDict for the temporary file, where user
> -         * options refer to the backing file */
> -        if (filename) {
> -            qdict_put(options, "file.filename", qstring_from_str(filename));
> -        }
> -        if (drv) {
> -            qdict_put(options, "driver", qstring_from_str(drv->format_name));
> -        }
> -
> -        snapshot_options = qdict_new();
> -        qdict_put(snapshot_options, "backing", options);
> -        qdict_flatten(snapshot_options);
> -
> -        bs->options = snapshot_options;
> -        options = qdict_clone_shallow(bs->options);
> -
> -        filename = tmp_filename;
> -        drv = bdrv_qcow2;
> -        bs->is_temporary = 1;
> -    }
> -
>       /* Open image file without format layer */
>       if (flags & BDRV_O_RDWR) {
>           flags |= BDRV_O_ALLOW_RDWR;
> @@ -1372,6 +1365,17 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>           }
>       }
>   
> +    /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> +     * temporary snapshot afterwards. */
> +    if (flags & BDRV_O_SNAPSHOT) {
> +        bdrv_append_temp_snapshot(bs, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto close_and_fail;
> +        }
> +    }
> +
> +
>   done:
>       /* Check if any unknown options were used */
>       if (options && (qdict_size(options) != 0)) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 1ed55d8..b3230a2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,6 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char 
> *filename,
>                       QDict *options, const char *bdref_key, int flags,
>                       bool allow_none, Error **errp);
>   int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error 
> **errp);
> +void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp);
>   int bdrv_open(BlockDriverState **pbs, const char *filename,
>                 const char *reference, QDict *options, int flags,
>                 BlockDriver *drv, Error **errp);
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 2f79b26..d6d926a 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -218,6 +218,8 @@ echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="$TEST_IMG"
>   echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
>   echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2,snapshot=on | _filter_qemu_io
>   echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2 -snapshot | _filter_qemu_io
> +echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="file:$TEST_IMG" -snapshot | _filter_qemu_io
> +echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="file:$TEST_IMG",snapshot=on | _filter_qemu_io
>   
>   $QEMU_IO -c "read -P 0x11 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index a631b0b..dae0f1c 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -319,6 +319,20 @@ wrote 4096/4096 bytes at offset 0
>   4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   (qemu) qququiquit
>   
> +Testing: -drive file=file:TEST_DIR/t.qcow2 -snapshot
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) 
> qqeqemqemuqemu-qemu-iqemu-ioqemu-io
>  qemu-io iqemu-io 
> idqemu-io ideqemu-io 
> ide0qemu-io 
> ide0-qemu-io 
> ide0-hqemu-io 
> ide0-hdqemu-io 
> ide0-hd0qemu-io ide0-hd0 
> qemu-io ide0-hd0 
> "qemu-io ide0-hd0 
> "wqemu-io ide0-hd0 
> "wrqemu-io ide0-hd0 
> "wri!
>   qemu-io ide0-hd0 "writqemu-io ide0-hd0 
> "writeqemu-io ide0-hd0 "write 
> qemu-io ide0-hd0 "write 
> -qemu-io ide0-hd0 "write 
> -Pqemu-io ide0-hd0 "write -P 
> qemu-io ide0-hd0 "write -P 
> 0qemu-io ide0-hd0 "write -P 
> 0xqemu-io ide0-hd0 "write -P 
> 0x2qemu-io ide0-hd0 "w!
>   rite -P 
> 0x22qemu-io 
> ide0-hd0 "write -P 0x22 
> qemu-io 
> ide0-hd0 "write -P 0x22 
> 0qemu-io 
> ide0-hd0 "write -P 0x22 0 
> qemu-io 
> ide0-hd0 "write -P 0x22 0 
> 4qemu-io
>  ide0-hd0 "write -P 0x22 0 
> 4kqemu-io
>  ide0-hd0 "write -P 0x22 0 4k"

The splitting if this one overly long line into three single lines gave 
git a hard time, but after manual fixing, it applies.

> +wrote 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) qququiquit
> +
> +Testing: -drive file=file:TEST_DIR/t.qcow2,snapshot=on
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) 
> qqeqemqemuqemu-qemu-iqemu-ioqemu-io
>  qemu-io iqemu-io 
> idqemu-io ideqemu-io 
> ide0qemu-io 
> ide0-qemu-io 
> ide0-hqemu-io 
> ide0-hdqemu-io 
> ide0-hd0qemu-io ide0-hd0 
> qemu-io ide0-hd0 
> "qemu-io ide0-hd0 
> "wqemu-io ide0-hd0 
> "wrqemu-io ide0-hd0 
> "wri!
>   qemu-io ide0-hd0 "writqemu-io ide0-hd0 
> "writeqemu-io ide0-hd0 "write 
> qemu-io ide0-hd0 "write 
> -qemu-io ide0-hd0 "write 
> -Pqemu-io ide0-hd0 "write -P 
> qemu-io ide0-hd0 "write -P 
> 0qemu-io ide0-hd0 "write -P 
> 0xqemu-io ide0-hd0 "write -P 
> 0x2qemu-io ide0-hd0 "w!
>   rite -P 
> 0x22qemu-io 
> ide0-hd0 "write -P 0x22 
> qemu-io 
> ide0-hd0 "write -P 0x22 
> 0qemu-io 
> ide0-hd0 "write -P 0x22 0 
> qemu-io 
> ide0-hd0 "write -P 0x22 0 
> 4qemu-io
>  ide0-hd0 "write -P 0x22 0 
> 4kqemu-io
>  ide0-hd0 "write -P 0x22 0 4k"
> +wrote 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) qququiquit
> +
>   read 4096/4096 bytes at offset 0
>   4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   Testing: -drive file=TEST_DIR/t.qcow2,snapshot=off

I don't find not checking the return values too bad, as both functions 
are pretty unlikely to fail; normally, I'd probably give a reviewed-by, 
but after just having reviewed the block fix series…

Max