From cbee81f6de57ddc1b21ba28f01f6a3b5d87428a5 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 4 Apr 2014 19:53:29 +0800 Subject: [PATCH 01/10] iscsi: Don't set error if already set in iscsi_do_inquiry This eliminates the possible assertion failure in error_setg(). Signed-off-by: Fam Zheng Reviewed-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block/iscsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 21c18a39dc..64a509f8f4 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1101,8 +1101,10 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun, return task; fail: - error_setg(errp, "iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsi)); + if (!error_is_set(errp)) { + error_setg(errp, "iSCSI: Inquiry command failed : %s", + iscsi_get_error(iscsi)); + } if (task != NULL) { scsi_free_scsi_task(task); } From 4c2e5f8f46a17966dc45b5a3e07b97434c0eabdf Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Apr 2014 13:47:50 +0200 Subject: [PATCH 02/10] qcow2: Flush metadata during read-only reopen If lazy refcounts are enabled for a backing file, committing to this backing file may leave it in a dirty state even if the commit succeeds. The reason is that the bdrv_flush() call in bdrv_commit() doesn't flush refcount updates with lazy refcounts enabled, and qcow2_reopen_prepare() doesn't take care to flush metadata. In order to fix this, this patch also fixes qcow2_mark_clean(), which contains another ineffective bdrv_flush() call beause lazy refcounts are disabled only afterwards. All existing callers of qcow2_mark_clean() either don't modify refcounts or already flush manually, so that this fixes only a latent, but not yet actually triggerable bug. Another instance of the same problem is live snapshots. Again, a real corruption is prevented by an explicit flush for non-read-only images in external_snapshot_prepare(), but images using lazy refcounts stay dirty. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi --- block/qcow2.c | 25 +++++++++++++++++++++---- tests/qemu-iotests/039 | 20 ++++++++++++++++++++ tests/qemu-iotests/039.out | 11 +++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 333e26d733..e903d971c3 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -269,12 +269,15 @@ static int qcow2_mark_clean(BlockDriverState *bs) BDRVQcowState *s = bs->opaque; if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) { - int ret = bdrv_flush(bs); + int ret; + + s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY; + + ret = bdrv_flush(bs); if (ret < 0) { return ret; } - s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY; return qcow2_update_header(bs); } return 0; @@ -900,11 +903,25 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key) return 0; } -/* We have nothing to do for QCOW2 reopen, stubs just return - * success */ +/* We have no actual commit/abort logic for qcow2, but we need to write out any + * unwritten data if we reopen read-only. */ static int qcow2_reopen_prepare(BDRVReopenState *state, BlockReopenQueue *queue, Error **errp) { + int ret; + + if ((state->flags & BDRV_O_RDWR) == 0) { + ret = bdrv_flush(state->bs); + if (ret < 0) { + return ret; + } + + ret = qcow2_mark_clean(state->bs); + if (ret < 0) { + return ret; + } + } + return 0; } diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 index 9b355c0977..b9cbe99560 100755 --- a/tests/qemu-iotests/039 +++ b/tests/qemu-iotests/039 @@ -131,6 +131,26 @@ ulimit -c "$old_ulimit" ./qcow2.py "$TEST_IMG" dump-header | grep incompatible_features _check_test_img +echo +echo "== Committing to a backing file with lazy_refcounts=on ==" + +IMGOPTS="compat=1.1,lazy_refcounts=on" +TEST_IMG="$TEST_IMG".base _make_test_img $size + +IMGOPTS="compat=1.1,lazy_refcounts=on,backing_file=$TEST_IMG.base" +_make_test_img $size + +$QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io +$QEMU_IMG commit "$TEST_IMG" + +# The dirty bit must not be set +./qcow2.py "$TEST_IMG" dump-header | grep incompatible_features +./qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features + +_check_test_img +TEST_IMG="$TEST_IMG".base _check_test_img + + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out index 077fa64cbf..fb31ae0624 100644 --- a/tests/qemu-iotests/039.out +++ b/tests/qemu-iotests/039.out @@ -54,4 +54,15 @@ wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) incompatible_features 0x0 No errors were found on the image. + +== Committing to a backing file with lazy_refcounts=on == +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.base' +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Image committed. +incompatible_features 0x0 +incompatible_features 0x0 +No errors were found on the image. +No errors were found on the image. *** done From 8885eadedd0ea8b57c1baa367ee2c2d616700bd9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Sat, 8 Feb 2014 17:44:59 +0100 Subject: [PATCH 03/10] qcow2: Put cache reference in error case When qcow2_get_cluster_offset() sees a zero cluster in a version 2 image, it (rightfully) returns an error. But in doing so it shouldn't leak an L2 table cache reference. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/qcow2-cluster.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 60a6910b1e..331ab08022 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -491,6 +491,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, break; case QCOW2_CLUSTER_ZERO: if (s->qcow_version < 3) { + qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); return -EIO; } c = count_contiguous_clusters(nb_clusters, s->cluster_size, From e3fa4bfa72d5037bfc1de95cf243d8c57e38f5da Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Apr 2014 12:45:51 +0200 Subject: [PATCH 04/10] block: Don't parse 'filename' option When using the QDict option 'filename', it is supposed to be interpreted literally. The code did correctly avoid guessing the protocol from any string before the first colon, but it still called bdrv_parse_filename() which would, for example, incorrectly remove a 'file:' prefix in the raw-posix driver. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 8 ++++---- tests/qemu-iotests/051 | 4 ++++ tests/qemu-iotests/051.out | 11 +++++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index 7a90a1b25e..df2b8d1b41 100644 --- a/block.c +++ b/block.c @@ -968,7 +968,7 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, { BlockDriver *drv; const char *drvname; - bool allow_protocol_prefix = false; + bool parse_filename = false; Error *local_err = NULL; int ret; @@ -977,7 +977,7 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, filename = qdict_get_try_str(*options, "filename"); } else if (filename && !qdict_haskey(*options, "filename")) { qdict_put(*options, "filename", qstring_from_str(filename)); - allow_protocol_prefix = true; + parse_filename = true; } else { error_setg(errp, "Can't specify 'file' and 'filename' options at the " "same time"); @@ -994,7 +994,7 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, } qdict_del(*options, "driver"); } else if (filename) { - drv = bdrv_find_protocol(filename, allow_protocol_prefix); + drv = bdrv_find_protocol(filename, parse_filename); if (!drv) { error_setg(errp, "Unknown protocol"); } @@ -1010,7 +1010,7 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, } /* Parse the filename and open it */ - if (drv->bdrv_parse_filename && filename) { + if (drv->bdrv_parse_filename && parse_filename) { drv->bdrv_parse_filename(filename, *options, &local_err); if (local_err) { error_propagate(errp, local_err); diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 14694e176b..2f79b26076 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -204,6 +204,10 @@ run_qemu -hda foo:bar run_qemu -drive file=foo:bar run_qemu -drive file.filename=foo:bar +run_qemu -hda "file:$TEST_IMG" +run_qemu -drive file="file:$TEST_IMG" +run_qemu -drive file.filename="file:$TEST_IMG" + echo echo === Snapshot mode === echo diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index f5e33ff395..671ac5f03d 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -275,6 +275,17 @@ QEMU_PROG: -drive file=foo:bar: could not open disk image foo:bar: Unknown proto Testing: -drive file.filename=foo:bar QEMU_PROG: -drive file.filename=foo:bar: could not open disk image ide0-hd0: Could not open 'foo:bar': No such file or directory +Testing: -hda file:TEST_DIR/t.qcow2 +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) qququiquit + +Testing: -drive file=file:TEST_DIR/t.qcow2 +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) qququiquit + +Testing: -drive file.filename=file:TEST_DIR/t.qcow2 +QEMU_PROG: -drive file.filename=file:TEST_DIR/t.qcow2: could not open disk image ide0-hd0: Could not open 'file:TEST_DIR/t.qcow2': No such file or directory + === Snapshot mode === From cd40890816a40ba70d4cd2107629a417f0f3c648 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Apr 2014 12:48:38 +0200 Subject: [PATCH 05/10] qemu-iotests: Remove CR line endings in reference output qemu doesn't print these CRs any more. The test still didn't fail because the output comparison ignores line endings, but the change turns up each time when you want to update the output. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- tests/qemu-iotests/051.out | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 671ac5f03d..a631b0b8c4 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -44,11 +44,11 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TE === Overriding backing file === Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig -nodefaults -QEMU X.Y.Z monitor - type 'help' for more information -(qemu) iininfinfoinfo info binfo blinfo bloinfo blocinfo block -ide0-hd0: TEST_DIR/t.qcow2 (qcow2) - Backing file: TEST_DIR/t.qcow2.orig (chain depth: 1) -(qemu) qququiquit +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) iininfinfoinfo info binfo blinfo bloinfo blocinfo block +ide0-hd0: TEST_DIR/t.qcow2 (qcow2) + Backing file: TEST_DIR/t.qcow2.orig (chain depth: 1) +(qemu) qququiquit === Enable and disable lazy refcounting on the command line, plus some invalid values === From b998875dcf2b21678cffa8b9a83c09930523861f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Apr 2014 12:09:34 +0200 Subject: [PATCH 06/10] block: Fix snapshot=on for protocol parsed from filename 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 Reviewed-by: Max Reitz --- block.c | 148 +++++++++++++++++++------------------ include/block/block.h | 1 + tests/qemu-iotests/051 | 8 ++ tests/qemu-iotests/051.out | 28 +++++++ 4 files changed, 115 insertions(+), 70 deletions(-) diff --git a/block.c b/block.c index df2b8d1b41..d89c344a13 100644 --- a/block.c +++ b/block.c @@ -767,6 +767,11 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags) { int open_flags = flags | BDRV_O_CACHE_WB; + /* The backing file of a temporary snapshot is read-only */ + if (flags & BDRV_O_SNAPSHOT) { + open_flags &= ~BDRV_O_RDWR; + } + /* * Clear flags that are internal to the block layer before opening the * image. @@ -1162,6 +1167,68 @@ 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; + + /* 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); +} + /* * Opens a disk image (raw, qcow2, vmdk, ...) * @@ -1182,8 +1249,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 +1308,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 +1369,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 1ed55d839a..b3230a25f6 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 2f79b26076..073dc7a2d3 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -218,6 +218,14 @@ 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 + +# Opening a read-only file r/w with snapshot=on +chmod u-w "$TEST_IMG" +echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive file="$TEST_IMG" -snapshot | _filter_qemu_io +echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive file="$TEST_IMG",snapshot=on | _filter_qemu_io +chmod u+w "$TEST_IMG" $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 a631b0b8c4..01b0384472 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -319,6 +319,34 @@ 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 "wriqemu-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 "write -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 + +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 "wriqemu-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 "write -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 + +Testing: -drive 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 "wriqemu-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 "write -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 + +Testing: -drive 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 "wriqemu-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 "write -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 From f187743acd39747cc8cc32111518142c924963b9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 4 Apr 2014 17:07:19 +0200 Subject: [PATCH 07/10] block: Check bdrv_getlength() return value in bdrv_append_temp_snapshot() Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index d89c344a13..990a7542a9 100644 --- a/block.c +++ b/block.c @@ -1184,7 +1184,12 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) instead of opening 'filename' directly */ /* Get the required size from the image */ - total_size = bdrv_getlength(bs) & BDRV_SECTOR_MASK; + total_size = bdrv_getlength(bs); + if (total_size < 0) { + error_setg_errno(errp, -total_size, "Could not get image size"); + return; + } + total_size &= BDRV_SECTOR_MASK; /* Create the temporary image */ ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename)); From 4d1cb6e6f51b0d8405f701806a203a73e7431fe5 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 28 Mar 2014 14:22:49 +0000 Subject: [PATCH 08/10] dma-helpers: Initialize DMAAIOCB in_cancel flag Initialize the dbs->in_cancel flag in dma_bdrv_io(), since qemu_aio_get() does not return zero-initialized memory. Spotted by the clang sanitizer (which complained when the value loaded in dma_complete() was not valid for a bool type); this might have resulted in leaking the AIO block. Signed-off-by: Peter Maydell Signed-off-by: Kevin Wolf --- dma-helpers.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dma-helpers.c b/dma-helpers.c index c9620a5bbd..5f421e9814 100644 --- a/dma-helpers.c +++ b/dma-helpers.c @@ -213,6 +213,7 @@ BlockDriverAIOCB *dma_bdrv_io( dbs->sg_cur_index = 0; dbs->sg_cur_byte = 0; dbs->dir = dir; + dbs->in_cancel = false; dbs->io_func = io_func; dbs->bh = NULL; qemu_iovec_init(&dbs->iov, sg->nsg); From 8c2664d86917c987944f1ca9770d1f7bbbf8eca8 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 20 Mar 2014 15:06:31 +0100 Subject: [PATCH 09/10] iothread: make IOThread struct definition public Make the IOThread struct definition public so objects can be embedded in parent structs. Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Tested-by: Christian Borntraeger Signed-off-by: Kevin Wolf --- include/sysemu/iothread.h | 12 +++++++++++- iothread.c | 11 ----------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index a32214a647..7c01a61d5e 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -15,10 +15,20 @@ #define IOTHREAD_H #include "block/aio.h" +#include "qemu/thread.h" #define TYPE_IOTHREAD "iothread" -typedef struct IOThread IOThread; +typedef struct { + Object parent_obj; + + QemuThread thread; + AioContext *ctx; + QemuMutex init_done_lock; + QemuCond init_done_cond; /* is thread initialization done? */ + bool stopping; + int thread_id; +} IOThread; #define IOTHREAD(obj) \ OBJECT_CHECK(IOThread, obj, TYPE_IOTHREAD) diff --git a/iothread.c b/iothread.c index cb5986b6c9..1fbf9f1c49 100644 --- a/iothread.c +++ b/iothread.c @@ -14,7 +14,6 @@ #include "qom/object.h" #include "qom/object_interfaces.h" #include "qemu/module.h" -#include "qemu/thread.h" #include "block/aio.h" #include "sysemu/iothread.h" #include "qmp-commands.h" @@ -22,16 +21,6 @@ #define IOTHREADS_PATH "/objects" typedef ObjectClass IOThreadClass; -struct IOThread { - Object parent_obj; - - QemuThread thread; - AioContext *ctx; - QemuMutex init_done_lock; - QemuCond init_done_cond; /* is thread initialization done? */ - bool stopping; - int thread_id; -}; #define IOTHREAD_GET_CLASS(obj) \ OBJECT_GET_CLASS(IOThreadClass, obj, TYPE_IOTHREAD) From 54bee5c2b487250dcb8631ddff4307f329ec0541 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 20 Mar 2014 15:06:32 +0100 Subject: [PATCH 10/10] dataplane: replace iothread object_add() with embedded instance Before IOThread was its own object, each virtio-blk device would create its own internal thread. We need to preserve this behavior for backwards compatibility when users do not specify -device virtio-blk-pci,iothread=. This patch changes how the internal IOThread object is created. Previously we used the monitor object_add() function, which is really a layering violation. The problem is that this needs to assign a name but we don't have a name for this internal object. Generating names for internal objects is a pain but even worse is that they may collide with user-defined names. Paolo Bonzini suggested that the internal IOThread object should not be named. This way the conflict cannot happen and we no longer need object_add(). One gotcha is that internal IOThread objects will not be listed by the query-iothreads command since they are not named. This is okay though because query-iothreads is new and the internal IOThread is just for backwards compatibility. New users should explicitly define IOThread objects. Reported-by: Christian Borntraeger Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Tested-by: Christian Borntraeger Signed-off-by: Kevin Wolf --- hw/block/dataplane/virtio-blk.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index f558b45a60..70b8a5ab75 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -23,7 +23,7 @@ #include "virtio-blk.h" #include "block/aio.h" #include "hw/virtio/virtio-bus.h" -#include "monitor/monitor.h" /* for object_add() */ +#include "qom/object_interfaces.h" enum { SEG_MAX = 126, /* maximum number of I/O segments */ @@ -59,7 +59,7 @@ struct VirtIOBlockDataPlane { * use it). */ IOThread *iothread; - bool internal_iothread; + IOThread internal_iothread_obj; AioContext *ctx; EventNotifier io_notifier; /* Linux AIO completion */ EventNotifier host_notifier; /* doorbell */ @@ -391,23 +391,19 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, s->blk = blk; if (blk->iothread) { - s->internal_iothread = false; s->iothread = blk->iothread; + object_ref(OBJECT(s->iothread)); } else { - /* Create per-device IOThread if none specified */ - Error *local_err = NULL; - - s->internal_iothread = true; - object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err); - if (error_is_set(&local_err)) { - error_propagate(errp, local_err); - g_free(s); - return; - } - s->iothread = iothread_find(vdev->name); - assert(s->iothread); + /* Create per-device IOThread if none specified. This is for + * x-data-plane option compatibility. If x-data-plane is removed we + * can drop this. + */ + object_initialize(&s->internal_iothread_obj, + sizeof(s->internal_iothread_obj), + TYPE_IOTHREAD); + user_creatable_complete(OBJECT(&s->internal_iothread_obj), &error_abort); + s->iothread = &s->internal_iothread_obj; } - object_ref(OBJECT(s->iothread)); s->ctx = iothread_get_aio_context(s->iothread); /* Prevent block operations that conflict with data plane thread */ @@ -426,9 +422,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) virtio_blk_data_plane_stop(s); bdrv_set_in_use(s->blk->conf.bs, 0); object_unref(OBJECT(s->iothread)); - if (s->internal_iothread) { - object_unparent(OBJECT(s->iothread)); - } g_free(s); }