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 <kw...@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
