From a71835a0ccff168b19ffc9656fe27988821ec59a Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Sat, 8 Feb 2014 14:38:33 +0100 Subject: [PATCH 01/54] qcow2: Set zero flag for discarded clusters Instead of making the backing file contents visible again after a discard request, set the zero flag if possible (i.e. on version >= 3). Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block/qcow2-cluster.c | 22 ++++++++++++++++++++-- tests/qemu-iotests/046 | 18 ++++++++++++++---- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index c57f39dd2b..36c1bed350 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1367,13 +1367,31 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset, uint64_t old_offset; old_offset = be64_to_cpu(l2_table[l2_index + i]); - if ((old_offset & L2E_OFFSET_MASK) == 0) { + + /* + * Make sure that a discarded area reads back as zeroes for v3 images + * (we cannot do it for v2 without actually writing a zero-filled + * buffer). We can skip the operation if the cluster is already marked + * as zero, or if it's unallocated and we don't have a backing file. + * + * TODO We might want to use bdrv_get_block_status(bs) here, but we're + * holding s->lock, so that doesn't work today. + */ + if (old_offset & QCOW_OFLAG_ZERO) { + continue; + } + + if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) { continue; } /* First remove L2 entries */ qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table); - l2_table[l2_index + i] = cpu_to_be64(0); + if (s->qcow_version >= 3) { + l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO); + } else { + l2_table[l2_index + i] = cpu_to_be64(0); + } /* Then decrease the refcount */ qcow2_free_any_clusters(bs, old_offset, 1, type); diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046 index 2d44bbb187..e0be46cf2b 100755 --- a/tests/qemu-iotests/046 +++ b/tests/qemu-iotests/046 @@ -193,6 +193,16 @@ echo "== Verify image content ==" function verify_io() { + if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then + # For v2 images, discarded clusters are read from the backing file + # Keep the variable empty so that the backing file value can be used as + # the default below + discarded= + else + # Discarded clusters are zeroed for v3 or later + discarded=0 + fi + echo read -P 0 0 0x10000 echo read -P 1 0x10000 0x2000 @@ -221,16 +231,16 @@ function verify_io() echo read -P 70 0x78000 0x6000 echo read -P 7 0x7e000 0x2000 - echo read -P 8 0x80000 0x6000 + echo read -P ${discarded:-8} 0x80000 0x6000 echo read -P 80 0x86000 0x2000 - echo read -P 8 0x88000 0x2000 + echo read -P ${discarded:-8} 0x88000 0x2000 echo read -P 81 0x8a000 0xe000 echo read -P 90 0x98000 0x6000 echo read -P 9 0x9e000 0x2000 - echo read -P 10 0xa0000 0x6000 + echo read -P ${discarded:-10} 0xa0000 0x6000 echo read -P 100 0xa6000 0x2000 - echo read -P 10 0xa8000 0x2000 + echo read -P ${discarded:-10} 0xa8000 0x2000 echo read -P 101 0xaa000 0xe000 echo read -P 110 0xb8000 0x8000 From e6dc8a1f83835054fcaf1dcb41af7c868688c068 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 4 Feb 2014 11:45:31 +0100 Subject: [PATCH 02/54] block: Fix bdrv_is_first_non_filter() Consider top level BlockDriverStates as well. Signed-off-by: Kevin Wolf Reviewed-by: Benoit Canet Tested-by: Benoit Canet --- block.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/block.c b/block.c index 6f4bacaa58..034e1ab04d 100644 --- a/block.c +++ b/block.c @@ -5416,11 +5416,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) QTAILQ_FOREACH(bs, &bdrv_states, device_list) { bool perm; - if (!bs->file) { - continue; - } - - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); + perm = bdrv_recurse_is_first_non_filter(bs, candidate); /* candidate is the first non filter */ if (perm) { From f67503e5bd8997ea7ec3f4bfa0af0e06321771a6 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:05 +0100 Subject: [PATCH 03/54] block: Change BDS parameter of bdrv_open() to ** Make bdrv_open() take a pointer to a BDS pointer, similarly to bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open() will create a new BDS with an empty name; if the BDS pointer is not NULL, that existing BDS will be reused (in the same way as bdrv_open() already did). Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 64 +++++++++++++++++++++++++++---------------- block/blkdebug.c | 1 + block/blkverify.c | 2 ++ block/qcow2.c | 14 ++++++---- block/vmdk.c | 5 ++-- block/vvfat.c | 6 ++-- blockdev.c | 20 ++++++-------- hw/block/xen_disk.c | 2 +- include/block/block.h | 2 +- qemu-img.c | 10 +++---- qemu-io.c | 2 +- qemu-nbd.c | 2 +- 12 files changed, 73 insertions(+), 57 deletions(-) diff --git a/block.c b/block.c index 034e1ab04d..c39bd3d198 100644 --- a/block.c +++ b/block.c @@ -1046,7 +1046,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, } if (!drv->bdrv_file_open) { - ret = bdrv_open(bs, filename, options, flags, drv, &local_err); + ret = bdrv_open(&bs, filename, options, flags, drv, &local_err); options = NULL; } else { ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err); @@ -1115,8 +1115,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) sizeof(backing_filename)); } - bs->backing_hd = bdrv_new(""); - if (bs->backing_format[0] != '\0') { back_drv = bdrv_find_format(bs->backing_format); } @@ -1125,11 +1123,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_COPY_ON_READ); - ret = bdrv_open(bs->backing_hd, + assert(bs->backing_hd == NULL); + ret = bdrv_open(&bs->backing_hd, *backing_filename ? backing_filename : NULL, options, back_flags, back_drv, &local_err); if (ret < 0) { - bdrv_unref(bs->backing_hd); bs->backing_hd = NULL; bs->open_flags |= BDRV_O_NO_BACKING; error_setg(errp, "Could not open backing file: %s", @@ -1166,6 +1164,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) * BlockdevRef. * * The BlockdevRef will be removed from the options QDict. + * + * To conform with the behavior of bdrv_open(), *pbs has to be NULL. */ int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, @@ -1176,6 +1176,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, char *bdref_key_dot; const char *reference; + assert(pbs); + assert(*pbs == NULL); + bdref_key_dot = g_strdup_printf("%s.", bdref_key); qdict_extract_subqdict(options, &image_options, bdref_key_dot); g_free(bdref_key_dot); @@ -1196,8 +1199,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, /* If a filename is given and the block driver should be detected automatically (instead of using none), use bdrv_open() in order to do that auto-detection. */ - BlockDriverState *bs; - if (reference) { error_setg(errp, "Cannot reference an existing block device while " "giving a filename"); @@ -1205,13 +1206,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, goto done; } - bs = bdrv_new(""); - ret = bdrv_open(bs, filename, image_options, flags, NULL, errp); - if (ret < 0) { - bdrv_unref(bs); - } else { - *pbs = bs; - } + ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp); } else { ret = bdrv_file_open(pbs, filename, reference, image_options, flags, errp); @@ -1229,17 +1224,28 @@ done: * empty set of options. The reference to the QDict belongs to the block layer * after the call (even on failure), so if the caller intends to reuse the * dictionary, it needs to use QINCREF() before calling bdrv_open. + * + * If *pbs is NULL, a new BDS will be created with a pointer to it stored there. + * If it is not NULL, the referenced BDS will be reused. */ -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, int flags, 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; + BlockDriverState *file = NULL, *bs; const char *drvname; Error *local_err = NULL; + assert(pbs); + + if (*pbs) { + bs = *pbs; + } else { + bs = bdrv_new(""); + } + /* NULL means an empty set of options */ if (options == NULL) { options = qdict_new(); @@ -1260,12 +1266,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, instead of opening 'filename' directly */ /* Get the required size from the image */ - bs1 = bdrv_new(""); QINCREF(options); - ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING, + bs1 = NULL; + ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING, drv, &local_err); if (ret < 0) { - bdrv_unref(bs1); goto fail; } total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK; @@ -1322,6 +1327,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, flags |= BDRV_O_ALLOW_RDWR; } + assert(file == NULL); ret = bdrv_open_image(&file, filename, options, "file", bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true, &local_err); @@ -1393,6 +1399,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, bdrv_dev_change_media_cb(bs, true); } + *pbs = bs; return 0; unlink_and_fail: @@ -1406,13 +1413,24 @@ fail: QDECREF(bs->options); QDECREF(options); bs->options = NULL; + if (!*pbs) { + /* If *pbs is NULL, a new BDS has been created in this function and + needs to be freed now. Otherwise, it does not need to be closed, + since it has not really been opened yet. */ + bdrv_unref(bs); + } if (local_err) { error_propagate(errp, local_err); } return ret; close_and_fail: - bdrv_close(bs); + /* See fail path, but now the BDS has to be always closed */ + if (*pbs) { + bdrv_close(bs); + } else { + bdrv_unref(bs); + } QDECREF(options); if (local_err) { error_propagate(errp, local_err); @@ -5290,9 +5308,8 @@ void bdrv_img_create(const char *filename, const char *fmt, back_flags = flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); - bs = bdrv_new(""); - - ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags, + bs = NULL; + ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags, backing_drv, &local_err); if (ret < 0) { error_setg_errno(errp, -ret, "Could not open '%s': %s", @@ -5300,7 +5317,6 @@ void bdrv_img_create(const char *filename, const char *fmt, error_get_pretty(local_err)); error_free(local_err); local_err = NULL; - bdrv_unref(bs); goto out; } bdrv_get_geometry(bs, &size); diff --git a/block/blkdebug.c b/block/blkdebug.c index ee10013362..46bd086da9 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, s->state = 1; /* Open the backing file */ + assert(bs->file == NULL); ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image", flags, true, false, &local_err); if (ret < 0) { diff --git a/block/blkverify.c b/block/blkverify.c index 1563c88324..7d8a32ec79 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, } /* Open the raw file */ + assert(bs->file == NULL); ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options, "raw", flags, true, false, &local_err); if (ret < 0) { @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, } /* Open the test file */ + assert(s->test_file == NULL); ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options, "test", flags, false, false, &local_err); if (ret < 0) { diff --git a/block/qcow2.c b/block/qcow2.c index b1dbdb120e..309ea12b7d 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size, goto out; } - bdrv_close(bs); + bdrv_unref(bs); + bs = NULL; /* * And now open the image and make it consistent first (i.e. increase the @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, */ BlockDriver* drv = bdrv_find_format("qcow2"); assert(drv != NULL); - ret = bdrv_open(bs, filename, NULL, + ret = bdrv_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size, } } - bdrv_close(bs); + bdrv_unref(bs); + bs = NULL; /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */ - ret = bdrv_open(bs, filename, NULL, + ret = bdrv_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &local_err); if (local_err) { @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size, ret = 0; out: - bdrv_unref(bs); + if (bs) { + bdrv_unref(bs); + } return ret; } diff --git a/block/vmdk.c b/block/vmdk.c index ff6f5ee911..0622db51c4 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, goto exit; } if (backing_file) { - BlockDriverState *bs = bdrv_new(""); - ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp); + BlockDriverState *bs = NULL; + ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp); if (ret != 0) { - bdrv_unref(bs); goto exit; } if (strcmp(bs->drv->format_name, "vmdk")) { diff --git a/block/vvfat.c b/block/vvfat.c index a19e4ca227..0a9f886a20 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s) goto err; } - s->qcow = bdrv_new(""); - - ret = bdrv_open(s->qcow, s->qcow_filename, NULL, + s->qcow = NULL; + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow, &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); - bdrv_unref(s->qcow); goto err; } diff --git a/blockdev.c b/blockdev.c index 3cc8cda2bd..056ec4c8c1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -504,7 +504,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, bdrv_flags |= ro ? 0 : BDRV_O_RDWR; QINCREF(bs_opts); - ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error); + ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error); if (ret < 0) { error_setg(errp, "could not open disk image %s: %s", @@ -1330,12 +1330,12 @@ static void external_snapshot_prepare(BlkTransactionState *common, qstring_from_str(snapshot_node_name)); } - /* We will manually add the backing_hd field to the bs later */ - state->new_bs = bdrv_new(""); /* TODO Inherit bs->options or only take explicit options with an * extended QMP command? */ - ret = bdrv_open(state->new_bs, new_image_file, options, + assert(state->new_bs == NULL); + ret = bdrv_open(&state->new_bs, new_image_file, options, flags | BDRV_O_NO_BACKING, drv, &local_err); + /* We will manually add the backing_hd field to the bs later */ if (ret != 0) { error_propagate(errp, local_err); } @@ -1582,7 +1582,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, Error *local_err = NULL; int ret; - ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err); + ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); return; @@ -2018,10 +2018,9 @@ void qmp_drive_backup(const char *device, const char *target, return; } - target_bs = bdrv_new(""); - ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err); + target_bs = NULL; + ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err); if (ret < 0) { - bdrv_unref(target_bs); error_propagate(errp, local_err); return; } @@ -2162,11 +2161,10 @@ void qmp_drive_mirror(const char *device, const char *target, /* Mirroring takes care of copy-on-write using the source's backing * file. */ - target_bs = bdrv_new(""); - ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv, + target_bs = NULL; + ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv, &local_err); if (ret < 0) { - bdrv_unref(target_bs); error_propagate(errp, local_err); return; } diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c index 098f6c62c7..14e6d0b56e 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev) Error *local_err = NULL; BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly); - if (bdrv_open(blkdev->bs, + if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, qflags, drv, &local_err) != 0) { xen_be_printf(&blkdev->xendev, 0, "error: %s\n", diff --git a/include/block/block.h b/include/block/block.h index 963a61fa4c..980869dd3f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, bool force_raw, bool allow_none, Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, int flags, BlockDriver *drv, Error **errp); BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs, int flags); diff --git a/qemu-img.c b/qemu-img.c index 0927b090de..59b101347c 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, drv = NULL; } - ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err); + ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err); if (ret < 0) { error_report("Could not open '%s': %s", filename, error_get_pretty(local_err)); @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, } return bs; fail: - if (bs) { - bdrv_unref(bs); - } + bdrv_unref(bs); return NULL; } @@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv) bs_old_backing = bdrv_new("old_backing"); bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); - ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS, + ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS, old_backing_drv, &local_err); if (ret) { error_report("Could not open old backing file '%s': %s", @@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv) } if (out_baseimg[0]) { bs_new_backing = bdrv_new("new_backing"); - ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS, + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS, new_backing_drv, &local_err); if (ret) { error_report("Could not open new backing file '%s': %s", diff --git a/qemu-io.c b/qemu-io.c index 7f459d8ffe..8da8f6ec67 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts) } else { qemuio_bs = bdrv_new("hda"); - if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) { + if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) { fprintf(stderr, "%s: can't open device %s: %s\n", progname, name, error_get_pretty(local_err)); error_free(local_err); diff --git a/qemu-nbd.c b/qemu-nbd.c index 136e8c9c05..0cf123cef2 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -597,7 +597,7 @@ int main(int argc, char **argv) bs = bdrv_new("hda"); srcpath = argv[optind]; - ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err); + ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err); if (ret < 0) { errno = -ret; err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind], From ddf5636dc9e4be894f2ab4a5f803d915478b5099 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:06 +0100 Subject: [PATCH 04/54] block: Add reference parameter to bdrv_open() Allow bdrv_open() to handle references to existing block devices just as bdrv_file_open() is already capable of. Signed-off-by: Max Reitz Signed-off-by: Kevin Wolf --- block.c | 44 ++++++++++++++++++++++++++++++++++++------- block/qcow2.c | 4 ++-- block/vmdk.c | 3 ++- block/vvfat.c | 2 +- blockdev.c | 12 ++++++------ hw/block/xen_disk.c | 4 ++-- include/block/block.h | 5 +++-- qemu-img.c | 8 ++++---- qemu-io.c | 4 +++- qemu-nbd.c | 2 +- 10 files changed, 61 insertions(+), 27 deletions(-) diff --git a/block.c b/block.c index c39bd3d198..86a32e7192 100644 --- a/block.c +++ b/block.c @@ -1046,7 +1046,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, } if (!drv->bdrv_file_open) { - ret = bdrv_open(&bs, filename, options, flags, drv, &local_err); + ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err); options = NULL; } else { ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err); @@ -1125,7 +1125,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) assert(bs->backing_hd == NULL); ret = bdrv_open(&bs->backing_hd, - *backing_filename ? backing_filename : NULL, options, + *backing_filename ? backing_filename : NULL, NULL, options, back_flags, back_drv, &local_err); if (ret < 0) { bs->backing_hd = NULL; @@ -1206,7 +1206,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, goto done; } - ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp); + ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp); } else { ret = bdrv_file_open(pbs, filename, reference, image_options, flags, errp); @@ -1227,9 +1227,14 @@ done: * * If *pbs is NULL, a new BDS will be created with a pointer to it stored there. * If it is not NULL, the referenced BDS will be reused. + * + * The reference parameter may be used to specify an existing block device which + * should be opened. If specified, neither options nor a filename may be given, + * nor can an existing BDS be reused (that is, *pbs has to be NULL). */ -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, - int flags, BlockDriver *drv, Error **errp) +int bdrv_open(BlockDriverState **pbs, const char *filename, + const char *reference, QDict *options, int flags, + BlockDriver *drv, Error **errp) { int ret; /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ @@ -1240,6 +1245,31 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, assert(pbs); + if (reference) { + bool options_non_empty = options ? qdict_size(options) : false; + QDECREF(options); + + if (*pbs) { + error_setg(errp, "Cannot reuse an existing BDS when referencing " + "another block device"); + return -EINVAL; + } + + if (filename || options_non_empty) { + error_setg(errp, "Cannot reference an existing block device with " + "additional options or a new filename"); + return -EINVAL; + } + + bs = bdrv_lookup_bs(reference, reference, errp); + if (!bs) { + return -ENODEV; + } + bdrv_ref(bs); + *pbs = bs; + return 0; + } + if (*pbs) { bs = *pbs; } else { @@ -1268,7 +1298,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, /* Get the required size from the image */ QINCREF(options); bs1 = NULL; - ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING, + ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING, drv, &local_err); if (ret < 0) { goto fail; @@ -5309,7 +5339,7 @@ void bdrv_img_create(const char *filename, const char *fmt, flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); bs = NULL; - ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags, + ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags, backing_drv, &local_err); if (ret < 0) { error_setg_errno(errp, -ret, "Could not open '%s': %s", diff --git a/block/qcow2.c b/block/qcow2.c index 309ea12b7d..e2aa5c1b3c 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, */ BlockDriver* drv = bdrv_find_format("qcow2"); assert(drv != NULL); - ret = bdrv_open(&bs, filename, NULL, + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, bs = NULL; /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */ - ret = bdrv_open(&bs, filename, NULL, + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &local_err); if (local_err) { diff --git a/block/vmdk.c b/block/vmdk.c index 0622db51c4..5df486fa8e 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, } if (backing_file) { BlockDriverState *bs = NULL; - ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp); + ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL, + errp); if (ret != 0) { goto exit; } diff --git a/block/vvfat.c b/block/vvfat.c index 0a9f886a20..4bde3bd220 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s) } s->qcow = NULL; - ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, + ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow, &local_err); if (ret < 0) { diff --git a/blockdev.c b/blockdev.c index 056ec4c8c1..357f7607ff 100644 --- a/blockdev.c +++ b/blockdev.c @@ -504,7 +504,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, bdrv_flags |= ro ? 0 : BDRV_O_RDWR; QINCREF(bs_opts); - ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error); + ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error); if (ret < 0) { error_setg(errp, "could not open disk image %s: %s", @@ -1333,7 +1333,7 @@ static void external_snapshot_prepare(BlkTransactionState *common, /* TODO Inherit bs->options or only take explicit options with an * extended QMP command? */ assert(state->new_bs == NULL); - ret = bdrv_open(&state->new_bs, new_image_file, options, + ret = bdrv_open(&state->new_bs, new_image_file, NULL, options, flags | BDRV_O_NO_BACKING, drv, &local_err); /* We will manually add the backing_hd field to the bs later */ if (ret != 0) { @@ -1582,7 +1582,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, Error *local_err = NULL; int ret; - ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err); + ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); return; @@ -2019,7 +2019,7 @@ void qmp_drive_backup(const char *device, const char *target, } target_bs = NULL; - ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err); + ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); return; @@ -2162,8 +2162,8 @@ void qmp_drive_mirror(const char *device, const char *target, * file. */ target_bs = NULL; - ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv, - &local_err); + ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING, + drv, &local_err); if (ret < 0) { error_propagate(errp, local_err); return; diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c index 14e6d0b56e..61e6ff3208 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev) Error *local_err = NULL; BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly); - if (bdrv_open(&blkdev->bs, - blkdev->filename, NULL, qflags, drv, &local_err) != 0) + if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags, + drv, &local_err) != 0) { xen_be_printf(&blkdev->xendev, 0, "error: %s\n", error_get_pretty(local_err)); diff --git a/include/block/block.h b/include/block/block.h index 980869dd3f..a421041690 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, bool force_raw, bool allow_none, Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options, - int flags, BlockDriver *drv, Error **errp); +int bdrv_open(BlockDriverState **pbs, const char *filename, + const char *reference, QDict *options, int flags, + BlockDriver *drv, Error **errp); BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs, int flags); int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp); diff --git a/qemu-img.c b/qemu-img.c index 59b101347c..79ab3e8cbe 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, drv = NULL; } - ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err); + ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err); if (ret < 0) { error_report("Could not open '%s': %s", filename, error_get_pretty(local_err)); @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv) bs_old_backing = bdrv_new("old_backing"); bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); - ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS, + ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS, old_backing_drv, &local_err); if (ret) { error_report("Could not open old backing file '%s': %s", @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv) } if (out_baseimg[0]) { bs_new_backing = bdrv_new("new_backing"); - ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS, - new_backing_drv, &local_err); + ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, + BDRV_O_FLAGS, new_backing_drv, &local_err); if (ret) { error_report("Could not open new backing file '%s': %s", out_baseimg, error_get_pretty(local_err)); diff --git a/qemu-io.c b/qemu-io.c index 8da8f6ec67..61d54c01a0 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts) } else { qemuio_bs = bdrv_new("hda"); - if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) { + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err) + < 0) + { fprintf(stderr, "%s: can't open device %s: %s\n", progname, name, error_get_pretty(local_err)); error_free(local_err); diff --git a/qemu-nbd.c b/qemu-nbd.c index 0cf123cef2..711162c2ec 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -597,7 +597,7 @@ int main(int argc, char **argv) bs = bdrv_new("hda"); srcpath = argv[optind]; - ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err); + ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err); if (ret < 0) { errno = -ret; err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind], From 2e40134bfdbb073512f9f264cb96162787ec62b1 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:07 +0100 Subject: [PATCH 05/54] block: Make bdrv_file_open() static Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the call to bdrv_file_open(). Additionally, make bdrv_file_open() static and therefore bdrv_open() the only way to call it. Consequently, all existing calls to bdrv_file_open() have to be adjusted to use bdrv_open() with the BDRV_O_PROTOCOL flag instead. Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Benoit Canet Signed-off-by: Kevin Wolf --- block.c | 16 +++++++++++----- block/cow.c | 5 +++-- block/qcow.c | 5 +++-- block/qcow2.c | 4 +++- block/qed.c | 8 +++++--- block/sheepdog.c | 7 +++++-- block/vhdx.c | 4 +++- block/vmdk.c | 13 +++++++++---- include/block/block.h | 6 +++--- qemu-io.c | 4 +++- 10 files changed, 48 insertions(+), 24 deletions(-) diff --git a/block.c b/block.c index 86a32e7192..8abbbefbd5 100644 --- a/block.c +++ b/block.c @@ -960,9 +960,9 @@ free_and_fail: * after the call (even on failure), so if the caller intends to reuse the * dictionary, it needs to use QINCREF() before calling bdrv_file_open. */ -int bdrv_file_open(BlockDriverState **pbs, const char *filename, - const char *reference, QDict *options, int flags, - Error **errp) +static int bdrv_file_open(BlockDriverState **pbs, const char *filename, + const char *reference, QDict *options, int flags, + Error **errp) { BlockDriverState *bs = NULL; BlockDriver *drv; @@ -1208,8 +1208,8 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp); } else { - ret = bdrv_file_open(pbs, filename, reference, image_options, flags, - errp); + ret = bdrv_open(pbs, filename, reference, image_options, + flags | BDRV_O_PROTOCOL, NULL, errp); } done: @@ -1245,6 +1245,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, assert(pbs); + if (flags & BDRV_O_PROTOCOL) { + assert(!drv); + return bdrv_file_open(pbs, filename, reference, options, + flags & ~BDRV_O_PROTOCOL, errp); + } + if (reference) { bool options_non_empty = options ? qdict_size(options) : false; QDECREF(options); diff --git a/block/cow.c b/block/cow.c index 7fc0b12163..d0385be1c5 100644 --- a/block/cow.c +++ b/block/cow.c @@ -351,8 +351,9 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, return ret; } - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, - &local_err); + cow_bs = NULL; + ret = bdrv_open(&cow_bs, filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); diff --git a/block/qcow.c b/block/qcow.c index 948b0c5601..8d00853ab5 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -691,8 +691,9 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options, return ret; } - ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR, - &local_err); + qcow_bs = NULL; + ret = bdrv_open(&qcow_bs, filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); diff --git a/block/qcow2.c b/block/qcow2.c index e2aa5c1b3c..9dfd90896b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1493,7 +1493,9 @@ static int qcow2_create2(const char *filename, int64_t total_size, return ret; } - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); + bs = NULL; + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); return ret; diff --git a/block/qed.c b/block/qed.c index b9ca7ac0da..9bc181f041 100644 --- a/block/qed.c +++ b/block/qed.c @@ -562,7 +562,7 @@ static int qed_create(const char *filename, uint32_t cluster_size, size_t l1_size = header.cluster_size * header.table_size; Error *local_err = NULL; int ret = 0; - BlockDriverState *bs = NULL; + BlockDriverState *bs; ret = bdrv_create_file(filename, NULL, &local_err); if (ret < 0) { @@ -571,8 +571,10 @@ static int qed_create(const char *filename, uint32_t cluster_size, return ret; } - ret = bdrv_file_open(&bs, filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err); + bs = NULL; + ret = bdrv_open(&bs, filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL, + &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); diff --git a/block/sheepdog.c b/block/sheepdog.c index e6c0376566..f7bd0242e5 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename) Error *local_err = NULL; int ret; - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + NULL, &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); @@ -1695,7 +1696,9 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, goto out; } - ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err); + bs = NULL; + ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL, + &local_err); if (ret < 0) { qerror_report_err(local_err); error_free(local_err); diff --git a/block/vhdx.c b/block/vhdx.c index 55689cf641..366ff2e627 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1797,7 +1797,9 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options, goto exit; } - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); + bs = NULL; + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto exit; diff --git a/block/vmdk.c b/block/vmdk.c index 5df486fa8e..54a1c59427 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, path_combine(extent_path, sizeof(extent_path), desc_file_path, fname); - ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL, - bs->open_flags, errp); + extent_file = NULL; + ret = bdrv_open(&extent_file, extent_path, NULL, NULL, + bs->open_flags | BDRV_O_PROTOCOL, NULL, errp); if (ret) { return ret; } @@ -1493,7 +1494,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, goto exit; } - ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); + assert(bs == NULL); + ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + NULL, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto exit; @@ -1831,7 +1834,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, goto exit; } } - ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); + assert(new_bs == NULL); + ret = bdrv_open(&new_bs, filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { error_setg_errno(errp, -ret, "Could not write description"); goto exit; diff --git a/include/block/block.h b/include/block/block.h index a421041690..bf78db5ada 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -102,6 +102,9 @@ typedef enum { #define BDRV_O_CHECK 0x1000 /* open solely for consistency check */ #define BDRV_O_ALLOW_RDWR 0x2000 /* allow reopen to change from r/o to r/w */ #define BDRV_O_UNMAP 0x4000 /* execute guest UNMAP/TRIM operations */ +#define BDRV_O_PROTOCOL 0x8000 /* if no block driver is explicitly given: + select an appropriate protocol driver, + ignoring the format layer */ #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH) @@ -183,9 +186,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); int bdrv_parse_cache_flags(const char *mode, int *flags); int bdrv_parse_discard_flags(const char *mode, int *flags); -int bdrv_file_open(BlockDriverState **pbs, const char *filename, - const char *reference, QDict *options, int flags, - Error **errp); int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, bool force_raw, bool allow_none, Error **errp); diff --git a/qemu-io.c b/qemu-io.c index 61d54c01a0..71d7806ed9 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -59,7 +59,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts) } if (growable) { - if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) { + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags | BDRV_O_PROTOCOL, + NULL, &local_err)) + { fprintf(stderr, "%s: can't open device %s: %s\n", progname, name, error_get_pretty(local_err)); error_free(local_err); From 5d12aa63c77b4ee502da9b87de79bf2a9c225ee4 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:08 +0100 Subject: [PATCH 06/54] block: Reuse reference handling from bdrv_open() Remove the reference parameter and the related handling code from bdrv_file_open(), since it exists in bdrv_open() now as well. Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/block.c b/block.c index 8abbbefbd5..29accdde76 100644 --- a/block.c +++ b/block.c @@ -961,8 +961,7 @@ free_and_fail: * dictionary, it needs to use QINCREF() before calling bdrv_file_open. */ static int bdrv_file_open(BlockDriverState **pbs, const char *filename, - const char *reference, QDict *options, int flags, - Error **errp) + QDict *options, int flags, Error **errp) { BlockDriverState *bs = NULL; BlockDriver *drv; @@ -976,23 +975,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename, options = qdict_new(); } - if (reference) { - if (filename || qdict_size(options)) { - error_setg(errp, "Cannot reference an existing block device with " - "additional options or a new filename"); - return -EINVAL; - } - QDECREF(options); - - bs = bdrv_lookup_bs(reference, reference, errp); - if (!bs) { - return -ENODEV; - } - bdrv_ref(bs); - *pbs = bs; - return 0; - } - bs = bdrv_new(""); bs->options = options; options = qdict_clone_shallow(options); @@ -1245,12 +1227,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, assert(pbs); - if (flags & BDRV_O_PROTOCOL) { - assert(!drv); - return bdrv_file_open(pbs, filename, reference, options, - flags & ~BDRV_O_PROTOCOL, errp); - } - if (reference) { bool options_non_empty = options ? qdict_size(options) : false; QDECREF(options); @@ -1276,6 +1252,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, return 0; } + if (flags & BDRV_O_PROTOCOL) { + assert(!drv); + return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL, + errp); + } + if (*pbs) { bs = *pbs; } else { From d4446eae630a363403ec73182cf371deeed4e172 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:09 +0100 Subject: [PATCH 07/54] block: Remove bdrv_new() from bdrv_file_open() Change bdrv_file_open() to take a simple pointer to an already existing BDS instead of an indirect one. The BDS will be created in bdrv_open() if necessary. Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/block.c b/block.c index 29accdde76..39f2b60b41 100644 --- a/block.c +++ b/block.c @@ -960,10 +960,9 @@ free_and_fail: * after the call (even on failure), so if the caller intends to reuse the * dictionary, it needs to use QINCREF() before calling bdrv_file_open. */ -static int bdrv_file_open(BlockDriverState **pbs, const char *filename, +static int bdrv_file_open(BlockDriverState *bs, const char *filename, QDict *options, int flags, Error **errp) { - BlockDriverState *bs = NULL; BlockDriver *drv; const char *drvname; bool allow_protocol_prefix = false; @@ -975,7 +974,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename, options = qdict_new(); } - bs = bdrv_new(""); bs->options = options; options = qdict_clone_shallow(options); @@ -1049,7 +1047,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename, QDECREF(options); bs->growable = 1; - *pbs = bs; return 0; fail: @@ -1057,7 +1054,6 @@ fail: if (!bs->drv) { QDECREF(bs->options); } - bdrv_unref(bs); return ret; } @@ -1252,18 +1248,24 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, return 0; } - if (flags & BDRV_O_PROTOCOL) { - assert(!drv); - return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL, - errp); - } - if (*pbs) { bs = *pbs; } else { bs = bdrv_new(""); } + if (flags & BDRV_O_PROTOCOL) { + assert(!drv); + ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, + errp); + if (ret && !*pbs) { + bdrv_unref(bs); + } else if (!ret) { + *pbs = bs; + } + return ret; + } + /* NULL means an empty set of options */ if (options == NULL) { options = qdict_new(); From 5469a2a688b47bc6d8d224c3f1b02cd96b0e4b65 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:10 +0100 Subject: [PATCH 08/54] block: Handle bs->options in bdrv_open() only The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit similarities, thus it is possible to reuse the one from bdrv_open() and shorten the one in bdrv_file_open() accordingly. Also, setting bs->options in bdrv_file_open() is not necessary if it is already done in bdrv_open(). Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/block.c b/block.c index 39f2b60b41..947a86f5af 100644 --- a/block.c +++ b/block.c @@ -969,14 +969,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, Error *local_err = NULL; int ret; - /* NULL means an empty set of options */ - if (options == NULL) { - options = qdict_new(); - } - - bs->options = options; - options = qdict_clone_shallow(options); - /* Fetch the file name from the options QDict if necessary */ if (!filename) { filename = qdict_get_try_str(options, "filename"); @@ -1051,9 +1043,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, fail: QDECREF(options); - if (!bs->drv) { - QDECREF(bs->options); - } return ret; } @@ -1254,18 +1243,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, bs = bdrv_new(""); } - if (flags & BDRV_O_PROTOCOL) { - assert(!drv); - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, - errp); - if (ret && !*pbs) { - bdrv_unref(bs); - } else if (!ret) { - *pbs = bs; - } - return ret; - } - /* NULL means an empty set of options */ if (options == NULL) { options = qdict_new(); @@ -1274,6 +1251,21 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, bs->options = options; options = qdict_clone_shallow(options); + if (flags & BDRV_O_PROTOCOL) { + assert(!drv); + ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, + &local_err); + options = NULL; + if (!ret) { + *pbs = bs; + return 0; + } else if (bs->drv) { + goto close_and_fail; + } else { + goto fail; + } + } + /* For snapshot=on, create a temporary qcow2 overlay */ if (flags & BDRV_O_SNAPSHOT) { BlockDriverState *bs1; From 5acd9d81e1a59e1929aa3a06571f3fda1101c3a2 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:11 +0100 Subject: [PATCH 09/54] block: Reuse success path from bdrv_open() The fail and success paths of bdrv_file_open() may be further shortened by reusing code already existent in bdrv_open(). This includes bdrv_file_open() not taking the reference to options which allows the removal of QDECREF(options) in that function. Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Benoit Canet Signed-off-by: Kevin Wolf --- block.c | 63 ++++++++++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 34 deletions(-) diff --git a/block.c b/block.c index 947a86f5af..308e7bacb4 100644 --- a/block.c +++ b/block.c @@ -955,13 +955,15 @@ free_and_fail: /* * Opens a file using a protocol (file, host_device, nbd, ...) * - * options is a QDict of options to pass to the block drivers, or NULL for an - * empty set of options. The reference to the QDict belongs to the block layer - * after the call (even on failure), so if the caller intends to reuse the - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. + * options is an indirect pointer to a QDict of options to pass to the block + * drivers, or pointer to NULL for an empty set of options. If this function + * takes ownership of the QDict reference, it will set *options to NULL; + * otherwise, it will contain unused/unrecognized options after this function + * returns. Then, the caller is responsible for freeing it. If it intends to + * reuse the QDict, QINCREF() should be called beforehand. */ static int bdrv_file_open(BlockDriverState *bs, const char *filename, - QDict *options, int flags, Error **errp) + QDict **options, int flags, Error **errp) { BlockDriver *drv; const char *drvname; @@ -971,9 +973,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, /* Fetch the file name from the options QDict if necessary */ if (!filename) { - filename = qdict_get_try_str(options, "filename"); - } else if (filename && !qdict_haskey(options, "filename")) { - qdict_put(options, "filename", qstring_from_str(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; } else { error_setg(errp, "Can't specify 'file' and 'filename' options at the " @@ -983,13 +985,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, } /* Find the right block driver */ - drvname = qdict_get_try_str(options, "driver"); + drvname = qdict_get_try_str(*options, "driver"); if (drvname) { drv = bdrv_find_format(drvname); if (!drv) { error_setg(errp, "Unknown driver '%s'", drvname); } - qdict_del(options, "driver"); + qdict_del(*options, "driver"); } else if (filename) { drv = bdrv_find_protocol(filename, allow_protocol_prefix); if (!drv) { @@ -1008,41 +1010,30 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, /* Parse the filename and open it */ if (drv->bdrv_parse_filename && filename) { - drv->bdrv_parse_filename(filename, options, &local_err); + drv->bdrv_parse_filename(filename, *options, &local_err); if (local_err) { error_propagate(errp, local_err); ret = -EINVAL; goto fail; } - qdict_del(options, "filename"); + qdict_del(*options, "filename"); } if (!drv->bdrv_file_open) { - ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err); - options = NULL; + ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); + *options = NULL; } else { - ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err); + ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err); } if (ret < 0) { error_propagate(errp, local_err); goto fail; } - /* Check if any unknown options were used */ - if (options && (qdict_size(options) != 0)) { - const QDictEntry *entry = qdict_first(options); - error_setg(errp, "Block protocol '%s' doesn't support the option '%s'", - drv->format_name, entry->key); - ret = -EINVAL; - goto fail; - } - QDECREF(options); - bs->growable = 1; return 0; fail: - QDECREF(options); return ret; } @@ -1253,12 +1244,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, if (flags & BDRV_O_PROTOCOL) { assert(!drv); - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, + ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL, &local_err); - options = NULL; if (!ret) { - *pbs = bs; - return 0; + goto done; } else if (bs->drv) { goto close_and_fail; } else { @@ -1395,12 +1384,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, } } +done: /* Check if any unknown options were used */ - if (qdict_size(options) != 0) { + if (options && (qdict_size(options) != 0)) { const QDictEntry *entry = qdict_first(options); - error_setg(errp, "Block format '%s' used by device '%s' doesn't " - "support the option '%s'", drv->format_name, bs->device_name, - entry->key); + if (flags & BDRV_O_PROTOCOL) { + error_setg(errp, "Block protocol '%s' doesn't support the option " + "'%s'", drv->format_name, entry->key); + } else { + error_setg(errp, "Block format '%s' used by device '%s' doesn't " + "support the option '%s'", drv->format_name, + bs->device_name, entry->key); + } ret = -EINVAL; goto close_and_fail; From f7d9fd8c7270de25b1e0d0a462b6958b53aa31b2 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 18 Feb 2014 18:33:12 +0100 Subject: [PATCH 10/54] block: Remove bdrv_open_image()'s force_raw option This option is now unnecessary since specifying BDRV_O_PROTOCOL as flag will do exactly the same. Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 27 ++++----------------------- block/blkdebug.c | 2 +- block/blkverify.c | 4 ++-- include/block/block.h | 2 +- 4 files changed, 8 insertions(+), 27 deletions(-) diff --git a/block.c b/block.c index 308e7bacb4..2fd5482572 100644 --- a/block.c +++ b/block.c @@ -1109,10 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) * Opens a disk image whose options are given as BlockdevRef in another block * device's options. * - * If force_raw is true, bdrv_file_open() will be used, thereby preventing any - * image format auto-detection. If it is false and a filename is given, - * bdrv_open() will be used for auto-detection. - * * If allow_none is true, no image will be opened if filename is false and no * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned. * @@ -1127,7 +1123,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) */ int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, - bool force_raw, bool allow_none, Error **errp) + bool allow_none, Error **errp) { QDict *image_options; int ret; @@ -1153,22 +1149,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, goto done; } - if (filename && !force_raw) { - /* If a filename is given and the block driver should be detected - automatically (instead of using none), use bdrv_open() in order to do - that auto-detection. */ - if (reference) { - error_setg(errp, "Cannot reference an existing block device while " - "giving a filename"); - ret = -EINVAL; - goto done; - } - - ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp); - } else { - ret = bdrv_open(pbs, filename, reference, image_options, - flags | BDRV_O_PROTOCOL, NULL, errp); - } + ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp); done: qdict_del(options, bdref_key); @@ -1330,8 +1311,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, assert(file == NULL); ret = bdrv_open_image(&file, filename, options, "file", - bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true, - &local_err); + bdrv_open_flags(bs, flags | BDRV_O_UNMAP) | + BDRV_O_PROTOCOL, true, &local_err); if (ret < 0) { goto fail; } diff --git a/block/blkdebug.c b/block/blkdebug.c index 46bd086da9..380c736101 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -412,7 +412,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, /* Open the backing file */ assert(bs->file == NULL); ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image", - flags, true, false, &local_err); + flags | BDRV_O_PROTOCOL, false, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto out; diff --git a/block/blkverify.c b/block/blkverify.c index 7d8a32ec79..0e28502e8e 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -137,7 +137,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, /* Open the raw file */ assert(bs->file == NULL); ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options, - "raw", flags, true, false, &local_err); + "raw", flags | BDRV_O_PROTOCOL, false, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto fail; @@ -146,7 +146,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, /* Open the test file */ assert(s->test_file == NULL); ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options, - "test", flags, false, false, &local_err); + "test", flags, false, &local_err); if (ret < 0) { error_propagate(errp, local_err); s->test_file = NULL; diff --git a/include/block/block.h b/include/block/block.h index bf78db5ada..780f48b7b3 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -188,7 +188,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags); int bdrv_parse_discard_flags(const char *mode, int *flags); int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, - bool force_raw, bool allow_none, Error **errp); + bool allow_none, Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); int bdrv_open(BlockDriverState **pbs, const char *filename, const char *reference, QDict *options, int flags, From a69d9af449e9de200abc751d8614124c7486426f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:48 +0100 Subject: [PATCH 11/54] nbd: produce a better error if neither host nor port is passed Before: $ qemu-io-old qemu-io-old> open -r -o file.driver=nbd qemu-io-old: can't open device (null): Could not open image: Invalid argument $ ./qemu-io-old qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar path and host may not be used at the same time. qemu-io-old: can't open device (null): Could not open image: Invalid argument After: $ ./qemu-io qemu-io> open -r -o file.driver=nbd one of path and host must be specified. qemu-io: can't open device (null): Could not open image: Invalid argument $ ./qemu-io qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar path and host may not be used at the same time. qemu-io: can't open device (null): Could not open image: Invalid argument Next patch will fix the error propagation. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/nbd.c | 13 ++++++------- tests/qemu-iotests/051.out | 2 ++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index abae506f04..a83e8565e5 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -192,19 +192,18 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export) { Error *local_err = NULL; - if (qdict_haskey(options, "path")) { - if (qdict_haskey(options, "host")) { + if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) { + if (qdict_haskey(options, "path")) { qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not " "be used at the same time."); - return -EINVAL; + } else { + qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host " + "must be specified."); } - s->client.is_unix = true; - } else if (qdict_haskey(options, "host")) { - s->client.is_unix = false; - } else { return -EINVAL; } + s->client.is_unix = qdict_haskey(options, "path"); s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort); diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 30e2dbd6d7..3e8d9621b7 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -231,6 +231,7 @@ Testing: -drive driver=file QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name Testing: -drive driver=nbd +QEMU_PROG: -drive driver=nbd: one of path and host must be specified. QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument Testing: -drive driver=raw @@ -240,6 +241,7 @@ Testing: -drive file.driver=file QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name Testing: -drive file.driver=nbd +QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified. QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument Testing: -drive file.driver=raw From 77e8b9ca64e85d3d309f322410964b7852ec091e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:49 +0100 Subject: [PATCH 12/54] nbd: correctly propagate errors Before: $ ./qemu-io-old qemu-io-old> open -r -o file.driver=nbd one of path and host must be specified. qemu-io-old: can't open device (null): Could not open image: Invalid argument $ ./qemu-io-old qemu-io-old> open -r -o file.driver=nbd,file.host=foo,file.path=bar path and host may not be used at the same time. qemu-io-old: can't open device (null): Could not open image: Invalid argument After: $ ./qemu-io qemu-io> open -r -o file.driver=nbd qemu-io: can't open device (null): one of path and host must be specified. $ ./qemu-io qemu-io> open -r -o file.driver=nbd,file.host=foo,file.path=bar qemu-io: can't open device (null): path and host may not be used at the same time. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/nbd.c | 34 ++++++++++++++++------------------ include/block/nbd.h | 1 - nbd.c | 12 ------------ tests/qemu-iotests/051.out | 6 ++---- 4 files changed, 18 insertions(+), 35 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index a83e8565e5..55124239df 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -188,19 +188,18 @@ out: g_free(file); } -static int nbd_config(BDRVNBDState *s, QDict *options, char **export) +static void nbd_config(BDRVNBDState *s, QDict *options, char **export, + Error **errp) { Error *local_err = NULL; if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) { if (qdict_haskey(options, "path")) { - qerror_report(ERROR_CLASS_GENERIC_ERROR, "path and host may not " - "be used at the same time."); + error_setg(errp, "path and host may not be used at the same time."); } else { - qerror_report(ERROR_CLASS_GENERIC_ERROR, "one of path and host " - "must be specified."); + error_setg(errp, "one of path and host must be specified."); } - return -EINVAL; + return; } s->client.is_unix = qdict_haskey(options, "path"); @@ -209,9 +208,8 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export) qemu_opts_absorb_qdict(s->socket_opts, options, &local_err); if (local_err) { - qerror_report_err(local_err); - error_free(local_err); - return -EINVAL; + error_propagate(errp, local_err); + return; } if (!qemu_opt_get(s->socket_opts, "port")) { @@ -222,19 +220,17 @@ static int nbd_config(BDRVNBDState *s, QDict *options, char **export) if (*export) { qdict_del(options, "export"); } - - return 0; } -static int nbd_establish_connection(BlockDriverState *bs) +static int nbd_establish_connection(BlockDriverState *bs, Error **errp) { BDRVNBDState *s = bs->opaque; int sock; if (s->client.is_unix) { - sock = unix_socket_outgoing(qemu_opt_get(s->socket_opts, "path")); + sock = unix_connect_opts(s->socket_opts, errp, NULL, NULL); } else { - sock = tcp_socket_outgoing_opts(s->socket_opts); + sock = inet_connect_opts(s->socket_opts, errp, NULL, NULL); if (sock >= 0) { socket_set_nodelay(sock); } @@ -255,17 +251,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, BDRVNBDState *s = bs->opaque; char *export = NULL; int result, sock; + Error *local_err = NULL; /* Pop the config into our state object. Exit if invalid. */ - result = nbd_config(s, options, &export); - if (result != 0) { - return result; + nbd_config(s, options, &export, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; } /* establish TCP connection, return error if it fails * TODO: Configurable retry-until-timeout behaviour. */ - sock = nbd_establish_connection(bs); + sock = nbd_establish_connection(bs, errp); if (sock < 0) { return sock; } diff --git a/include/block/nbd.h b/include/block/nbd.h index c90f5e4d9e..e10ab82155 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -64,7 +64,6 @@ enum { ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read); int tcp_socket_incoming(const char *address, uint16_t port); int tcp_socket_incoming_spec(const char *address_and_port); -int tcp_socket_outgoing_opts(QemuOpts *opts); int unix_socket_outgoing(const char *path); int unix_socket_incoming(const char *path); diff --git a/nbd.c b/nbd.c index 030f56b5c7..17ca95bf0d 100644 --- a/nbd.c +++ b/nbd.c @@ -199,18 +199,6 @@ static void combine_addr(char *buf, size_t len, const char* address, } } -int tcp_socket_outgoing_opts(QemuOpts *opts) -{ - Error *local_err = NULL; - int fd = inet_connect_opts(opts, &local_err, NULL, NULL); - if (local_err != NULL) { - qerror_report_err(local_err); - error_free(local_err); - } - - return fd; -} - int tcp_socket_incoming(const char *address, uint16_t port) { char address_and_port[128]; diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 3e8d9621b7..7de18704f8 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -231,8 +231,7 @@ Testing: -drive driver=file QEMU_PROG: -drive driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name Testing: -drive driver=nbd -QEMU_PROG: -drive driver=nbd: one of path and host must be specified. -QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument +QEMU_PROG: -drive driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified. Testing: -drive driver=raw QEMU_PROG: -drive driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level @@ -241,8 +240,7 @@ Testing: -drive file.driver=file QEMU_PROG: -drive file.driver=file: could not open disk image ide0-hd0: The 'file' block driver requires a file name Testing: -drive file.driver=nbd -QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified. -QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: Could not open image: Invalid argument +QEMU_PROG: -drive file.driver=nbd: could not open disk image ide0-hd0: one of path and host must be specified. Testing: -drive file.driver=raw QEMU_PROG: -drive file.driver=raw: could not open disk image ide0-hd0: Can't use 'raw' as a block driver for the protocol level From c06b72781dc6dff3f1e8209b7280ff4650eb6f36 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:50 +0100 Subject: [PATCH 13/54] nbd: inline tcp_socket_incoming_spec into sole caller Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- include/block/nbd.h | 1 - nbd.c | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index e10ab82155..1b39c064f5 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -63,7 +63,6 @@ enum { ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read); int tcp_socket_incoming(const char *address, uint16_t port); -int tcp_socket_incoming_spec(const char *address_and_port); int unix_socket_outgoing(const char *path); int unix_socket_incoming(const char *path); diff --git a/nbd.c b/nbd.c index 17ca95bf0d..2fc1f1fbf8 100644 --- a/nbd.c +++ b/nbd.c @@ -202,13 +202,9 @@ static void combine_addr(char *buf, size_t len, const char* address, int tcp_socket_incoming(const char *address, uint16_t port) { char address_and_port[128]; - combine_addr(address_and_port, 128, address, port); - return tcp_socket_incoming_spec(address_and_port); -} - -int tcp_socket_incoming_spec(const char *address_and_port) -{ Error *local_err = NULL; + + combine_addr(address_and_port, 128, address, port); int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err); if (local_err != NULL) { From 537b41f5013e1951fa15e8f18855b18d76124ce4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:51 +0100 Subject: [PATCH 14/54] nbd: move socket wrappers to qemu-nbd qemu-nbd is one of the few valid users of qerror_report_err. Move the error-reporting socket wrappers there. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- include/block/nbd.h | 4 ---- nbd.c | 50 ------------------------------------------- qemu-nbd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 54 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 1b39c064f5..79502a090b 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -62,10 +62,6 @@ enum { #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024) ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read); -int tcp_socket_incoming(const char *address, uint16_t port); -int unix_socket_outgoing(const char *path); -int unix_socket_incoming(const char *path); - int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags, off_t *size, size_t *blocksize); int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize); diff --git a/nbd.c b/nbd.c index 2fc1f1fbf8..e5084b6e7c 100644 --- a/nbd.c +++ b/nbd.c @@ -188,56 +188,6 @@ static ssize_t write_sync(int fd, void *buffer, size_t size) return ret; } -static void combine_addr(char *buf, size_t len, const char* address, - uint16_t port) -{ - /* If the address-part contains a colon, it's an IPv6 IP so needs [] */ - if (strstr(address, ":")) { - snprintf(buf, len, "[%s]:%u", address, port); - } else { - snprintf(buf, len, "%s:%u", address, port); - } -} - -int tcp_socket_incoming(const char *address, uint16_t port) -{ - char address_and_port[128]; - Error *local_err = NULL; - - combine_addr(address_and_port, 128, address, port); - int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err); - - if (local_err != NULL) { - qerror_report_err(local_err); - error_free(local_err); - } - return fd; -} - -int unix_socket_incoming(const char *path) -{ - Error *local_err = NULL; - int fd = unix_listen(path, NULL, 0, &local_err); - - if (local_err != NULL) { - qerror_report_err(local_err); - error_free(local_err); - } - return fd; -} - -int unix_socket_outgoing(const char *path) -{ - Error *local_err = NULL; - int fd = unix_connect(path, &local_err); - - if (local_err != NULL) { - qerror_report_err(local_err); - error_free(local_err); - } - return fd; -} - /* Basic flow for negotiation Server Client diff --git a/qemu-nbd.c b/qemu-nbd.c index 711162c2ec..c1dfff3b85 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -20,6 +20,8 @@ #include "block/block.h" #include "block/nbd.h" #include "qemu/main-loop.h" +#include "qemu/sockets.h" +#include "qemu/error-report.h" #include "block/snapshot.h" #include @@ -201,6 +203,56 @@ static void termsig_handler(int signum) qemu_notify_event(); } +static void combine_addr(char *buf, size_t len, const char* address, + uint16_t port) +{ + /* If the address-part contains a colon, it's an IPv6 IP so needs [] */ + if (strstr(address, ":")) { + snprintf(buf, len, "[%s]:%u", address, port); + } else { + snprintf(buf, len, "%s:%u", address, port); + } +} + +static int tcp_socket_incoming(const char *address, uint16_t port) +{ + char address_and_port[128]; + Error *local_err = NULL; + + combine_addr(address_and_port, 128, address, port); + int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err); + + if (local_err != NULL) { + qerror_report_err(local_err); + error_free(local_err); + } + return fd; +} + +static int unix_socket_incoming(const char *path) +{ + Error *local_err = NULL; + int fd = unix_listen(path, NULL, 0, &local_err); + + if (local_err != NULL) { + qerror_report_err(local_err); + error_free(local_err); + } + return fd; +} + +static int unix_socket_outgoing(const char *path) +{ + Error *local_err = NULL; + int fd = unix_connect(path, &local_err); + + if (local_err != NULL) { + qerror_report_err(local_err); + error_free(local_err); + } + return fd; +} + static void *show_parts(void *arg) { char *device = arg; From 35cb1748d54c8e56881a5e10138b3eb090f3a6bc Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:52 +0100 Subject: [PATCH 15/54] iscsi: fix indentation Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/iscsi.c | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index f8e496f8ef..ac5a5c73e3 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1065,35 +1065,36 @@ static QemuOptsList runtime_opts = { }, }; -static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, - int lun, int evpd, int pc) { - int full_size; - struct scsi_task *task = NULL; - task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64); +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun, + int evpd, int pc) +{ + int full_size; + struct scsi_task *task = NULL; + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64); + if (task == NULL || task->status != SCSI_STATUS_GOOD) { + goto fail; + } + full_size = scsi_datain_getfullsize(task); + if (full_size > task->datain.size) { + scsi_free_scsi_task(task); + + /* we need more data for the full list */ + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size); if (task == NULL || task->status != SCSI_STATUS_GOOD) { goto fail; } - full_size = scsi_datain_getfullsize(task); - if (full_size > task->datain.size) { - scsi_free_scsi_task(task); + } - /* we need more data for the full list */ - task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size); - if (task == NULL || task->status != SCSI_STATUS_GOOD) { - goto fail; - } - } - - return task; + return task; fail: - error_report("iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsi)); - if (task) { - scsi_free_scsi_task(task); - return NULL; - } + error_report("iSCSI: Inquiry command failed : %s", + iscsi_get_error(iscsi)); + if (task) { + scsi_free_scsi_task(task); return NULL; + } + return NULL; } /* From f2917853f715b0ef55df29eb2ffea29dc69ce814 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:53 +0100 Subject: [PATCH 16/54] iscsi: correctly propagate errors in iscsi_open Before: $ ./qemu-io-old qemu-io-old> open -r -o file.driver=iscsi,file.filename=foo Failed to parse URL : foo qemu-io-old: can't open device (null): Could not open 'foo': Invalid argument After: $ ./qemu-io qemu-io> open -r -o file.driver=iscsi,file.filename=foo qemu-io: can't open device (null): Failed to parse URL : foo Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/iscsi.c | 103 ++++++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 50 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index ac5a5c73e3..41ec09709d 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -856,7 +856,8 @@ retry: #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */ -static int parse_chap(struct iscsi_context *iscsi, const char *target) +static void parse_chap(struct iscsi_context *iscsi, const char *target, + Error **errp) { QemuOptsList *list; QemuOpts *opts; @@ -865,37 +866,35 @@ static int parse_chap(struct iscsi_context *iscsi, const char *target) list = qemu_find_opts("iscsi"); if (!list) { - return 0; + return; } opts = qemu_opts_find(list, target); if (opts == NULL) { opts = QTAILQ_FIRST(&list->head); if (!opts) { - return 0; + return; } } user = qemu_opt_get(opts, "user"); if (!user) { - return 0; + return; } password = qemu_opt_get(opts, "password"); if (!password) { - error_report("CHAP username specified but no password was given"); - return -1; + error_setg(errp, "CHAP username specified but no password was given"); + return; } if (iscsi_set_initiator_username_pwd(iscsi, user, password)) { - error_report("Failed to set initiator username and password"); - return -1; + error_setg(errp, "Failed to set initiator username and password"); } - - return 0; } -static void parse_header_digest(struct iscsi_context *iscsi, const char *target) +static void parse_header_digest(struct iscsi_context *iscsi, const char *target, + Error **errp) { QemuOptsList *list; QemuOpts *opts; @@ -928,7 +927,7 @@ static void parse_header_digest(struct iscsi_context *iscsi, const char *target) } else if (!strcmp(digest, "NONE-CRC32C")) { iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C); } else { - error_report("Invalid header-digest setting : %s", digest); + error_setg(errp, "Invalid header-digest setting : %s", digest); } } @@ -986,12 +985,11 @@ static void iscsi_nop_timed_event(void *opaque) } #endif -static int iscsi_readcapacity_sync(IscsiLun *iscsilun) +static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp) { struct scsi_task *task = NULL; struct scsi_readcapacity10 *rc10 = NULL; struct scsi_readcapacity16 *rc16 = NULL; - int ret = 0; int retries = ISCSI_CMD_RETRIES; do { @@ -1006,8 +1004,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun) if (task != NULL && task->status == SCSI_STATUS_GOOD) { rc16 = scsi_datain_unmarshall(task); if (rc16 == NULL) { - error_report("iSCSI: Failed to unmarshall readcapacity16 data."); - ret = -EINVAL; + error_setg(errp, "iSCSI: Failed to unmarshall readcapacity16 data."); } else { iscsilun->block_size = rc16->block_length; iscsilun->num_blocks = rc16->returned_lba + 1; @@ -1021,8 +1018,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun) if (task != NULL && task->status == SCSI_STATUS_GOOD) { rc10 = scsi_datain_unmarshall(task); if (rc10 == NULL) { - error_report("iSCSI: Failed to unmarshall readcapacity10 data."); - ret = -EINVAL; + error_setg(errp, "iSCSI: Failed to unmarshall readcapacity10 data."); } else { iscsilun->block_size = rc10->block_size; if (rc10->lba == 0) { @@ -1035,20 +1031,18 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun) } break; default: - return 0; + return; } } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION && task->sense.key == SCSI_SENSE_UNIT_ATTENTION && retries-- > 0); if (task == NULL || task->status != SCSI_STATUS_GOOD) { - error_report("iSCSI: failed to send readcapacity10 command."); - ret = -EINVAL; + error_setg(errp, "iSCSI: failed to send readcapacity10 command."); } if (task) { scsi_free_scsi_task(task); } - return ret; } /* TODO Convert to fine grained options */ @@ -1066,7 +1060,7 @@ static QemuOptsList runtime_opts = { }; static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun, - int evpd, int pc) + int evpd, int pc, Error **errp) { int full_size; struct scsi_task *task = NULL; @@ -1088,8 +1082,8 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun, return task; fail: - error_report("iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsi)); + error_setg(errp, "iSCSI: Inquiry command failed : %s", + iscsi_get_error(iscsi)); if (task) { scsi_free_scsi_task(task); return NULL; @@ -1120,27 +1114,25 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, int ret; if ((BDRV_SECTOR_SIZE % 512) != 0) { - error_report("iSCSI: Invalid BDRV_SECTOR_SIZE. " - "BDRV_SECTOR_SIZE(%lld) is not a multiple " - "of 512", BDRV_SECTOR_SIZE); + error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. " + "BDRV_SECTOR_SIZE(%lld) is not a multiple " + "of 512", BDRV_SECTOR_SIZE); return -EINVAL; } opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); ret = -EINVAL; goto out; } filename = qemu_opt_get(opts, "filename"); - iscsi_url = iscsi_parse_full_url(iscsi, filename); if (iscsi_url == NULL) { - error_report("Failed to parse URL : %s", filename); + error_setg(errp, "Failed to parse URL : %s", filename); ret = -EINVAL; goto out; } @@ -1151,13 +1143,13 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, iscsi = iscsi_create_context(initiator_name); if (iscsi == NULL) { - error_report("iSCSI: Failed to create iSCSI context."); + error_setg(errp, "iSCSI: Failed to create iSCSI context."); ret = -ENOMEM; goto out; } if (iscsi_set_targetname(iscsi, iscsi_url->target)) { - error_report("iSCSI: Failed to set target name."); + error_setg(errp, "iSCSI: Failed to set target name."); ret = -EINVAL; goto out; } @@ -1166,21 +1158,22 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, ret = iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user, iscsi_url->passwd); if (ret != 0) { - error_report("Failed to set initiator username and password"); + error_setg(errp, "Failed to set initiator username and password"); ret = -EINVAL; goto out; } } /* check if we got CHAP username/password via the options */ - if (parse_chap(iscsi, iscsi_url->target) != 0) { - error_report("iSCSI: Failed to set CHAP user/password"); + parse_chap(iscsi, iscsi_url->target, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); ret = -EINVAL; goto out; } if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) { - error_report("iSCSI: Failed to set session type to normal."); + error_setg(errp, "iSCSI: Failed to set session type to normal."); ret = -EINVAL; goto out; } @@ -1188,10 +1181,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C); /* check if we got HEADER_DIGEST via the options */ - parse_header_digest(iscsi, iscsi_url->target); + parse_header_digest(iscsi, iscsi_url->target, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto out; + } if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) { - error_report("iSCSI: Failed to connect to LUN : %s", + error_setg(errp, "iSCSI: Failed to connect to LUN : %s", iscsi_get_error(iscsi)); ret = -EINVAL; goto out; @@ -1203,14 +1201,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 0, 0, 36); if (task == NULL || task->status != SCSI_STATUS_GOOD) { - error_report("iSCSI: failed to send inquiry command."); + error_setg(errp, "iSCSI: failed to send inquiry command."); ret = -EINVAL; goto out; } inq = scsi_datain_unmarshall(task); if (inq == NULL) { - error_report("iSCSI: Failed to unmarshall inquiry data."); + error_setg(errp, "iSCSI: Failed to unmarshall inquiry data."); ret = -EINVAL; goto out; } @@ -1218,7 +1216,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, iscsilun->type = inq->periperal_device_type; iscsilun->has_write_same = true; - if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) { + iscsi_readcapacity_sync(iscsilun, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); goto out; } bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun); @@ -1236,14 +1236,15 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, if (iscsilun->lbpme) { struct scsi_inquiry_logical_block_provisioning *inq_lbp; task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING); + SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, + errp); if (task == NULL) { ret = -EINVAL; goto out; } inq_lbp = scsi_datain_unmarshall(task); if (inq_lbp == NULL) { - error_report("iSCSI: failed to unmarshall inquiry datain blob"); + error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob"); ret = -EINVAL; goto out; } @@ -1256,14 +1257,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) { struct scsi_inquiry_block_limits *inq_bl; task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, - SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS); + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS, errp); if (task == NULL) { ret = -EINVAL; goto out; } inq_bl = scsi_datain_unmarshall(task); if (inq_bl == NULL) { - error_report("iSCSI: failed to unmarshall inquiry datain blob"); + error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob"); ret = -EINVAL; goto out; } @@ -1354,14 +1355,16 @@ static int iscsi_reopen_prepare(BDRVReopenState *state, static int iscsi_truncate(BlockDriverState *bs, int64_t offset) { IscsiLun *iscsilun = bs->opaque; - int ret = 0; + Error *local_err = NULL; if (iscsilun->type != TYPE_DISK) { return -ENOTSUP; } - if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) { - return ret; + iscsi_readcapacity_sync(iscsilun, &local_err); + if (local_err != NULL) { + error_free(local_err); + return -EIO; } if (offset > iscsi_getlength(bs)) { From 24897a767bd778fc6a050537d024565f9272cd06 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:54 +0100 Subject: [PATCH 17/54] gluster: default scheme to gluster:// and host to localhost. Currently, "gluster:///volname/img" and (using file. options) "file.driver=gluster,file.filename=foo" will segfault. Also, "//host/volname/img" will be rejected, but it is a valid URL that should be accepted just fine with "file.driver=gluster". Accept all of these, by inferring missing transport and host as TCP and localhost respectively. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/gluster.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 58eab07829..89450efd2c 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -127,7 +127,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) } /* transport */ - if (!strcmp(uri->scheme, "gluster")) { + if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { gconf->transport = g_strdup("tcp"); } else if (!strcmp(uri->scheme, "gluster+tcp")) { gconf->transport = g_strdup("tcp"); @@ -163,7 +163,7 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) } gconf->server = g_strdup(qp->p[0].value); } else { - gconf->server = g_strdup(uri->server); + gconf->server = g_strdup(uri->server ? uri->server : "localhost"); gconf->port = uri->port; } From a7451cb850d115f257080aff3fbc54f255ebf8f7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:55 +0100 Subject: [PATCH 18/54] gluster: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/gluster.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 89450efd2c..14d390b4c7 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -175,7 +175,8 @@ out: return ret; } -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename) +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename, + Error **errp) { struct glfs *glfs = NULL; int ret; @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename) ret = qemu_gluster_parseuri(gconf, filename); if (ret < 0) { - error_report("Usage: file=gluster[+transport]://[server[:port]]/" - "volname/image[?socket=...]"); + error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/" + "volname/image[?socket=...]"); errno = -ret; goto out; } @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename) ret = glfs_init(glfs); if (ret) { - error_report("Gluster connection failed for server=%s port=%d " - "volume=%s image=%s transport=%s", gconf->server, gconf->port, - gconf->volname, gconf->image, gconf->transport); + error_setg_errno(errp, errno, + "Gluster connection failed for server=%s port=%d " + "volume=%s image=%s transport=%s", gconf->server, + gconf->port, gconf->volname, gconf->image, + gconf->transport); goto out; } return glfs; @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); ret = -EINVAL; goto out; } filename = qemu_opt_get(opts, "filename"); - s->glfs = qemu_gluster_init(gconf, filename); + s->glfs = qemu_gluster_init(gconf, filename, errp); if (!s->glfs) { ret = -errno; goto out; @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename, int64_t total_size = 0; GlusterConf *gconf = g_malloc0(sizeof(GlusterConf)); - glfs = qemu_gluster_init(gconf, filename); + glfs = qemu_gluster_init(gconf, filename, errp); if (!glfs) { - ret = -errno; + ret = -EINVAL; goto out; } From f8d924e48167ec14ec4556441ec7999a30ef6640 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:56 +0100 Subject: [PATCH 19/54] cow: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/cow.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/block/cow.c b/block/cow.c index d0385be1c5..9e4f624b01 100644 --- a/block/cow.c +++ b/block/cow.c @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, char version[64]; snprintf(version, sizeof(version), "COW version %d", cow_header.version); - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "cow", version); ret = -ENOTSUP; goto fail; @@ -346,8 +346,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, ret = bdrv_create_file(filename, options, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } @@ -355,8 +354,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, ret = bdrv_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } From 2a94fee3f649bdd2d71c78bb56977284f096f842 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:57 +0100 Subject: [PATCH 20/54] curl: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/curl.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/block/curl.c b/block/curl.c index bb1fc4ae28..3494c6d662 100644 --- a/block/curl.c +++ b/block/curl.c @@ -456,30 +456,27 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, static int inited = 0; if (flags & BDRV_O_RDWR) { - qerror_report(ERROR_CLASS_GENERIC_ERROR, - "curl block device does not support writes"); + error_setg(errp, "curl block device does not support writes"); return -EROFS; } opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); goto out_noclean; } s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE); if ((s->readahead_size & 0x1ff) != 0) { - fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n", - s->readahead_size); + error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512", + s->readahead_size); goto out_noclean; } file = qemu_opt_get(opts, "url"); if (file == NULL) { - qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires " - "an 'url' option"); + error_setg(errp, "curl block driver requires an 'url' option"); goto out_noclean; } From b6d5066d32f9e6c3d7508c1af9ae78327a927120 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:58 +0100 Subject: [PATCH 21/54] qcow: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/qcow.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/qcow.c b/block/qcow.c index 8d00853ab5..ef8920bf07 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -119,17 +119,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, if (header.version != QCOW_VERSION) { char version[64]; snprintf(version, sizeof(version), "QCOW version %d", header.version); - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, - bs->device_name, "qcow", version); + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, + bs->device_name, "qcow", version); ret = -ENOTSUP; goto fail; } if (header.size <= 1 || header.cluster_bits < 9) { + error_setg(errp, "invalid value in qcow header"); ret = -EINVAL; goto fail; } if (header.crypt_method > QCOW_CRYPT_AES) { + error_setg(errp, "invalid encryption method in qcow header"); ret = -EINVAL; goto fail; } @@ -686,8 +688,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options, ret = bdrv_create_file(filename, options, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } @@ -695,8 +696,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options, ret = bdrv_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } From 0fea6b797202c9efea534a474220a1cf23dd1968 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:43:59 +0100 Subject: [PATCH 22/54] qed: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/qed.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/block/qed.c b/block/qed.c index 9bc181f041..968028ecbd 100644 --- a/block/qed.c +++ b/block/qed.c @@ -398,7 +398,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags, char buf[64]; snprintf(buf, sizeof(buf), "%" PRIx64, s->header.features & ~QED_FEATURE_MASK); - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "QED", buf); return -ENOTSUP; } @@ -545,7 +545,8 @@ static void bdrv_qed_close(BlockDriverState *bs) static int qed_create(const char *filename, uint32_t cluster_size, uint64_t image_size, uint32_t table_size, - const char *backing_file, const char *backing_fmt) + const char *backing_file, const char *backing_fmt, + Error **errp) { QEDHeader header = { .magic = QED_MAGIC, @@ -566,8 +567,7 @@ static int qed_create(const char *filename, uint32_t cluster_size, ret = bdrv_create_file(filename, NULL, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } @@ -576,8 +576,7 @@ static int qed_create(const char *filename, uint32_t cluster_size, BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL, &local_err); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); return ret; } @@ -667,7 +666,7 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options, } return qed_create(filename, cluster_size, image_size, table_size, - backing_file, backing_fmt); + backing_file, backing_fmt, errp); } typedef struct { From 6890aad46b14849318053fe3ace6109e0f9c5932 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:00 +0100 Subject: [PATCH 23/54] vhdx: correctly propagate errors Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vhdx.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/block/vhdx.c b/block/vhdx.c index 366ff2e627..5390ba6d0f 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -402,9 +402,10 @@ int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s, } /* opens the specified header block from the VHDX file header section */ -static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s) +static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s, + Error **errp) { - int ret = 0; + int ret; VHDXHeader *header1; VHDXHeader *header2; bool h1_valid = false; @@ -462,7 +463,6 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s) } else if (!h1_valid && h2_valid) { s->curr_header = 1; } else if (!h1_valid && !h2_valid) { - ret = -EINVAL; goto fail; } else { /* If both headers are valid, then we choose the active one by the @@ -473,27 +473,22 @@ static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s) } else if (h2_seq > h1_seq) { s->curr_header = 1; } else { - ret = -EINVAL; goto fail; } } vhdx_region_register(s, s->headers[s->curr_header]->log_offset, s->headers[s->curr_header]->log_length); - - ret = 0; - goto exit; fail: - qerror_report(ERROR_CLASS_GENERIC_ERROR, "No valid VHDX header found"); + error_setg_errno(errp, -ret, "No valid VHDX header found"); qemu_vfree(header1); qemu_vfree(header2); s->headers[0] = NULL; s->headers[1] = NULL; exit: qemu_vfree(buffer); - return ret; } @@ -878,7 +873,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, int ret = 0; uint32_t i; uint64_t signature; - + Error *local_err = NULL; s->bat = NULL; s->first_visible_write = true; @@ -901,8 +896,10 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, * header update */ vhdx_guid_generate(&s->session_guid); - ret = vhdx_parse_header(bs, s); - if (ret < 0) { + vhdx_parse_header(bs, s, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + ret = -EINVAL; goto fail; } From c0f92b526dbd45fc5b539f51b7379156814dafe9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:01 +0100 Subject: [PATCH 24/54] vvfat: correctly propagate errors Before: $ ./qemu-io-old qemu-io-old> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu Valid FAT types are only 12, 16 and 32 qemu-io-old: can't open device (null): Could not open image: Invalid argument After: $ ./qemu-io qemu-io> open -r -o driver=vvfat,fat-type=24,dir=i386-softmmu qemu-io: can't open device (null): Valid FAT types are only 12, 16 and 32 Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vvfat.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/block/vvfat.c b/block/vvfat.c index 4bde3bd220..f966ea5da8 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1086,16 +1086,14 @@ DLOG(if (stderr == NULL) { opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { - qerror_report_err(local_err); - error_free(local_err); + error_propagate(errp, local_err); ret = -EINVAL; goto fail; } dirname = qemu_opt_get(opts, "dir"); if (!dirname) { - qerror_report(ERROR_CLASS_GENERIC_ERROR, "vvfat block driver requires " - "a 'dir' option"); + error_setg(errp, "vvfat block driver requires a 'dir' option"); ret = -EINVAL; goto fail; } @@ -1135,8 +1133,7 @@ DLOG(if (stderr == NULL) { case 12: break; default: - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Valid FAT types are only " - "12, 16 and 32"); + error_setg(errp, "Valid FAT types are only 12, 16 and 32"); ret = -EINVAL; goto fail; } From a8842e6d2acc815e9660cc743bd0b0daba18c935 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:02 +0100 Subject: [PATCH 25/54] vmdk: extract vmdk_read_desc Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 54a1c59427..b80810802a 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -529,6 +529,32 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, static int vmdk_open_desc_file(BlockDriverState *bs, int flags, uint64_t desc_offset, Error **errp); +static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset, + Error **errp) +{ + int64_t size; + char *buf; + int ret; + + size = bdrv_getlength(file); + if (size < 0) { + error_setg_errno(errp, -size, "Could not access file"); + return NULL; + } + + size = MIN(size, 1 << 20); /* avoid unbounded allocation */ + buf = g_malloc0(size + 1); + + ret = bdrv_pread(file, desc_offset, buf, size); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not read from file"); + g_free(buf); + return NULL; + } + + return buf; +} + static int vmdk_open_vmdk4(BlockDriverState *bs, BlockDriverState *file, int flags, Error **errp) @@ -823,23 +849,16 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, uint64_t desc_offset, Error **errp) { int ret; - char *buf = NULL; + char *buf; char ct[128]; BDRVVmdkState *s = bs->opaque; - int64_t size; - size = bdrv_getlength(bs->file); - if (size < 0) { + buf = vmdk_read_desc(bs->file, desc_offset, errp); + if (!buf) { return -EINVAL; - } - - size = MIN(size, 1 << 20); /* avoid unbounded allocation */ - buf = g_malloc0(size + 1); - - ret = bdrv_pread(bs->file, desc_offset, buf, size); - if (ret < 0) { goto exit; } + if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { ret = -EMEDIUMTYPE; goto exit; From d1833ef52be349e41d17e9c5ddaea8bb4ad3a7fb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:03 +0100 Subject: [PATCH 26/54] vmdk: push vmdk_read_desc up to caller Currently, we just try reading a VMDK file as both image and descriptor. This makes it hard to choose which of the two attempts gave the best error. We'll decide in advance if the file looks like an image or a descriptor, and this patch is the first step to that end. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 55 +++++++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index b80810802a..f34c16db7c 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -526,8 +526,8 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, return ret; } -static int vmdk_open_desc_file(BlockDriverState *bs, int flags, - uint64_t desc_offset, Error **errp); +static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf, + Error **errp); static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset, Error **errp) @@ -576,7 +576,13 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, if (header.capacity == 0) { uint64_t desc_offset = le64_to_cpu(header.desc_offset); if (desc_offset) { - return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp); + char *buf = vmdk_read_desc(file, desc_offset << 9, errp); + if (!buf) { + return -EINVAL; + } + ret = vmdk_open_desc_file(bs, flags, buf, errp); + g_free(buf); + return ret; } } @@ -727,16 +733,12 @@ static int vmdk_parse_description(const char *desc, const char *opt_name, /* Open an extent file and append to bs array */ static int vmdk_open_sparse(BlockDriverState *bs, - BlockDriverState *file, - int flags, Error **errp) + BlockDriverState *file, int flags, + char *buf, Error **errp) { uint32_t magic; - if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) { - return -EIO; - } - - magic = be32_to_cpu(magic); + magic = ldl_be_p(buf); switch (magic) { case VMDK3_MAGIC: return vmdk_open_vmfs_sparse(bs, file, flags, errp); @@ -821,8 +823,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, extent->flat_start_offset = flat_offset << 9; } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) { /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse file*/ - ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp); + char *buf = vmdk_read_desc(extent_file, 0, errp); + if (!buf) { + ret = -EINVAL; + } else { + ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, buf, errp); + } if (ret) { + g_free(buf); bdrv_unref(extent_file); return ret; } @@ -845,20 +853,13 @@ next_line: return 0; } -static int vmdk_open_desc_file(BlockDriverState *bs, int flags, - uint64_t desc_offset, Error **errp) +static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf, + Error **errp) { int ret; - char *buf; char ct[128]; BDRVVmdkState *s = bs->opaque; - buf = vmdk_read_desc(bs->file, desc_offset, errp); - if (!buf) { - return -EINVAL; - goto exit; - } - if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { ret = -EMEDIUMTYPE; goto exit; @@ -876,20 +877,25 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, s->desc_offset = 0; ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp); exit: - g_free(buf); return ret; } static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + char *buf = NULL; int ret; BDRVVmdkState *s = bs->opaque; - if (vmdk_open_sparse(bs, bs->file, flags, errp) == 0) { + buf = vmdk_read_desc(bs->file, 0, errp); + if (!buf) { + return -EINVAL; + } + + if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) { s->desc_offset = 0x200; } else { - ret = vmdk_open_desc_file(bs, flags, 0, errp); + ret = vmdk_open_desc_file(bs, flags, buf, errp); if (ret) { goto fail; } @@ -908,10 +914,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, "vmdk", bs->device_name, "live migration"); migrate_add_blocker(s->migration_blocker); - + g_free(buf); return 0; fail: + g_free(buf); g_free(s->create_type); s->create_type = NULL; vmdk_free_extents(bs); From 37f09e5e3d206e7c555d28a7755cecfa137dad22 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:04 +0100 Subject: [PATCH 27/54] vmdk: do not try opening a file as both image and descriptor This prepares for propagating errors from vmdk_open_sparse and vmdk_open_desc_file up to the caller of vmdk_open. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 22 +++++++++++++++------- tests/qemu-iotests/059.out | 4 ++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index f34c16db7c..50a279a8d8 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -886,20 +886,28 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, char *buf = NULL; int ret; BDRVVmdkState *s = bs->opaque; + uint32_t magic; buf = vmdk_read_desc(bs->file, 0, errp); if (!buf) { return -EINVAL; } - if (vmdk_open_sparse(bs, bs->file, flags, buf, errp) == 0) { - s->desc_offset = 0x200; - } else { - ret = vmdk_open_desc_file(bs, flags, buf, errp); - if (ret) { - goto fail; - } + magic = ldl_be_p(buf); + switch (magic) { + case VMDK3_MAGIC: + case VMDK4_MAGIC: + ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp); + s->desc_offset = 0x200; + break; + default: + ret = vmdk_open_desc_file(bs, flags, buf, errp); + break; } + if (ret) { + goto fail; + } + /* try to open parent images, if exist */ ret = vmdk_parent_open(bs); if (ret) { diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out index 4ffeb54710..4600670161 100644 --- a/tests/qemu-iotests/059.out +++ b/tests/qemu-iotests/059.out @@ -8,7 +8,7 @@ no file open, try 'help open' === Testing too big L2 table size === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 L2 table size too big -qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Wrong medium type +qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument no file open, try 'help open' === Testing too big L1 table size === @@ -2046,7 +2046,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1 === Testing truncated sparse === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 qemu-img: File truncated, expecting at least 13172736 bytes -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Wrong medium type +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument === Testing version 3 === image: TEST_DIR/iotest-version3.IMGFMT From 89ac8480a8c7f73dd943dcb1313d6bd984f9a870 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:05 +0100 Subject: [PATCH 28/54] vmdk: correctly propagate errors Now that we can return the "right" errors, use the Error** parameter to pass them back instead of just printing them. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 11 ++++++----- tests/qemu-iotests/059.out | 6 ++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 50a279a8d8..9c3711cbbf 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -572,6 +572,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, error_setg_errno(errp, -ret, "Could not read header from file '%s'", file->filename); + return -EINVAL; } if (header.capacity == 0) { uint64_t desc_offset = le64_to_cpu(header.desc_offset); @@ -641,8 +642,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, char buf[64]; snprintf(buf, sizeof(buf), "VMDK version %d", le32_to_cpu(header.version)); - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, - bs->device_name, "vmdk", buf); + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, + bs->device_name, "vmdk", buf); return -ENOTSUP; } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) { /* VMware KB 2064959 explains that version 3 added support for @@ -654,7 +655,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, } if (le32_to_cpu(header.num_gtes_per_gt) > 512) { - error_report("L2 table size too big"); + error_setg(errp, "L2 table size too big"); return -EINVAL; } @@ -670,8 +671,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, } if (bdrv_getlength(file) < le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) { - error_report("File truncated, expecting at least %lld bytes", - le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE); + error_setg(errp, "File truncated, expecting at least %lld bytes", + le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE); return -EINVAL; } diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out index 4600670161..3371c867bb 100644 --- a/tests/qemu-iotests/059.out +++ b/tests/qemu-iotests/059.out @@ -7,8 +7,7 @@ no file open, try 'help open' === Testing too big L2 table size === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -L2 table size too big -qemu-io: can't open device TEST_DIR/t.vmdk: Could not open 'TEST_DIR/t.vmdk': Invalid argument +qemu-io: can't open device TEST_DIR/t.vmdk: L2 table size too big no file open, try 'help open' === Testing too big L1 table size === @@ -2045,8 +2044,7 @@ RW 12582912 VMFS "dummy.IMGFMT" 1 === Testing truncated sparse === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 -qemu-img: File truncated, expecting at least 13172736 bytes -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at least 13172736 bytes === Testing version 3 === image: TEST_DIR/iotest-version3.IMGFMT From 76abe4071d111a9ca6dcc9b9689a831c39ffa718 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:06 +0100 Subject: [PATCH 29/54] block: do not abuse EMEDIUMTYPE Returning "Wrong medium type" for an image that does not have a valid header is a bit weird. Improve the error by mentioning what format was trying to open it. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/bochs.c | 3 ++- block/cow.c | 3 ++- block/parallels.c | 3 ++- block/qcow.c | 3 ++- block/qcow2.c | 2 +- block/qed.c | 3 ++- block/vdi.c | 4 ++-- block/vmdk.c | 6 ++++-- block/vpc.c | 3 ++- 9 files changed, 19 insertions(+), 11 deletions(-) diff --git a/block/bochs.c b/block/bochs.c index 51d9a90577..4d6403f904 100644 --- a/block/bochs.c +++ b/block/bochs.c @@ -129,7 +129,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags, strcmp(bochs.subtype, GROWING_TYPE) || ((le32_to_cpu(bochs.version) != HEADER_VERSION) && (le32_to_cpu(bochs.version) != HEADER_V1))) { - return -EMEDIUMTYPE; + error_setg(errp, "Image not in Bochs format"); + return -EINVAL; } if (le32_to_cpu(bochs.version) == HEADER_V1) { diff --git a/block/cow.c b/block/cow.c index 9e4f624b01..30deb88deb 100644 --- a/block/cow.c +++ b/block/cow.c @@ -74,7 +74,8 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, } if (be32_to_cpu(cow_header.magic) != COW_MAGIC) { - ret = -EMEDIUMTYPE; + error_setg(errp, "Image not in COW format"); + ret = -EINVAL; goto fail; } diff --git a/block/parallels.c b/block/parallels.c index 2121e43204..3f588f58dc 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -85,7 +85,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, if (memcmp(ph.magic, HEADER_MAGIC, 16) || (le32_to_cpu(ph.version) != HEADER_VERSION)) { - ret = -EMEDIUMTYPE; + error_setg(errp, "Image not in Parallels format"); + ret = -EINVAL; goto fail; } diff --git a/block/qcow.c b/block/qcow.c index ef8920bf07..1e128becf0 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -113,7 +113,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, be64_to_cpus(&header.l1_table_offset); if (header.magic != QCOW_MAGIC) { - ret = -EMEDIUMTYPE; + error_setg(errp, "Image not in qcow format"); + ret = -EINVAL; goto fail; } if (header.version != QCOW_VERSION) { diff --git a/block/qcow2.c b/block/qcow2.c index 9dfd90896b..cfe80befa0 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -449,7 +449,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, if (header.magic != QCOW_MAGIC) { error_setg(errp, "Image is not in qcow2 format"); - ret = -EMEDIUMTYPE; + ret = -EINVAL; goto fail; } if (header.version < 2 || header.version > 3) { diff --git a/block/qed.c b/block/qed.c index 968028ecbd..8802ad3845 100644 --- a/block/qed.c +++ b/block/qed.c @@ -391,7 +391,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags, qed_header_le_to_cpu(&le_header, &s->header); if (s->header.magic != QED_MAGIC) { - return -EMEDIUMTYPE; + error_setg(errp, "Image not in QED format"); + return -EINVAL; } if (s->header.features & ~QED_FEATURE_MASK) { /* image uses unsupported feature bits */ diff --git a/block/vdi.c b/block/vdi.c index 2d7490f173..f3c6acf3cf 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -395,8 +395,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, } if (header.signature != VDI_SIGNATURE) { - logout("bad vdi signature %08x\n", header.signature); - ret = -EMEDIUMTYPE; + error_setg(errp, "Image not in VDI format (bad signature %08x)", header.signature); + ret = -EINVAL; goto fail; } else if (header.version != VDI_VERSION_1_1) { logout("unsupported version %u.%u\n", diff --git a/block/vmdk.c b/block/vmdk.c index 9c3711cbbf..83839f9b7a 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -748,7 +748,8 @@ static int vmdk_open_sparse(BlockDriverState *bs, return vmdk_open_vmdk4(bs, file, flags, errp); break; default: - return -EMEDIUMTYPE; + error_setg(errp, "Image not in VMDK format"); + return -EINVAL; break; } } @@ -862,7 +863,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf, BDRVVmdkState *s = bs->opaque; if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { - ret = -EMEDIUMTYPE; + error_setg(errp, "invalid VMDK image descriptor"); + ret = -EINVAL; goto exit; } if (strcmp(ct, "monolithicFlat") && diff --git a/block/vpc.c b/block/vpc.c index 1d326cbf44..82bf2485a5 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -190,7 +190,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } if (strncmp(footer->creator, "conectix", 8)) { - ret = -EMEDIUMTYPE; + error_setg(errp, "invalid VPC image"); + ret = -EINVAL; goto fail; } disk_type = VHD_FIXED; From 5b7aa9b56d1bfc79916262f380c3fc7961becb50 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Feb 2014 14:44:07 +0100 Subject: [PATCH 30/54] vdi: say why an image is bad Instead of just putting it in debugging output, we can now put the value in an Error. Signed-off-by: Paolo Bonzini Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vdi.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index f3c6acf3cf..ae49cd83ca 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -399,39 +399,46 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, ret = -EINVAL; goto fail; } else if (header.version != VDI_VERSION_1_1) { - logout("unsupported version %u.%u\n", - header.version >> 16, header.version & 0xffff); + error_setg(errp, "unsupported VDI image (version %u.%u)", + header.version >> 16, header.version & 0xffff); ret = -ENOTSUP; goto fail; } else if (header.offset_bmap % SECTOR_SIZE != 0) { /* We only support block maps which start on a sector boundary. */ - logout("unsupported block map offset 0x%x B\n", header.offset_bmap); + error_setg(errp, "unsupported VDI image (unaligned block map offset " + "0x%x)", header.offset_bmap); ret = -ENOTSUP; goto fail; } else if (header.offset_data % SECTOR_SIZE != 0) { /* We only support data blocks which start on a sector boundary. */ - logout("unsupported data offset 0x%x B\n", header.offset_data); + error_setg(errp, "unsupported VDI image (unaligned data offset 0x%x)", + header.offset_data); ret = -ENOTSUP; goto fail; } else if (header.sector_size != SECTOR_SIZE) { - logout("unsupported sector size %u B\n", header.sector_size); + error_setg(errp, "unsupported VDI image (sector size %u is not %u)", + header.sector_size, SECTOR_SIZE); ret = -ENOTSUP; goto fail; } else if (header.block_size != 1 * MiB) { - logout("unsupported block size %u B\n", header.block_size); + error_setg(errp, "unsupported VDI image (sector size %u is not %u)", + header.block_size, 1 * MiB); ret = -ENOTSUP; goto fail; } else if (header.disk_size > (uint64_t)header.blocks_in_image * header.block_size) { - logout("unsupported disk size %" PRIu64 " B\n", header.disk_size); + error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", " + "image bitmap has room for %" PRIu64 ")", + header.disk_size, + (uint64_t)header.blocks_in_image * header.block_size); ret = -ENOTSUP; goto fail; } else if (!uuid_is_null(header.uuid_link)) { - logout("link uuid != 0, unsupported\n"); + error_setg(errp, "unsupported VDI image (non-NULL link UUID)"); ret = -ENOTSUP; goto fail; } else if (!uuid_is_null(header.uuid_parent)) { - logout("parent uuid != 0, unsupported\n"); + error_setg(errp, "unsupported VDI image (non-NULL parent UUID)"); ret = -ENOTSUP; goto fail; } From 7cc07ab8daa01f100f36ab63382d491f2d278c64 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:03 +0100 Subject: [PATCH 31/54] qemu-option: has_help_option() and is_valid_option_list() has_help_option() checks if any help option ('help' or '?') occurs anywhere in an option string, so that things like 'cluster_size=4k,help' are recognised. is_valid_option_list() ensures that the option list doesn't have options with leading commas or trailing unescaped commas. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- include/qemu/option.h | 2 ++ util/qemu-option.c | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/include/qemu/option.h b/include/qemu/option.h index 3ea871a3ba..8c0ac3485e 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -79,6 +79,8 @@ void parse_option_size(const char *name, const char *value, void free_option_parameters(QEMUOptionParameter *list); void print_option_parameters(QEMUOptionParameter *list); void print_option_help(QEMUOptionParameter *list); +bool has_help_option(const char *param); +bool is_valid_option_list(const char *param); /* ------------------------------------------------------------------ */ diff --git a/util/qemu-option.c b/util/qemu-option.c index fd76cd2ada..9d898af443 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -450,6 +450,55 @@ fail: return NULL; } +bool has_help_option(const char *param) +{ + size_t buflen = strlen(param) + 1; + char *buf = g_malloc0(buflen); + const char *p = param; + bool result = false; + + while (*p) { + p = get_opt_value(buf, buflen, p); + if (*p) { + p++; + } + + if (is_help_option(buf)) { + result = true; + goto out; + } + } + +out: + free(buf); + return result; +} + +bool is_valid_option_list(const char *param) +{ + size_t buflen = strlen(param) + 1; + char *buf = g_malloc0(buflen); + const char *p = param; + bool result = true; + + while (*p) { + p = get_opt_value(buf, buflen, p); + if (*p && !*++p) { + result = false; + goto out; + } + + if (!*buf || *buf == ',') { + result = false; + goto out; + } + } + +out: + free(buf); + return result; +} + /* * Prints all options of a list that have a value to stdout */ From 77386bf6ebe67164a2d102b207fb3bc11af8c1e8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:04 +0100 Subject: [PATCH 32/54] qemu-img create: Support multiple -o options If you specified multiple -o options for qemu-img create, it would silently ignore all but the last one. This patch fixes the problem. Now multiple -o options has the same meaning as having a single option with all settings in the order of their respective -o options. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- qemu-img.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 79ab3e8cbe..9c1643d0c1 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -369,13 +369,23 @@ static int img_create(int argc, char **argv) case 'e': error_report("option -e is deprecated, please use \'-o " "encryption\' instead!"); - return 1; + goto fail; case '6': error_report("option -6 is deprecated, please use \'-o " "compat6\' instead!"); - return 1; + goto fail; case 'o': - options = optarg; + if (!is_valid_option_list(optarg)) { + error_report("Invalid option list: %s", optarg); + goto fail; + } + if (!options) { + options = g_strdup(optarg); + } else { + char *old_options = options; + options = g_strdup_printf("%s,%s", options, optarg); + g_free(old_options); + } break; case 'q': quiet = true; @@ -403,7 +413,7 @@ static int img_create(int argc, char **argv) error_report("kilobytes, megabytes, gigabytes, terabytes, " "petabytes and exabytes."); } - return 1; + goto fail; } img_size = (uint64_t)sval; } @@ -411,7 +421,8 @@ static int img_create(int argc, char **argv) help(); } - if (options && is_help_option(options)) { + if (options && has_help_option(options)) { + g_free(options); return print_block_option_help(filename, fmt); } @@ -420,10 +431,15 @@ static int img_create(int argc, char **argv) if (local_err) { error_report("%s: %s", filename, error_get_pretty(local_err)); error_free(local_err); - return 1; + goto fail; } + g_free(options); return 0; + +fail: + g_free(options); + return 1; } static void dump_json_image_check(ImageCheck *check, bool quiet) From 2dc8328b4c6aba60f4ad543186f4e8aec2e9287e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:05 +0100 Subject: [PATCH 33/54] qemu-img convert: Support multiple -o options Instead of ignoring all option values but the last one, multiple -o options now have the same meaning as having a single option with all settings in the order of their respective -o options. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- qemu-img.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 9c1643d0c1..3fd2168679 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1164,6 +1164,9 @@ static int img_convert(int argc, char **argv) Error *local_err = NULL; QemuOpts *sn_opts = NULL; + /* Initialize before goto out */ + qemu_progress_init(progress, 1.0); + fmt = NULL; out_fmt = "raw"; cache = "unsafe"; @@ -1195,13 +1198,26 @@ static int img_convert(int argc, char **argv) case 'e': error_report("option -e is deprecated, please use \'-o " "encryption\' instead!"); - return 1; + ret = -1; + goto out; case '6': error_report("option -6 is deprecated, please use \'-o " "compat6\' instead!"); - return 1; + ret = -1; + goto out; case 'o': - options = optarg; + if (!is_valid_option_list(optarg)) { + error_report("Invalid option list: %s", optarg); + ret = -1; + goto out; + } + if (!options) { + options = g_strdup(optarg); + } else { + char *old_options = options; + options = g_strdup_printf("%s,%s", options, optarg); + g_free(old_options); + } break; case 's': snapshot_name = optarg; @@ -1212,7 +1228,8 @@ static int img_convert(int argc, char **argv) if (!sn_opts) { error_report("Failed in parsing snapshot param '%s'", optarg); - return 1; + ret = -1; + goto out; } } else { snapshot_name = optarg; @@ -1225,7 +1242,8 @@ static int img_convert(int argc, char **argv) sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B); if (sval < 0 || *end) { error_report("Invalid minimum zero buffer size for sparse output specified"); - return 1; + ret = -1; + goto out; } min_sparse = sval / BDRV_SECTOR_SIZE; @@ -1257,10 +1275,7 @@ static int img_convert(int argc, char **argv) out_filename = argv[argc - 1]; - /* Initialize before goto out */ - qemu_progress_init(progress, 1.0); - - if (options && is_help_option(options)) { + if (options && has_help_option(options)) { ret = print_block_option_help(out_filename, out_fmt); goto out; } @@ -1653,6 +1668,7 @@ out: free_option_parameters(create_options); free_option_parameters(param); qemu_vfree(buf); + g_free(options); if (sn_opts) { qemu_opts_del(sn_opts); } From 626f84f39d4ae365a44dbbc0d0dd3c7739c3971a Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:06 +0100 Subject: [PATCH 34/54] qemu-img amend: Support multiple -o options Instead of ignoring all option values but the last one, multiple -o options now have the same meaning as having a single option with all settings in the order of their respective -o options. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- qemu-img.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 3fd2168679..6ceaeb244a 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2667,7 +2667,18 @@ static int img_amend(int argc, char **argv) help(); break; case 'o': - options = optarg; + if (!is_valid_option_list(optarg)) { + error_report("Invalid option list: %s", optarg); + ret = -1; + goto out; + } + if (!options) { + options = g_strdup(optarg); + } else { + char *old_options = options; + options = g_strdup_printf("%s,%s", options, optarg); + g_free(old_options); + } break; case 'f': fmt = optarg; @@ -2697,7 +2708,7 @@ static int img_amend(int argc, char **argv) fmt = bs->drv->format_name; - if (is_help_option(options)) { + if (has_help_option(options)) { ret = print_block_option_help(filename, fmt); goto out; } @@ -2724,6 +2735,8 @@ out: } free_option_parameters(create_options); free_option_parameters(options_param); + g_free(options); + if (ret) { return 1; } From a283cb6e58fca846c658360971d23fdd1129db65 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:07 +0100 Subject: [PATCH 35/54] qemu-img: Allow -o help with incomplete argument list This patch allows using 'qemu-img $subcmd -o help' for the create, convert and amend subcommands, without specifying the previously required filename arguments. Note that it's still allowed and meaningful to specify a filename: An invocation like 'qemu-img create -o help sheepdog:foo' will also display options that are provided by the Sheepdog driver. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- qemu-img.c | 60 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 6ceaeb244a..5512f5bfe4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -250,16 +250,19 @@ static int print_block_option_help(const char *filename, const char *fmt) return 1; } - proto_drv = bdrv_find_protocol(filename, true); - if (!proto_drv) { - error_report("Unknown protocol '%s'", filename); - return 1; - } - create_options = append_option_parameters(create_options, drv->create_options); - create_options = append_option_parameters(create_options, - proto_drv->create_options); + + if (filename) { + proto_drv = bdrv_find_protocol(filename, true); + if (!proto_drv) { + error_report("Unknown protocol '%s'", filename); + return 1; + } + create_options = append_option_parameters(create_options, + proto_drv->create_options); + } + print_option_help(create_options); free_option_parameters(create_options); return 0; @@ -394,10 +397,16 @@ static int img_create(int argc, char **argv) } /* Get the filename */ + filename = (optind < argc) ? argv[optind] : NULL; + if (options && has_help_option(options)) { + g_free(options); + return print_block_option_help(filename, fmt); + } + if (optind >= argc) { help(); } - filename = argv[optind++]; + optind++; /* Get image size, if specified */ if (optind < argc) { @@ -421,11 +430,6 @@ static int img_create(int argc, char **argv) help(); } - if (options && has_help_option(options)) { - g_free(options); - return print_block_option_help(filename, fmt); - } - bdrv_img_create(filename, fmt, base_filename, base_fmt, options, img_size, BDRV_O_FLAGS, &local_err, quiet); if (local_err) { @@ -1269,17 +1273,18 @@ static int img_convert(int argc, char **argv) } bs_n = argc - optind - 1; - if (bs_n < 1) { - help(); - } - - out_filename = argv[argc - 1]; + out_filename = bs_n >= 1 ? argv[argc - 1] : NULL; if (options && has_help_option(options)) { ret = print_block_option_help(out_filename, out_fmt); goto out; } + if (bs_n < 1) { + help(); + } + + if (bs_n > 1 && out_baseimg) { error_report("-B makes no sense when concatenating multiple input " "images"); @@ -2689,15 +2694,21 @@ static int img_amend(int argc, char **argv) } } - if (optind != argc - 1) { - help(); - } - if (!options) { help(); } - filename = argv[argc - 1]; + filename = (optind == argc - 1) ? argv[argc - 1] : NULL; + if (fmt && has_help_option(options)) { + /* If a format is explicitly specified (and possibly no filename is + * given), print option help here */ + ret = print_block_option_help(filename, fmt); + goto out; + } + + if (optind != argc - 1) { + help(); + } bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet); if (!bs) { @@ -2709,6 +2720,7 @@ static int img_amend(int argc, char **argv) fmt = bs->drv->format_name; if (has_help_option(options)) { + /* If the format was auto-detected, print option help here */ ret = print_block_option_help(filename, fmt); goto out; } From a33cc31d08eb46ec2a4f214087c99e4bd4c907e9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 21 Feb 2014 16:24:08 +0100 Subject: [PATCH 36/54] qemu-iotests: Check qemu-img command line parsing Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Eric Blake --- tests/qemu-iotests/082 | 208 +++++++++++++++ tests/qemu-iotests/082.out | 529 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 738 insertions(+) create mode 100755 tests/qemu-iotests/082 create mode 100644 tests/qemu-iotests/082.out diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082 new file mode 100755 index 0000000000..f6eb75f624 --- /dev/null +++ b/tests/qemu-iotests/082 @@ -0,0 +1,208 @@ +#!/bin/bash +# +# Test qemu-img command line parsing +# +# Copyright (C) 2014 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +function run_qemu_img() +{ + echo + echo Testing: "$@" | _filter_testdir + "$QEMU_IMG" "$@" 2>&1 | _filter_testdir +} + +size=128M + +echo +echo === create: Options specified more than once === + +# Last -f should win +run_qemu_img create -f foo -f $IMGFMT "$TEST_IMG" $size +run_qemu_img info "$TEST_IMG" + +# Multiple -o should be merged +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on "$TEST_IMG" $size +run_qemu_img info "$TEST_IMG" + +# If the same -o key is specified more than once, the last one wins +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k "$TEST_IMG" $size +run_qemu_img info "$TEST_IMG" +run_qemu_img create -f $IMGFMT -o cluster_size=4k,cluster_size=8k "$TEST_IMG" $size +run_qemu_img info "$TEST_IMG" + +echo +echo === create: help for -o === + +# Adding the help option to a command without other -o options +run_qemu_img create -f $IMGFMT -o help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o \? "$TEST_IMG" $size + +# Adding the help option to the same -o option +run_qemu_img create -f $IMGFMT -o cluster_size=4k,help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o help,cluster_size=4k "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o \?,cluster_size=4k "$TEST_IMG" $size + +# Adding the help option to a separate -o option +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? "$TEST_IMG" $size + +# Looks like a help option, but is part of the backing file name +run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG",,\? "$TEST_IMG" $size + +# Try to trick qemu-img into creating escaped commas +run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG" $size +run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_IMG" $size + +# Leave out everything that isn't needed +run_qemu_img create -f $IMGFMT -o help +run_qemu_img create -o help + +echo +echo === convert: Options specified more than once === + +# We need a valid source image +run_qemu_img create -f $IMGFMT "$TEST_IMG" $size + +# Last -f should win +run_qemu_img convert -f foo -f $IMGFMT "$TEST_IMG" "$TEST_IMG".base +run_qemu_img info "$TEST_IMG".base + +# Last -O should win +run_qemu_img convert -O foo -O $IMGFMT "$TEST_IMG" "$TEST_IMG".base +run_qemu_img info "$TEST_IMG".base + +# Multiple -o should be merged +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on "$TEST_IMG" "$TEST_IMG".base +run_qemu_img info "$TEST_IMG".base + +# If the same -o key is specified more than once, the last one wins +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k "$TEST_IMG" "$TEST_IMG".base +run_qemu_img info "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,cluster_size=8k "$TEST_IMG" "$TEST_IMG".base +run_qemu_img info "$TEST_IMG".base + +echo +echo === convert: help for -o === + +# Adding the help option to a command without other -o options +run_qemu_img convert -O $IMGFMT -o help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o \? "$TEST_IMG" "$TEST_IMG".base + +# Adding the help option to the same -o option +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,\? "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o help,cluster_size=4k "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o \?,cluster_size=4k "$TEST_IMG" "$TEST_IMG".base + +# Adding the help option to a separate -o option +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o \? "$TEST_IMG" "$TEST_IMG".base + +# Looks like a help option, but is part of the backing file name +run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG",,help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG",,\? "$TEST_IMG" "$TEST_IMG".base + +# Try to trick qemu-img into creating escaped commas +run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG" "$TEST_IMG".base +run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_IMG" "$TEST_IMG".base + +# Leave out everything that isn't needed +run_qemu_img convert -O $IMGFMT -o help +run_qemu_img convert -o help + +echo +echo === amend: Options specified more than once === + +# Last -f should win +run_qemu_img amend -f foo -f $IMGFMT -o lazy_refcounts=on "$TEST_IMG" +run_qemu_img info "$TEST_IMG" + +# Multiple -o should be merged +run_qemu_img amend -f $IMGFMT -o size=130M -o lazy_refcounts=off "$TEST_IMG" +run_qemu_img info "$TEST_IMG" + +# If the same -o key is specified more than once, the last one wins +run_qemu_img amend -f $IMGFMT -o size=8M -o lazy_refcounts=on -o size=132M "$TEST_IMG" +run_qemu_img info "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o size=4M,size=148M "$TEST_IMG" +run_qemu_img info "$TEST_IMG" + +echo +echo === amend: help for -o === + +# Adding the help option to a command without other -o options +run_qemu_img amend -f $IMGFMT -o help "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o \? "$TEST_IMG" + +# Adding the help option to the same -o option +run_qemu_img amend -f $IMGFMT -o cluster_size=4k,help "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o cluster_size=4k,\? "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o help,cluster_size=4k "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o \?,cluster_size=4k "$TEST_IMG" + +# Adding the help option to a separate -o option +run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o help "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o cluster_size=4k -o \? "$TEST_IMG" + +# Looks like a help option, but is part of the backing file name +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG",,help "$TEST_IMG" +run_qemu_img rebase -u -b "" -f $IMGFMT "$TEST_IMG" + +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG",,\? "$TEST_IMG" +run_qemu_img rebase -u -b "" -f $IMGFMT "$TEST_IMG" + +# Try to trick qemu-img into creating escaped commas +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG", -o help "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,help "$TEST_IMG" +run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_IMG" + +# Leave out everything that isn't needed +run_qemu_img amend -f $IMGFMT -o help +run_qemu_img convert -o help + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out new file mode 100644 index 0000000000..28309a0327 --- /dev/null +++ b/tests/qemu-iotests/082.out @@ -0,0 +1,529 @@ +QA output created by 082 + +=== create: Options specified more than once === + +Testing: create -f foo -f qcow2 TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: false + +Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=4096 lazy_refcounts=on + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 16K +cluster_size: 4096 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: create -f qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=on + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 28K +cluster_size: 8192 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: create -f qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=8192 lazy_refcounts=off + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 28K +cluster_size: 8192 +Format specific information: + compat: 1.1 + lazy refcounts: false + +=== create: help for -o === + +Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,help' encryption=off cluster_size=65536 lazy_refcounts=off + +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2,?' encryption=off cluster_size=65536 lazy_refcounts=off + +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 128M +qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2, + +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2 128M +qemu-img: Invalid option list: ,help + +Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2 128M +qemu-img: Invalid option list: ,, + +Testing: create -f qcow2 -o help +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: create -o help +Supported options: +size Virtual disk size + +=== convert: Options specified more than once === + +Testing: create -f qcow2 TEST_DIR/t.qcow2 128M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 encryption=off cluster_size=65536 lazy_refcounts=off + +Testing: convert -f foo -f qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base + +Testing: info TEST_DIR/t.qcow2.base +image: TEST_DIR/t.qcow2.base +file format: raw +virtual size: 128M (134217728 bytes) +disk size: 0 + +Testing: convert -O foo -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base + +Testing: info TEST_DIR/t.qcow2.base +image: TEST_DIR/t.qcow2.base +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: false + +Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base + +Testing: info TEST_DIR/t.qcow2.base +image: TEST_DIR/t.qcow2.base +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 16K +cluster_size: 4096 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: convert -O qcow2 -o cluster_size=4k -o lazy_refcounts=on -o cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base + +Testing: info TEST_DIR/t.qcow2.base +image: TEST_DIR/t.qcow2.base +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 28K +cluster_size: 8192 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: convert -O qcow2 -o cluster_size=4k,cluster_size=8k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base + +Testing: info TEST_DIR/t.qcow2.base +image: TEST_DIR/t.qcow2.base +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 28K +cluster_size: 8192 +Format specific information: + compat: 1.1 + lazy refcounts: false + +=== convert: help for -o === + +Testing: convert -O qcow2 -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help': No such file or directory + +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,?': No such file or directory + +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2, + +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +qemu-img: Invalid option list: ,help + +Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base +qemu-img: Invalid option list: ,, + +Testing: convert -O qcow2 -o help +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -o help +Supported options: +size Virtual disk size + +=== amend: Options specified more than once === + +Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2 + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 128M (134217728 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: amend -f qcow2 -o size=130M -o lazy_refcounts=off TEST_DIR/t.qcow2 + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 130M (136314880 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: false + +Testing: amend -f qcow2 -o size=8M -o lazy_refcounts=on -o size=132M TEST_DIR/t.qcow2 + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 132M (138412032 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: true + +Testing: amend -f qcow2 -o size=4M,size=148M TEST_DIR/t.qcow2 + +Testing: info TEST_DIR/t.qcow2 +image: TEST_DIR/t.qcow2 +file format: qcow2 +virtual size: 148M (155189248 bytes) +disk size: 196K +cluster_size: 65536 +Format specific information: + compat: 1.1 + lazy refcounts: true + +=== amend: help for -o === + +Testing: amend -f qcow2 -o help TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 + +Testing: rebase -u -b -f qcow2 TEST_DIR/t.qcow2 + +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,? TEST_DIR/t.qcow2 + +Testing: rebase -u -b -f qcow2 TEST_DIR/t.qcow2 + +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help TEST_DIR/t.qcow2 +qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2, + +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help TEST_DIR/t.qcow2 +qemu-img: Invalid option list: ,help + +Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/t.qcow2 +qemu-img: Invalid option list: ,, + +Testing: amend -f qcow2 -o help +Supported options: +size Virtual disk size +compat Compatibility level (0.10 or 1.1) +backing_file File name of a base image +backing_fmt Image format of the base image +encryption Encrypt the image +cluster_size qcow2 cluster size +preallocation Preallocation mode (allowed values: off, metadata) +lazy_refcounts Postpone refcount updates + +Testing: convert -o help +Supported options: +size Virtual disk size +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index d8be74a17e..80b181f8b1 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -83,3 +83,4 @@ 074 rw auto 077 rw auto 079 rw auto +082 rw auto quick From ae39c4b2015dd5ee35021d0f4212bb1304106524 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 19:11:39 +0100 Subject: [PATCH 37/54] qemu-config: Sections must consist of keys In config_parse_qdict_section(), the QList returned by qdict_array_split() is assumed to only contain QDicts. Currently, this is true but it may (and will) change in the future. Therefore, check whether the assumption actually holds. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- util/qemu-config.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/util/qemu-config.c b/util/qemu-config.c index 797df71569..f6101012c0 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -413,6 +413,12 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts, QDict *section = qobject_to_qdict(qlist_entry_obj(list_entry)); char *opt_name; + if (!section) { + error_setg(errp, "[%s] section (index %u) does not consist of " + "keys", opts->name, i); + goto out; + } + opt_name = g_strdup_printf("%s.%u", opts->name, i++); subopts = qemu_opts_create(opts, opt_name, 1, &local_err); g_free(opt_name); From bae3f92a016b8eddc0d5806c24baea3ecedac0a0 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 19:11:40 +0100 Subject: [PATCH 38/54] qdict: Extract non-QDicts in qdict_array_split() Currently, qdict_array_split() only splits off entries with a key prefix of "%u.", packing them into a new QDict. This patch makes it support entries with the plain key "%u" as well, directly putting them into the new QList without creating a QDict. If there is both an entry with a key of "%u" and other entries with keys prefixed "%u." (for the same index), the function simply terminates. To do this, this patch also adds a static function which tests whether a given QDict contains any keys with the given prefix. This is used to test whether entries with a key prefixed "%u." do exist in the source QDict without modifying it. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- qobject/qdict.c | 60 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/qobject/qdict.c b/qobject/qdict.c index a3924f24bd..42ec4c0d2c 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -597,18 +597,33 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start) } } +static bool qdict_has_prefixed_entries(const QDict *src, const char *start) +{ + const QDictEntry *entry; + + for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) { + if (strstart(entry->key, start, NULL)) { + return true; + } + } + + return false; +} + /** * qdict_array_split(): This function moves array-like elements of a QDict into - * a new QList of QDicts. Every entry in the original QDict with a key prefixed - * "%u.", where %u designates an unsigned integer starting at 0 and + * a new QList. Every entry in the original QDict with a key "%u" or one + * prefixed "%u.", where %u designates an unsigned integer starting at 0 and * incrementally counting up, will be moved to a new QDict at index %u in the - * output QList with the key prefix removed. The function terminates when there - * is no entry in the QDict with a prefix directly (incrementally) following the - * last one. - * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "3.y": 1, "o.o": 7} - * (or {"1.x": 0, "3.y": 1, "0.a": 42, "o.o": 7, "0.b": 23}) - * => [{"a": 42, "b": 23}, {"x": 0}] - * and {"3.y": 1, "o.o": 7} (remainder of the old QDict) + * output QList with the key prefix removed, if that prefix is "%u.". If the + * whole key is just "%u", the whole QObject will be moved unchanged without + * creating a new QDict. The function terminates when there is no entry in the + * QDict with a prefix directly (incrementally) following the last one; it also + * returns if there are both entries with "%u" and "%u." for the same index %u. + * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66} + * (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66}) + * => [{"a": 42, "b": 23}, {"x": 0}, 66] + * and {"4.y": 1, "o.o": 7} (remainder of the old QDict) */ void qdict_array_split(QDict *src, QList **dst) { @@ -617,19 +632,36 @@ void qdict_array_split(QDict *src, QList **dst) *dst = qlist_new(); for (i = 0; i < UINT_MAX; i++) { + QObject *subqobj; + bool is_subqdict; QDict *subqdict; - char prefix[32]; + char indexstr[32], prefix[32]; size_t snprintf_ret; + snprintf_ret = snprintf(indexstr, 32, "%u", i); + assert(snprintf_ret < 32); + + subqobj = qdict_get(src, indexstr); + snprintf_ret = snprintf(prefix, 32, "%u.", i); assert(snprintf_ret < 32); - qdict_extract_subqdict(src, &subqdict, prefix); - if (!qdict_size(subqdict)) { - QDECREF(subqdict); + is_subqdict = qdict_has_prefixed_entries(src, prefix); + + // There may be either a single subordinate object (named "%u") or + // multiple objects (each with a key prefixed "%u."), but not both. + if (!subqobj == !is_subqdict) { break; } - qlist_append_obj(*dst, QOBJECT(subqdict)); + if (is_subqdict) { + qdict_extract_subqdict(src, &subqdict, prefix); + assert(qdict_size(subqdict) > 0); + } else { + qobject_incref(subqobj); + qdict_del(src, indexstr); + } + + qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict)); } } From 7841c768846dcfa5a162ff46a8e98429aa0d2238 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 19:11:41 +0100 Subject: [PATCH 39/54] check-qdict: Adjust test for qdict_array_split() Test the new functionality of qdict_array_split(), that is, splitting off single objects. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/check-qdict.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/check-qdict.c b/tests/check-qdict.c index 7a7461b0b2..680e3c39ce 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -306,6 +306,7 @@ static void qdict_array_split_test(void) { QDict *test_dict = qdict_new(); QDict *dict1, *dict2; + QInt *int1; QList *test_list; /* @@ -313,10 +314,11 @@ static void qdict_array_split_test(void) * * { * "1.x": 0, - * "3.y": 1, + * "4.y": 1, * "0.a": 42, * "o.o": 7, - * "0.b": 23 + * "0.b": 23, + * "2": 66 * } * * to @@ -328,13 +330,14 @@ static void qdict_array_split_test(void) * }, * { * "x": 0 - * } + * }, + * 66 * ] * * and * * { - * "3.y": 1, + * "4.y": 1, * "o.o": 7 * } * @@ -344,18 +347,21 @@ static void qdict_array_split_test(void) */ qdict_put(test_dict, "1.x", qint_from_int(0)); - qdict_put(test_dict, "3.y", qint_from_int(1)); + qdict_put(test_dict, "4.y", qint_from_int(1)); qdict_put(test_dict, "0.a", qint_from_int(42)); qdict_put(test_dict, "o.o", qint_from_int(7)); qdict_put(test_dict, "0.b", qint_from_int(23)); + qdict_put(test_dict, "2", qint_from_int(66)); qdict_array_split(test_dict, &test_list); dict1 = qobject_to_qdict(qlist_pop(test_list)); dict2 = qobject_to_qdict(qlist_pop(test_list)); + int1 = qobject_to_qint(qlist_pop(test_list)); g_assert(dict1); g_assert(dict2); + g_assert(int1); g_assert(qlist_empty(test_list)); QDECREF(test_list); @@ -373,7 +379,11 @@ static void qdict_array_split_test(void) QDECREF(dict2); - g_assert(qdict_get_int(test_dict, "3.y") == 1); + g_assert(qint_get_int(int1) == 66); + + QDECREF(int1); + + g_assert(qdict_get_int(test_dict, "4.y") == 1); g_assert(qdict_get_int(test_dict, "o.o") == 7); g_assert(qdict_size(test_dict) == 2); From 64757582dafca9b0b3846677e368dd40bcd68b43 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 21:05:13 +0100 Subject: [PATCH 40/54] check-qdict: Test termination of qdict_array_split() qdict_array_split() should terminate if it encounters both an entry with a key of "%u" and entries with keys prefixed "%u." for the same index. This patch adds a test for this case. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/check-qdict.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/check-qdict.c b/tests/check-qdict.c index 680e3c39ce..2ad0f7827e 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -389,6 +389,59 @@ static void qdict_array_split_test(void) g_assert(qdict_size(test_dict) == 2); QDECREF(test_dict); + + + /* + * Test the split of + * + * { + * "0": 42, + * "1": 23, + * "1.x": 84 + * } + * + * to + * + * [ + * 42 + * ] + * + * and + * + * { + * "1": 23, + * "1.x": 84 + * } + * + * That is, test whether splitting stops if there is both an entry with key + * of "%u" and other entries with keys prefixed "%u." for the same index. + */ + + test_dict = qdict_new(); + + qdict_put(test_dict, "0", qint_from_int(42)); + qdict_put(test_dict, "1", qint_from_int(23)); + qdict_put(test_dict, "1.x", qint_from_int(84)); + + qdict_array_split(test_dict, &test_list); + + int1 = qobject_to_qint(qlist_pop(test_list)); + + g_assert(int1); + g_assert(qlist_empty(test_list)); + + QDECREF(test_list); + + g_assert(qint_get_int(int1) == 42); + + QDECREF(int1); + + g_assert(qdict_get_int(test_dict, "1") == 23); + g_assert(qdict_get_int(test_dict, "1.x") == 84); + + g_assert(qdict_size(test_dict) == 2); + + QDECREF(test_dict); } /* From 27cec15e4ed4e69155f2499ceb46d22d8425102a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:10 +0100 Subject: [PATCH 41/54] quorum: Create quorum.c, add QuorumChildRequest and QuorumAIOCB. Quorum is a block filter mirroring writes to num_children children. For reads quorum reads each children and does a vote. If more than vote_threshold versions are identical the quorum is reached and this winning version is returned to the guest. So quorum prevents bit corruption. For high availability purpose minority errors are reported via QMP but the guest does not see them. This patch creates the driver C source file and introduces the structures that will be used in asynchronous reads and writes. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/Makefile.objs | 1 + block/quorum.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 block/quorum.c diff --git a/block/Makefile.objs b/block/Makefile.objs index e254a2180a..716556f6ec 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -3,6 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o +block-obj-y += quorum.o block-obj-y += parallels.o blkdebug.o blkverify.o block-obj-y += snapshot.o qapi.o block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o diff --git a/block/quorum.c b/block/quorum.c new file mode 100644 index 0000000000..950f5cc990 --- /dev/null +++ b/block/quorum.c @@ -0,0 +1,53 @@ +/* + * Quorum Block filter + * + * Copyright (C) 2012-2014 Nodalink, EURL. + * + * Author: + * Benoît Canet + * + * Based on the design and code of blkverify.c (Copyright (C) 2010 IBM, Corp) + * and blkmirror.c (Copyright (C) 2011 Red Hat, Inc). + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "block/block_int.h" + +typedef struct QuorumAIOCB QuorumAIOCB; + +/* Quorum will create one instance of the following structure per operation it + * performs on its children. + * So for each read/write operation coming from the upper layer there will be + * $children_count QuorumChildRequest. + */ +typedef struct QuorumChildRequest { + BlockDriverAIOCB *aiocb; + QEMUIOVector qiov; + uint8_t *buf; + int ret; + QuorumAIOCB *parent; +} QuorumChildRequest; + +/* Quorum will use the following structure to track progress of each read/write + * operation received by the upper layer. + * This structure hold pointers to the QuorumChildRequest structures instances + * used to do operations on each children and track overall progress. + */ +struct QuorumAIOCB { + BlockDriverAIOCB common; + + /* Request metadata */ + uint64_t sector_num; + int nb_sectors; + + QEMUIOVector *qiov; /* calling IOV */ + + QuorumChildRequest *qcrs; /* individual child requests */ + int count; /* number of completed AIOCB */ + int success_count; /* number of successfully completed AIOCB */ + + bool is_read; + int vote_ret; +}; From cadebd7a2a590c2ac5ced58c2fc207c7ae78fb1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:11 +0100 Subject: [PATCH 42/54] quorum: Create BDRVQuorumState and BlkDriver and do init. Create the structure holding the quorum settings and write the minimal block driver instanciation boilerplate. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index 950f5cc990..375ffd5963 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -15,6 +15,23 @@ #include "block/block_int.h" +/* the following structure holds the state of one quorum instance */ +typedef struct BDRVQuorumState { + BlockDriverState **bs; /* children BlockDriverStates */ + int num_children; /* children count */ + int threshold; /* if less than threshold children reads gave the + * same result a quorum error occurs. + */ + bool is_blkverify; /* true if the driver is in blkverify mode + * Writes are mirrored on two children devices. + * On reads the two children devices' contents are + * compared and if a difference is spotted its + * location is printed and the code aborts. + * It is useful to debug other block drivers by + * comparing them with a reference one. + */ +} BDRVQuorumState; + typedef struct QuorumAIOCB QuorumAIOCB; /* Quorum will create one instance of the following structure per operation it @@ -51,3 +68,17 @@ struct QuorumAIOCB { bool is_read; int vote_ret; }; + +static BlockDriver bdrv_quorum = { + .format_name = "quorum", + .protocol_name = "quorum", + + .instance_size = sizeof(BDRVQuorumState), +}; + +static void bdrv_quorum_init(void) +{ + bdrv_register(&bdrv_quorum); +} + +block_init(bdrv_quorum_init); From 13e7956e3190b51f02e75374bb9dfdcacfd08829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:12 +0100 Subject: [PATCH 43/54] quorum: Add quorum_aio_writev and its dependencies. Writes are mirrored num_children times on num_children devices. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index 375ffd5963..99e0e03cdf 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -69,11 +69,114 @@ struct QuorumAIOCB { int vote_ret; }; +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb) +{ + QuorumAIOCB *acb = container_of(blockacb, QuorumAIOCB, common); + BDRVQuorumState *s = acb->common.bs->opaque; + int i; + + /* cancel all callbacks */ + for (i = 0; i < s->num_children; i++) { + bdrv_aio_cancel(acb->qcrs[i].aiocb); + } + + g_free(acb->qcrs); + qemu_aio_release(acb); +} + +static AIOCBInfo quorum_aiocb_info = { + .aiocb_size = sizeof(QuorumAIOCB), + .cancel = quorum_aio_cancel, +}; + +static void quorum_aio_finalize(QuorumAIOCB *acb) +{ + int ret = 0; + + acb->common.cb(acb->common.opaque, ret); + + g_free(acb->qcrs); + qemu_aio_release(acb); +} + +static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, + BlockDriverState *bs, + QEMUIOVector *qiov, + uint64_t sector_num, + int nb_sectors, + BlockDriverCompletionFunc *cb, + void *opaque) +{ + QuorumAIOCB *acb = qemu_aio_get(&quorum_aiocb_info, bs, cb, opaque); + int i; + + acb->common.bs->opaque = s; + acb->sector_num = sector_num; + acb->nb_sectors = nb_sectors; + acb->qiov = qiov; + acb->qcrs = g_new0(QuorumChildRequest, s->num_children); + acb->count = 0; + acb->success_count = 0; + acb->is_read = false; + acb->vote_ret = 0; + + for (i = 0; i < s->num_children; i++) { + acb->qcrs[i].buf = NULL; + acb->qcrs[i].ret = 0; + acb->qcrs[i].parent = acb; + } + + return acb; +} + +static void quorum_aio_cb(void *opaque, int ret) +{ + QuorumChildRequest *sacb = opaque; + QuorumAIOCB *acb = sacb->parent; + BDRVQuorumState *s = acb->common.bs->opaque; + + sacb->ret = ret; + acb->count++; + if (ret == 0) { + acb->success_count++; + } + assert(acb->count <= s->num_children); + assert(acb->success_count <= s->num_children); + if (acb->count < s->num_children) { + return; + } + + quorum_aio_finalize(acb); +} + +static BlockDriverAIOCB *quorum_aio_writev(BlockDriverState *bs, + int64_t sector_num, + QEMUIOVector *qiov, + int nb_sectors, + BlockDriverCompletionFunc *cb, + void *opaque) +{ + BDRVQuorumState *s = bs->opaque; + QuorumAIOCB *acb = quorum_aio_get(s, bs, qiov, sector_num, nb_sectors, + cb, opaque); + int i; + + for (i = 0; i < s->num_children; i++) { + acb->qcrs[i].aiocb = bdrv_aio_writev(s->bs[i], sector_num, qiov, + nb_sectors, &quorum_aio_cb, + &acb->qcrs[i]); + } + + return &acb->common; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + + .bdrv_aio_writev = quorum_aio_writev, }; static void bdrv_quorum_init(void) From f70d7f7e4d05b7a7797815afdcb83f4375740838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:13 +0100 Subject: [PATCH 44/54] blkverify: Extract qemu_iovec_clone() and qemu_iovec_compare() from blkverify. qemu_iovec_compare() will be used to compare IOs vectors in quorum blkverify mode. The patch extracts these functions in order to factorize the code. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/blkverify.c | 108 +----------------------------------------- include/qemu-common.h | 2 + util/iov.c | 106 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 106 deletions(-) diff --git a/block/blkverify.c b/block/blkverify.c index 0e28502e8e..b98b08bedf 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -173,110 +173,6 @@ static int64_t blkverify_getlength(BlockDriverState *bs) return bdrv_getlength(s->test_file); } -/** - * Check that I/O vector contents are identical - * - * @a: I/O vector - * @b: I/O vector - * @ret: Offset to first mismatching byte or -1 if match - */ -static ssize_t blkverify_iovec_compare(QEMUIOVector *a, QEMUIOVector *b) -{ - int i; - ssize_t offset = 0; - - assert(a->niov == b->niov); - for (i = 0; i < a->niov; i++) { - size_t len = 0; - uint8_t *p = (uint8_t *)a->iov[i].iov_base; - uint8_t *q = (uint8_t *)b->iov[i].iov_base; - - assert(a->iov[i].iov_len == b->iov[i].iov_len); - while (len < a->iov[i].iov_len && *p++ == *q++) { - len++; - } - - offset += len; - - if (len != a->iov[i].iov_len) { - return offset; - } - } - return -1; -} - -typedef struct { - int src_index; - struct iovec *src_iov; - void *dest_base; -} IOVectorSortElem; - -static int sortelem_cmp_src_base(const void *a, const void *b) -{ - const IOVectorSortElem *elem_a = a; - const IOVectorSortElem *elem_b = b; - - /* Don't overflow */ - if (elem_a->src_iov->iov_base < elem_b->src_iov->iov_base) { - return -1; - } else if (elem_a->src_iov->iov_base > elem_b->src_iov->iov_base) { - return 1; - } else { - return 0; - } -} - -static int sortelem_cmp_src_index(const void *a, const void *b) -{ - const IOVectorSortElem *elem_a = a; - const IOVectorSortElem *elem_b = b; - - return elem_a->src_index - elem_b->src_index; -} - -/** - * Copy contents of I/O vector - * - * The relative relationships of overlapping iovecs are preserved. This is - * necessary to ensure identical semantics in the cloned I/O vector. - */ -static void blkverify_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, - void *buf) -{ - IOVectorSortElem sortelems[src->niov]; - void *last_end; - int i; - - /* Sort by source iovecs by base address */ - for (i = 0; i < src->niov; i++) { - sortelems[i].src_index = i; - sortelems[i].src_iov = &src->iov[i]; - } - qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_base); - - /* Allocate buffer space taking into account overlapping iovecs */ - last_end = NULL; - for (i = 0; i < src->niov; i++) { - struct iovec *cur = sortelems[i].src_iov; - ptrdiff_t rewind = 0; - - /* Detect overlap */ - if (last_end && last_end > cur->iov_base) { - rewind = last_end - cur->iov_base; - } - - sortelems[i].dest_base = buf - rewind; - buf += cur->iov_len - MIN(rewind, cur->iov_len); - last_end = MAX(cur->iov_base + cur->iov_len, last_end); - } - - /* Sort by source iovec index and build destination iovec */ - qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_index); - for (i = 0; i < src->niov; i++) { - qemu_iovec_add(dest, sortelems[i].dest_base, src->iov[i].iov_len); - } -} - static BlkverifyAIOCB *blkverify_aio_get(BlockDriverState *bs, bool is_write, int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, @@ -340,7 +236,7 @@ static void blkverify_aio_cb(void *opaque, int ret) static void blkverify_verify_readv(BlkverifyAIOCB *acb) { - ssize_t offset = blkverify_iovec_compare(acb->qiov, &acb->raw_qiov); + ssize_t offset = qemu_iovec_compare(acb->qiov, &acb->raw_qiov); if (offset != -1) { blkverify_err(acb, "contents mismatch in sector %" PRId64, acb->sector_num + (int64_t)(offset / BDRV_SECTOR_SIZE)); @@ -358,7 +254,7 @@ static BlockDriverAIOCB *blkverify_aio_readv(BlockDriverState *bs, acb->verify = blkverify_verify_readv; acb->buf = qemu_blockalign(bs->file, qiov->size); qemu_iovec_init(&acb->raw_qiov, acb->qiov->niov); - blkverify_iovec_clone(&acb->raw_qiov, qiov, acb->buf); + qemu_iovec_clone(&acb->raw_qiov, qiov, acb->buf); bdrv_aio_readv(s->test_file, sector_num, qiov, nb_sectors, blkverify_aio_cb, acb); diff --git a/include/qemu-common.h b/include/qemu-common.h index b0e34b2e15..20ace93098 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -338,6 +338,8 @@ size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset, const void *buf, size_t bytes); size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, int fillc, size_t bytes); +ssize_t qemu_iovec_compare(QEMUIOVector *a, QEMUIOVector *b); +void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf); bool buffer_is_zero(const void *buf, size_t len); diff --git a/util/iov.c b/util/iov.c index bb46c04e4d..03934da74d 100644 --- a/util/iov.c +++ b/util/iov.c @@ -378,6 +378,112 @@ size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, return iov_memset(qiov->iov, qiov->niov, offset, fillc, bytes); } +/** + * Check that I/O vector contents are identical + * + * The IO vectors must have the same structure (same length of all parts). + * A typical usage is to compare vectors created with qemu_iovec_clone(). + * + * @a: I/O vector + * @b: I/O vector + * @ret: Offset to first mismatching byte or -1 if match + */ +ssize_t qemu_iovec_compare(QEMUIOVector *a, QEMUIOVector *b) +{ + int i; + ssize_t offset = 0; + + assert(a->niov == b->niov); + for (i = 0; i < a->niov; i++) { + size_t len = 0; + uint8_t *p = (uint8_t *)a->iov[i].iov_base; + uint8_t *q = (uint8_t *)b->iov[i].iov_base; + + assert(a->iov[i].iov_len == b->iov[i].iov_len); + while (len < a->iov[i].iov_len && *p++ == *q++) { + len++; + } + + offset += len; + + if (len != a->iov[i].iov_len) { + return offset; + } + } + return -1; +} + +typedef struct { + int src_index; + struct iovec *src_iov; + void *dest_base; +} IOVectorSortElem; + +static int sortelem_cmp_src_base(const void *a, const void *b) +{ + const IOVectorSortElem *elem_a = a; + const IOVectorSortElem *elem_b = b; + + /* Don't overflow */ + if (elem_a->src_iov->iov_base < elem_b->src_iov->iov_base) { + return -1; + } else if (elem_a->src_iov->iov_base > elem_b->src_iov->iov_base) { + return 1; + } else { + return 0; + } +} + +static int sortelem_cmp_src_index(const void *a, const void *b) +{ + const IOVectorSortElem *elem_a = a; + const IOVectorSortElem *elem_b = b; + + return elem_a->src_index - elem_b->src_index; +} + +/** + * Copy contents of I/O vector + * + * The relative relationships of overlapping iovecs are preserved. This is + * necessary to ensure identical semantics in the cloned I/O vector. + */ +void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf) +{ + IOVectorSortElem sortelems[src->niov]; + void *last_end; + int i; + + /* Sort by source iovecs by base address */ + for (i = 0; i < src->niov; i++) { + sortelems[i].src_index = i; + sortelems[i].src_iov = &src->iov[i]; + } + qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_base); + + /* Allocate buffer space taking into account overlapping iovecs */ + last_end = NULL; + for (i = 0; i < src->niov; i++) { + struct iovec *cur = sortelems[i].src_iov; + ptrdiff_t rewind = 0; + + /* Detect overlap */ + if (last_end && last_end > cur->iov_base) { + rewind = last_end - cur->iov_base; + } + + sortelems[i].dest_base = buf - rewind; + buf += cur->iov_len - MIN(rewind, cur->iov_len); + last_end = MAX(cur->iov_base + cur->iov_len, last_end); + } + + /* Sort by source iovec index and build destination iovec */ + qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_index); + for (i = 0; i < src->niov; i++) { + qemu_iovec_add(dest, sortelems[i].dest_base, src->iov[i].iov_len); + } +} + size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt, size_t bytes) { From 7db6982a19f61e3668397b5e31ebfb16a477c414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:14 +0100 Subject: [PATCH 45/54] quorum: Add quorum_aio_readv. Add code to do num_children reads in parallel and cleanup the structures afterwards. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/block/quorum.c b/block/quorum.c index 99e0e03cdf..fd19662bcb 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -91,10 +91,18 @@ static AIOCBInfo quorum_aiocb_info = { static void quorum_aio_finalize(QuorumAIOCB *acb) { - int ret = 0; + BDRVQuorumState *s = acb->common.bs->opaque; + int i, ret = 0; acb->common.cb(acb->common.opaque, ret); + if (acb->is_read) { + for (i = 0; i < s->num_children; i++) { + qemu_vfree(acb->qcrs[i].buf); + qemu_iovec_destroy(&acb->qcrs[i].qiov); + } + } + g_free(acb->qcrs); qemu_aio_release(acb); } @@ -149,6 +157,34 @@ static void quorum_aio_cb(void *opaque, int ret) quorum_aio_finalize(acb); } +static BlockDriverAIOCB *quorum_aio_readv(BlockDriverState *bs, + int64_t sector_num, + QEMUIOVector *qiov, + int nb_sectors, + BlockDriverCompletionFunc *cb, + void *opaque) +{ + BDRVQuorumState *s = bs->opaque; + QuorumAIOCB *acb = quorum_aio_get(s, bs, qiov, sector_num, + nb_sectors, cb, opaque); + int i; + + acb->is_read = true; + + for (i = 0; i < s->num_children; i++) { + acb->qcrs[i].buf = qemu_blockalign(s->bs[i], qiov->size); + qemu_iovec_init(&acb->qcrs[i].qiov, qiov->niov); + qemu_iovec_clone(&acb->qcrs[i].qiov, qiov, acb->qcrs[i].buf); + } + + for (i = 0; i < s->num_children; i++) { + bdrv_aio_readv(s->bs[i], sector_num, qiov, nb_sectors, + quorum_aio_cb, &acb->qcrs[i]); + } + + return &acb->common; +} + static BlockDriverAIOCB *quorum_aio_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov, @@ -176,6 +212,7 @@ static BlockDriver bdrv_quorum = { .instance_size = sizeof(BDRVQuorumState), + .bdrv_aio_readv = quorum_aio_readv, .bdrv_aio_writev = quorum_aio_writev, }; From 95c6bff3561eedaf7c7de287bc4a002720605a8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:15 +0100 Subject: [PATCH 46/54] quorum: Add quorum mechanism. This patchset enables the core of the quorum mechanism. The num_children reads are compared to get the majority version and if this version exists more than threshold times the guest won't see the error at all. If a block is corrupted or if an error occurs during an IO or if the quorum cannot be established QMP events are used to report to the management. Use gnutls's SHA-256 to compare versions. --enable-quorum must be used to enable the feature. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/Makefile.objs | 2 +- block/quorum.c | 391 +++++++++++++++++++++++++++++++++++++- configure | 36 ++++ docs/qmp/qmp-events.txt | 36 ++++ include/monitor/monitor.h | 2 + monitor.c | 2 + 6 files changed, 467 insertions(+), 2 deletions(-) diff --git a/block/Makefile.objs b/block/Makefile.objs index 716556f6ec..425d7fbb59 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o -block-obj-y += quorum.o +block-obj-$(CONFIG_QUORUM) += quorum.o block-obj-y += parallels.o blkdebug.o blkverify.o block-obj-y += snapshot.o qapi.o block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o diff --git a/block/quorum.c b/block/quorum.c index fd19662bcb..4beee96ffa 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -13,7 +13,43 @@ * See the COPYING file in the top-level directory. */ +#include +#include #include "block/block_int.h" +#include "qapi/qmp/qjson.h" + +#define HASH_LENGTH 32 + +/* This union holds a vote hash value */ +typedef union QuorumVoteValue { + char h[HASH_LENGTH]; /* SHA-256 hash */ + int64_t l; /* simpler 64 bits hash */ +} QuorumVoteValue; + +/* A vote item */ +typedef struct QuorumVoteItem { + int index; + QLIST_ENTRY(QuorumVoteItem) next; +} QuorumVoteItem; + +/* this structure is a vote version. A version is the set of votes sharing the + * same vote value. + * The set of votes will be tracked with the items field and its cardinality is + * vote_count. + */ +typedef struct QuorumVoteVersion { + QuorumVoteValue value; + int index; + int vote_count; + QLIST_HEAD(, QuorumVoteItem) items; + QLIST_ENTRY(QuorumVoteVersion) next; +} QuorumVoteVersion; + +/* this structure holds a group of vote versions together */ +typedef struct QuorumVotes { + QLIST_HEAD(, QuorumVoteVersion) vote_list; + bool (*compare)(QuorumVoteValue *a, QuorumVoteValue *b); +} QuorumVotes; /* the following structure holds the state of one quorum instance */ typedef struct BDRVQuorumState { @@ -65,10 +101,14 @@ struct QuorumAIOCB { int count; /* number of completed AIOCB */ int success_count; /* number of successfully completed AIOCB */ + QuorumVotes votes; + bool is_read; int vote_ret; }; +static void quorum_vote(QuorumAIOCB *acb); + static void quorum_aio_cancel(BlockDriverAIOCB *blockacb) { QuorumAIOCB *acb = container_of(blockacb, QuorumAIOCB, common); @@ -94,6 +134,10 @@ static void quorum_aio_finalize(QuorumAIOCB *acb) BDRVQuorumState *s = acb->common.bs->opaque; int i, ret = 0; + if (acb->vote_ret) { + ret = acb->vote_ret; + } + acb->common.cb(acb->common.opaque, ret); if (acb->is_read) { @@ -107,6 +151,16 @@ static void quorum_aio_finalize(QuorumAIOCB *acb) qemu_aio_release(acb); } +static bool quorum_sha256_compare(QuorumVoteValue *a, QuorumVoteValue *b) +{ + return !memcmp(a->h, b->h, HASH_LENGTH); +} + +static bool quorum_64bits_compare(QuorumVoteValue *a, QuorumVoteValue *b) +{ + return a->l == b->l; +} + static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, BlockDriverState *bs, QEMUIOVector *qiov, @@ -125,6 +179,8 @@ static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, acb->qcrs = g_new0(QuorumChildRequest, s->num_children); acb->count = 0; acb->success_count = 0; + acb->votes.compare = quorum_sha256_compare; + QLIST_INIT(&acb->votes.vote_list); acb->is_read = false; acb->vote_ret = 0; @@ -137,6 +193,48 @@ static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, return acb; } +static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret) +{ + QObject *data; + assert(node_name); + data = qobject_from_jsonf("{ 'ret': %d" + ", 'node-name': %s" + ", 'sector-num': %" PRId64 + ", 'sectors-count': %d }", + ret, node_name, acb->sector_num, acb->nb_sectors); + monitor_protocol_event(QEVENT_QUORUM_REPORT_BAD, data); + qobject_decref(data); +} + +static void quorum_report_failure(QuorumAIOCB *acb) +{ + QObject *data; + const char *reference = acb->common.bs->device_name[0] ? + acb->common.bs->device_name : + acb->common.bs->node_name; + data = qobject_from_jsonf("{ 'reference': %s" + ", 'sector-num': %" PRId64 + ", 'sectors-count': %d }", + reference, acb->sector_num, acb->nb_sectors); + monitor_protocol_event(QEVENT_QUORUM_FAILURE, data); + qobject_decref(data); +} + +static int quorum_vote_error(QuorumAIOCB *acb); + +static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb) +{ + BDRVQuorumState *s = acb->common.bs->opaque; + + if (acb->success_count < s->threshold) { + acb->vote_ret = quorum_vote_error(acb); + quorum_report_failure(acb); + return true; + } + + return false; +} + static void quorum_aio_cb(void *opaque, int ret) { QuorumChildRequest *sacb = opaque; @@ -147,6 +245,8 @@ static void quorum_aio_cb(void *opaque, int ret) acb->count++; if (ret == 0) { acb->success_count++; + } else { + quorum_report_bad(acb, sacb->aiocb->bs->node_name, ret); } assert(acb->count <= s->num_children); assert(acb->success_count <= s->num_children); @@ -154,9 +254,298 @@ static void quorum_aio_cb(void *opaque, int ret) return; } + /* Do the vote on read */ + if (acb->is_read) { + quorum_vote(acb); + } else { + quorum_has_too_much_io_failed(acb); + } + quorum_aio_finalize(acb); } +static void quorum_report_bad_versions(BDRVQuorumState *s, + QuorumAIOCB *acb, + QuorumVoteValue *value) +{ + QuorumVoteVersion *version; + QuorumVoteItem *item; + + QLIST_FOREACH(version, &acb->votes.vote_list, next) { + if (acb->votes.compare(&version->value, value)) { + continue; + } + QLIST_FOREACH(item, &version->items, next) { + quorum_report_bad(acb, s->bs[item->index]->node_name, 0); + } + } +} + +static void quorum_copy_qiov(QEMUIOVector *dest, QEMUIOVector *source) +{ + int i; + assert(dest->niov == source->niov); + assert(dest->size == source->size); + for (i = 0; i < source->niov; i++) { + assert(dest->iov[i].iov_len == source->iov[i].iov_len); + memcpy(dest->iov[i].iov_base, + source->iov[i].iov_base, + source->iov[i].iov_len); + } +} + +static void quorum_count_vote(QuorumVotes *votes, + QuorumVoteValue *value, + int index) +{ + QuorumVoteVersion *v = NULL, *version = NULL; + QuorumVoteItem *item; + + /* look if we have something with this hash */ + QLIST_FOREACH(v, &votes->vote_list, next) { + if (votes->compare(&v->value, value)) { + version = v; + break; + } + } + + /* It's a version not yet in the list add it */ + if (!version) { + version = g_new0(QuorumVoteVersion, 1); + QLIST_INIT(&version->items); + memcpy(&version->value, value, sizeof(version->value)); + version->index = index; + version->vote_count = 0; + QLIST_INSERT_HEAD(&votes->vote_list, version, next); + } + + version->vote_count++; + + item = g_new0(QuorumVoteItem, 1); + item->index = index; + QLIST_INSERT_HEAD(&version->items, item, next); +} + +static void quorum_free_vote_list(QuorumVotes *votes) +{ + QuorumVoteVersion *version, *next_version; + QuorumVoteItem *item, *next_item; + + QLIST_FOREACH_SAFE(version, &votes->vote_list, next, next_version) { + QLIST_REMOVE(version, next); + QLIST_FOREACH_SAFE(item, &version->items, next, next_item) { + QLIST_REMOVE(item, next); + g_free(item); + } + g_free(version); + } +} + +static int quorum_compute_hash(QuorumAIOCB *acb, int i, QuorumVoteValue *hash) +{ + int j, ret; + gnutls_hash_hd_t dig; + QEMUIOVector *qiov = &acb->qcrs[i].qiov; + + ret = gnutls_hash_init(&dig, GNUTLS_DIG_SHA256); + + if (ret < 0) { + return ret; + } + + for (j = 0; j < qiov->niov; j++) { + ret = gnutls_hash(dig, qiov->iov[j].iov_base, qiov->iov[j].iov_len); + if (ret < 0) { + break; + } + } + + gnutls_hash_deinit(dig, (void *) hash); + return ret; +} + +static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes) +{ + int max = 0; + QuorumVoteVersion *candidate, *winner = NULL; + + QLIST_FOREACH(candidate, &votes->vote_list, next) { + if (candidate->vote_count > max) { + max = candidate->vote_count; + winner = candidate; + } + } + + return winner; +} + +/* qemu_iovec_compare is handy for blkverify mode because it returns the first + * differing byte location. Yet it is handcoded to compare vectors one byte + * after another so it does not benefit from the libc SIMD optimizations. + * quorum_iovec_compare is written for speed and should be used in the non + * blkverify mode of quorum. + */ +static bool quorum_iovec_compare(QEMUIOVector *a, QEMUIOVector *b) +{ + int i; + int result; + + assert(a->niov == b->niov); + for (i = 0; i < a->niov; i++) { + assert(a->iov[i].iov_len == b->iov[i].iov_len); + result = memcmp(a->iov[i].iov_base, + b->iov[i].iov_base, + a->iov[i].iov_len); + if (result) { + return false; + } + } + + return true; +} + +static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB *acb, + const char *fmt, ...) +{ + va_list ap; + + va_start(ap, fmt); + fprintf(stderr, "quorum: sector_num=%" PRId64 " nb_sectors=%d ", + acb->sector_num, acb->nb_sectors); + vfprintf(stderr, fmt, ap); + fprintf(stderr, "\n"); + va_end(ap); + exit(1); +} + +static bool quorum_compare(QuorumAIOCB *acb, + QEMUIOVector *a, + QEMUIOVector *b) +{ + BDRVQuorumState *s = acb->common.bs->opaque; + ssize_t offset; + + /* This driver will replace blkverify in this particular case */ + if (s->is_blkverify) { + offset = qemu_iovec_compare(a, b); + if (offset != -1) { + quorum_err(acb, "contents mismatch in sector %" PRId64, + acb->sector_num + + (uint64_t)(offset / BDRV_SECTOR_SIZE)); + } + return true; + } + + return quorum_iovec_compare(a, b); +} + +/* Do a vote to get the error code */ +static int quorum_vote_error(QuorumAIOCB *acb) +{ + BDRVQuorumState *s = acb->common.bs->opaque; + QuorumVoteVersion *winner = NULL; + QuorumVotes error_votes; + QuorumVoteValue result_value; + int i, ret = 0; + bool error = false; + + QLIST_INIT(&error_votes.vote_list); + error_votes.compare = quorum_64bits_compare; + + for (i = 0; i < s->num_children; i++) { + ret = acb->qcrs[i].ret; + if (ret) { + error = true; + result_value.l = ret; + quorum_count_vote(&error_votes, &result_value, i); + } + } + + if (error) { + winner = quorum_get_vote_winner(&error_votes); + ret = winner->value.l; + } + + quorum_free_vote_list(&error_votes); + + return ret; +} + +static void quorum_vote(QuorumAIOCB *acb) +{ + bool quorum = true; + int i, j, ret; + QuorumVoteValue hash; + BDRVQuorumState *s = acb->common.bs->opaque; + QuorumVoteVersion *winner; + + if (quorum_has_too_much_io_failed(acb)) { + return; + } + + /* get the index of the first successful read */ + for (i = 0; i < s->num_children; i++) { + if (!acb->qcrs[i].ret) { + break; + } + } + + assert(i < s->num_children); + + /* compare this read with all other successful reads stopping at quorum + * failure + */ + for (j = i + 1; j < s->num_children; j++) { + if (acb->qcrs[j].ret) { + continue; + } + quorum = quorum_compare(acb, &acb->qcrs[i].qiov, &acb->qcrs[j].qiov); + if (!quorum) { + break; + } + } + + /* Every successful read agrees */ + if (quorum) { + quorum_copy_qiov(acb->qiov, &acb->qcrs[i].qiov); + return; + } + + /* compute hashes for each successful read, also store indexes */ + for (i = 0; i < s->num_children; i++) { + if (acb->qcrs[i].ret) { + continue; + } + ret = quorum_compute_hash(acb, i, &hash); + /* if ever the hash computation failed */ + if (ret < 0) { + acb->vote_ret = ret; + goto free_exit; + } + quorum_count_vote(&acb->votes, &hash, i); + } + + /* vote to select the most represented version */ + winner = quorum_get_vote_winner(&acb->votes); + + /* if the winner count is smaller than threshold the read fails */ + if (winner->vote_count < s->threshold) { + quorum_report_failure(acb); + acb->vote_ret = -EIO; + goto free_exit; + } + + /* we have a winner: copy it */ + quorum_copy_qiov(acb->qiov, &acb->qcrs[winner->index].qiov); + + /* some versions are bad print them */ + quorum_report_bad_versions(s, acb, &winner->value); + +free_exit: + /* free lists */ + quorum_free_vote_list(&acb->votes); +} + static BlockDriverAIOCB *quorum_aio_readv(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov, @@ -178,7 +567,7 @@ static BlockDriverAIOCB *quorum_aio_readv(BlockDriverState *bs, } for (i = 0; i < s->num_children; i++) { - bdrv_aio_readv(s->bs[i], sector_num, qiov, nb_sectors, + bdrv_aio_readv(s->bs[i], sector_num, &acb->qcrs[i].qiov, nb_sectors, quorum_aio_cb, &acb->qcrs[i]); } diff --git a/configure b/configure index 39f2a1acd2..6143acbcbf 100755 --- a/configure +++ b/configure @@ -264,6 +264,7 @@ gtkabi="2.0" tpm="no" libssh2="" vhdx="" +quorum="no" # parse CC options first for opt do @@ -1005,6 +1006,10 @@ for opt do ;; --disable-vhdx) vhdx="no" ;; + --disable-quorum) quorum="no" + ;; + --enable-quorum) quorum="yes" + ;; *) echo "ERROR: unknown option $opt"; show_help="yes" ;; esac @@ -1261,6 +1266,8 @@ Advanced options (experts only): --enable-libssh2 enable ssh block device support --disable-vhdx disables support for the Microsoft VHDX image format --enable-vhdx enable support for the Microsoft VHDX image format + --disable-quorum disable quorum block filter support + --enable-quorum enable quorum block filter support NOTE: The object files are built at the place where configure is launched EOF @@ -1936,6 +1943,30 @@ EOF fi fi +########################################## +# Quorum probe (check for gnutls) +if test "$quorum" != "no" ; then +cat > $TMPC < +#include +int main(void) {char data[4096], digest[32]; +gnutls_hash_fast(GNUTLS_DIG_SHA256, data, 4096, digest); +return 0; +} +EOF +quorum_tls_cflags=`$pkg_config --cflags gnutls 2> /dev/null` +quorum_tls_libs=`$pkg_config --libs gnutls 2> /dev/null` +if compile_prog "$quorum_tls_cflags" "$quorum_tls_libs" ; then + qcow_tls=yes + libs_softmmu="$quorum_tls_libs $libs_softmmu" + libs_tools="$quorum_tls_libs $libs_softmmu" + QEMU_CFLAGS="$QEMU_CFLAGS $quorum_tls_cflags" +else + echo "gnutls > 2.10.0 required to compile Quorum" + exit 1 +fi +fi + ########################################## # VNC SASL detection if test "$vnc" = "yes" -a "$vnc_sasl" != "no" ; then @@ -3893,6 +3924,7 @@ echo "libssh2 support $libssh2" echo "TPM passthrough $tpm_passthrough" echo "QOM debugging $qom_cast_debug" echo "vhdx $vhdx" +echo "Quorum $quorum" if test "$sdl_too_old" = "yes"; then echo "-> Your SDL version is too old - please upgrade to have SDL support" @@ -4295,6 +4327,10 @@ if test "$libssh2" = "yes" ; then echo "CONFIG_LIBSSH2=y" >> $config_host_mak fi +if test "$quorum" = "yes" ; then + echo "CONFIG_QUORUM=y" >> $config_host_mak +fi + if test "$virtio_blk_data_plane" = "yes" ; then echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak fi diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt index a378c87583..00f95154dd 100644 --- a/docs/qmp/qmp-events.txt +++ b/docs/qmp/qmp-events.txt @@ -500,3 +500,39 @@ Example: Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is followed respectively by the RESET, SHUTDOWN, or STOP events. + +QUORUM_FAILURE +-------------- + +Emitted by the Quorum block driver if it fails to establish a quorum. + +Data: + +- "reference": device name if defined else node name. +- "sector-num": Number of the first sector of the failed read operation. +- "sector-count": Failed read operation sector count. + +Example: + +{ "event": "QUORUM_FAILURE", + "data": { "reference": "usr1", "sector-num": 345435, "sector-count": 5 }, + "timestamp": { "seconds": 1344522075, "microseconds": 745528 } } + +QUORUM_REPORT_BAD +----------------- + +Emitted to report a corruption of a Quorum file. + +Data: + +- "ret": The IO return code. +- "node-name": The graph node name of the block driver state. +- "sector-num": Number of the first sector of the failed read operation. +- "sector-count": Failed read operation sector count. + +Example: + +{ "event": "QUORUM_REPORT_BAD", + "data": { "ret": 0, "node-name": "1.raw", "sector-num": 345435, + "sector-count": 5 }, + "timestamp": { "seconds": 1344522075, "microseconds": 745528 } } diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 7e5f752b7a..a49ea11eb4 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -49,6 +49,8 @@ typedef enum MonitorEvent { QEVENT_SPICE_MIGRATE_COMPLETED, QEVENT_GUEST_PANICKED, QEVENT_BLOCK_IMAGE_CORRUPTED, + QEVENT_QUORUM_FAILURE, + QEVENT_QUORUM_REPORT_BAD, /* Add to 'monitor_event_names' array in monitor.c when * defining new events here */ diff --git a/monitor.c b/monitor.c index de90fba41e..8ae095f16a 100644 --- a/monitor.c +++ b/monitor.c @@ -508,6 +508,8 @@ static const char *monitor_event_names[] = { [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED", [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED", [QEVENT_BLOCK_IMAGE_CORRUPTED] = "BLOCK_IMAGE_CORRUPTED", + [QEVENT_QUORUM_FAILURE] = "QUORUM_FAILURE", + [QEVENT_QUORUM_REPORT_BAD] = "QUORUM_REPORT_BAD", }; QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) From d55dee2044791a02394a3db7055cedac68dca26b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:16 +0100 Subject: [PATCH 47/54] quorum: Add quorum_getlength(). Check that every bs file returns the same length. Otherwise, return -EIO to disable the quorum and avoid length discrepancy. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index 4beee96ffa..c6ea862132 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -595,12 +595,38 @@ static BlockDriverAIOCB *quorum_aio_writev(BlockDriverState *bs, return &acb->common; } +static int64_t quorum_getlength(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + int64_t result; + int i; + + /* check that all file have the same length */ + result = bdrv_getlength(s->bs[0]); + if (result < 0) { + return result; + } + for (i = 1; i < s->num_children; i++) { + int64_t value = bdrv_getlength(s->bs[i]); + if (value < 0) { + return value; + } + if (value != result) { + return -EIO; + } + } + + return result; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_getlength = quorum_getlength, + .bdrv_aio_readv = quorum_aio_readv, .bdrv_aio_writev = quorum_aio_writev, }; From a28e4c408b28e4d55c5bd327a19290e1da3855dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:17 +0100 Subject: [PATCH 48/54] quorum: Add quorum_invalidate_cache(). We really want that live migration works with quorum so implement quorum_invalidate_cache(). Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index c6ea862132..38bc2176e1 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -619,6 +619,16 @@ static int64_t quorum_getlength(BlockDriverState *bs) return result; } +static void quorum_invalidate_cache(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + int i; + + for (i = 0; i < s->num_children; i++) { + bdrv_invalidate_cache(s->bs[i]); + } +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", @@ -629,6 +639,7 @@ static BlockDriver bdrv_quorum = { .bdrv_aio_readv = quorum_aio_readv, .bdrv_aio_writev = quorum_aio_writev, + .bdrv_invalidate_cache = quorum_invalidate_cache, }; static void bdrv_quorum_init(void) From 1c508d174d4b9dfd066c3729a2560afeef5e081f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:18 +0100 Subject: [PATCH 49/54] quorum: Add quorum_co_flush(). Makes a vote to select error if any. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index 38bc2176e1..840afdaf60 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -629,12 +629,40 @@ static void quorum_invalidate_cache(BlockDriverState *bs) } } +static coroutine_fn int quorum_co_flush(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + QuorumVoteVersion *winner = NULL; + QuorumVotes error_votes; + QuorumVoteValue result_value; + int i; + int result = 0; + + QLIST_INIT(&error_votes.vote_list); + error_votes.compare = quorum_64bits_compare; + + for (i = 0; i < s->num_children; i++) { + result = bdrv_co_flush(s->bs[i]); + result_value.l = result; + quorum_count_vote(&error_votes, &result_value, i); + } + + winner = quorum_get_vote_winner(&error_votes); + result = winner->value.l; + + quorum_free_vote_list(&error_votes); + + return result; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_co_flush_to_disk = quorum_co_flush, + .bdrv_getlength = quorum_getlength, .bdrv_aio_readv = quorum_aio_readv, From 98a7a38f81af2b79a134eaa6cbed543aa3ca5fe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:19 +0100 Subject: [PATCH 50/54] quorum: Implement recursive .bdrv_recurse_is_first_non_filter in quorum. This is used to activate quorum snapshot. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index 840afdaf60..1b2b56f8a0 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -655,6 +655,23 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs) return result; } +static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs, + BlockDriverState *candidate) +{ + BDRVQuorumState *s = bs->opaque; + int i; + + for (i = 0; i < s->num_children; i++) { + bool perm = bdrv_recurse_is_first_non_filter(s->bs[i], + candidate); + if (perm) { + return true; + } + } + + return false; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", @@ -668,6 +685,8 @@ static BlockDriver bdrv_quorum = { .bdrv_aio_readv = quorum_aio_readv, .bdrv_aio_writev = quorum_aio_writev, .bdrv_invalidate_cache = quorum_invalidate_cache, + + .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, }; static void bdrv_quorum_init(void) From c88a1de51ab2f26a9a37ffc317249736de8c015c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:20 +0100 Subject: [PATCH 51/54] quorum: Add quorum_open() and quorum_close(). Example of command line: -drive if=virtio,driver=quorum,\ children.0.file.filename=1.raw,\ children.0.node-name=1.raw,\ children.0.driver=raw,\ children.1.file.filename=2.raw,\ children.1.node-name=2.raw,\ children.1.driver=raw,\ children.2.file.filename=3.raw,\ children.2.node-name=3.raw,\ children.2.driver=raw,\ vote-threshold=2 blkverify=on with vote-threshold=2 and two files can be passed to emulate blkverify. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block/quorum.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++ monitor.c | 3 + qapi-schema.json | 21 ++++++- 3 files changed, 184 insertions(+), 1 deletion(-) diff --git a/block/quorum.c b/block/quorum.c index 1b2b56f8a0..73dd45b6ff 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -20,6 +20,9 @@ #define HASH_LENGTH 32 +#define QUORUM_OPT_VOTE_THRESHOLD "vote-threshold" +#define QUORUM_OPT_BLKVERIFY "blkverify" + /* This union holds a vote hash value */ typedef union QuorumVoteValue { char h[HASH_LENGTH]; /* SHA-256 hash */ @@ -672,12 +675,170 @@ static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs, return false; } +static int quorum_valid_threshold(int threshold, int num_children, Error **errp) +{ + + if (threshold < 1) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, + "vote-threshold", "value >= 1"); + return -ERANGE; + } + + if (threshold > num_children) { + error_setg(errp, "threshold may not exceed children count"); + return -ERANGE; + } + + return 0; +} + +static QemuOptsList quorum_runtime_opts = { + .name = "quorum", + .head = QTAILQ_HEAD_INITIALIZER(quorum_runtime_opts.head), + .desc = { + { + .name = QUORUM_OPT_VOTE_THRESHOLD, + .type = QEMU_OPT_NUMBER, + .help = "The number of vote needed for reaching quorum", + }, + { + .name = QUORUM_OPT_BLKVERIFY, + .type = QEMU_OPT_BOOL, + .help = "Trigger block verify mode if set", + }, + { /* end of list */ } + }, +}; + +static int quorum_open(BlockDriverState *bs, QDict *options, int flags, + Error **errp) +{ + BDRVQuorumState *s = bs->opaque; + Error *local_err = NULL; + QemuOpts *opts; + bool *opened; + QDict *sub = NULL; + QList *list = NULL; + const QListEntry *lentry; + const QDictEntry *dentry; + int i; + int ret = 0; + + qdict_flatten(options); + qdict_extract_subqdict(options, &sub, "children."); + qdict_array_split(sub, &list); + + /* count how many different children are present and validate + * qdict_size(sub) address the open by reference case + */ + s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list); + if (s->num_children < 2) { + error_setg(&local_err, + "Number of provided children must be greater than 1"); + ret = -EINVAL; + goto exit; + } + + opts = qemu_opts_create(&quorum_runtime_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &local_err); + if (error_is_set(&local_err)) { + ret = -EINVAL; + goto exit; + } + + s->threshold = qemu_opt_get_number(opts, QUORUM_OPT_VOTE_THRESHOLD, 0); + + /* and validate it against s->num_children */ + ret = quorum_valid_threshold(s->threshold, s->num_children, &local_err); + if (ret < 0) { + goto exit; + } + + /* is the driver in blkverify mode */ + if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) && + s->num_children == 2 && s->threshold == 2) { + s->is_blkverify = true; + } else if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false)) { + fprintf(stderr, "blkverify mode is set by setting blkverify=on " + "and using two files with vote_threshold=2\n"); + } + + /* allocate the children BlockDriverState array */ + s->bs = g_new0(BlockDriverState *, s->num_children); + opened = g_new0(bool, s->num_children); + + /* Open by file name or options dict (command line or QMP) */ + if (s->num_children == qlist_size(list)) { + for (i = 0, lentry = qlist_first(list); lentry; + lentry = qlist_next(lentry), i++) { + QDict *d = qobject_to_qdict(lentry->value); + QINCREF(d); + ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL, &local_err); + if (ret < 0) { + goto close_exit; + } + opened[i] = true; + } + /* Open by QMP references */ + } else { + for (i = 0, dentry = qdict_first(sub); dentry; + dentry = qdict_next(sub, dentry), i++) { + QString *string = qobject_to_qstring(dentry->value); + ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL, + flags, NULL, &local_err); + if (ret < 0) { + goto close_exit; + } + opened[i] = true; + } + } + + g_free(opened); + goto exit; + +close_exit: + /* cleanup on error */ + for (i = 0; i < s->num_children; i++) { + if (!opened[i]) { + continue; + } + bdrv_unref(s->bs[i]); + } + g_free(s->bs); + g_free(opened); +exit: + /* propagate error */ + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); + } + QDECREF(list); + QDECREF(sub); + return ret; +} + +static void quorum_close(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + int i; + + for (i = 0; i < s->num_children; i++) { + bdrv_unref(s->bs[i]); + } + + g_free(s->bs); +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_file_open = quorum_open, + .bdrv_close = quorum_close, + + .authorizations = { true, true }, + .bdrv_co_flush_to_disk = quorum_co_flush, .bdrv_getlength = quorum_getlength, diff --git a/monitor.c b/monitor.c index 8ae095f16a..aebcbd8beb 100644 --- a/monitor.c +++ b/monitor.c @@ -640,6 +640,9 @@ static void monitor_protocol_event_init(void) monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000); monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000); monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000); + /* limit the rate of quorum events to avoid hammering the management */ + monitor_protocol_event_throttle(QEVENT_QUORUM_REPORT_BAD, 1000); + monitor_protocol_event_throttle(QEVENT_QUORUM_FAILURE, 1000); } /** diff --git a/qapi-schema.json b/qapi-schema.json index 473c096fa9..fcb2280053 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4431,6 +4431,24 @@ 'data': { 'test': 'BlockdevRef', 'raw': 'BlockdevRef' } } +## +# @BlockdevOptionsQuorum +# +# Driver specific block device options for Quorum +# +# @blkverify: #optional true if the driver must print content mismatch +# +# @children: the children block device to use +# +# @vote_threshold: the vote limit under which a read will fail +# +# Since: 2.0 +## +{ 'type': 'BlockdevOptionsQuorum', + 'data': { '*blkverify': 'bool', + 'children': [ 'BlockdevRef' ], + 'vote-threshold': 'int' } } + ## # @BlockdevOptions # @@ -4470,7 +4488,8 @@ 'vdi': 'BlockdevOptionsGenericFormat', 'vhdx': 'BlockdevOptionsGenericFormat', 'vmdk': 'BlockdevOptionsGenericCOWFormat', - 'vpc': 'BlockdevOptionsGenericFormat' + 'vpc': 'BlockdevOptionsGenericFormat', + 'quorum': 'BlockdevOptionsQuorum' } } ## From c7fc5bc2a4d89ccdb1ffabc720e7c87558c9aaef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 21 Feb 2014 22:21:21 +0100 Subject: [PATCH 52/54] quorum: Add unit test. Signed-off-by: Benoit Canet Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- tests/qemu-iotests/081 | 95 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/081.out | 34 ++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 130 insertions(+) create mode 100755 tests/qemu-iotests/081 create mode 100644 tests/qemu-iotests/081.out diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 new file mode 100755 index 0000000000..be345443c6 --- /dev/null +++ b/tests/qemu-iotests/081 @@ -0,0 +1,95 @@ +#!/bin/bash +# +# Test Quorum block driver +# +# Copyright (C) 2013 Nodalink, SARL. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=benoit@irqsave.net + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -rf $TEST_DIR/1.raw + rm -rf $TEST_DIR/2.raw + rm -rf $TEST_DIR/3.raw +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt raw +_supported_proto generic +_supported_os Linux + +quorum="file.driver=quorum,file.children.0.file.filename=$TEST_DIR/1.raw" +quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw" +quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2" + +echo +echo "== creating quorum files ==" + +size=10M + +TEST_IMG="$TEST_DIR/1.raw" _make_test_img $size +TEST_IMG="$TEST_DIR/2.raw" _make_test_img $size +TEST_IMG="$TEST_DIR/3.raw" _make_test_img $size + +echo +echo "== writing images ==" + +$QEMU_IO -c "open -o $quorum" -c "write -P 0x32 0 $size" | _filter_qemu_io + +echo +echo "== checking quorum write ==" + +$QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io +$QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io +$QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/3.raw" | _filter_qemu_io + +echo +echo "== corrupting image ==" + +$QEMU_IO -c "write -P 0x42 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io + +echo +echo "== checking quorum correction ==" + +$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io + +echo +echo "== breaking quorum ==" + +$QEMU_IO -c "write -P 0x41 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io +echo +echo "== checking that quorum is broken ==" + +$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out new file mode 100644 index 0000000000..b5b55abc7b --- /dev/null +++ b/tests/qemu-iotests/081.out @@ -0,0 +1,34 @@ +QA output created by 081 + +== creating quorum files == +Formatting 'TEST_DIR/1.IMGFMT', fmt=IMGFMT size=10485760 +Formatting 'TEST_DIR/2.IMGFMT', fmt=IMGFMT size=10485760 +Formatting 'TEST_DIR/3.IMGFMT', fmt=IMGFMT size=10485760 + +== writing images == +wrote 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== checking quorum write == +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== corrupting image == +wrote 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== checking quorum correction == +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== breaking quorum == +wrote 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== checking that quorum is broken == +qemu-io: can't open device (null): Could not read image for determining its format: Input/output error +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 80b181f8b1..db127d924d 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -83,4 +83,5 @@ 074 rw auto 077 rw auto 079 rw auto +081 rw auto 082 rw auto quick From 8a87f3d72279acb89f3d09b28d285d2fb6a7decf Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 22:30:37 +0100 Subject: [PATCH 53/54] quorum: Simplify quorum_open() Although it may not look like it, this patch simplifies quorum_open(). qdict_array_split() is now able to return QLists with different objects than only QDicts, therefore it will now do all the work and quorum_open() does not have to handle reference strings by itself. This allows mixing full option dicts and reference strings for specifying the child block devices of quorum; furthermore, it improves handling of malformed specifications. Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Signed-off-by: Kevin Wolf --- block/quorum.c | 66 +++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index 73dd45b6ff..6c28239718 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -720,7 +720,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, QDict *sub = NULL; QList *list = NULL; const QListEntry *lentry; - const QDictEntry *dentry; int i; int ret = 0; @@ -728,10 +727,15 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, qdict_extract_subqdict(options, &sub, "children."); qdict_array_split(sub, &list); - /* count how many different children are present and validate - * qdict_size(sub) address the open by reference case - */ - s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list); + if (qdict_size(sub)) { + error_setg(&local_err, "Invalid option children.%s", + qdict_first(sub)->key); + ret = -EINVAL; + goto exit; + } + + /* count how many different children are present */ + s->num_children = qlist_size(list); if (s->num_children < 2) { error_setg(&local_err, "Number of provided children must be greater than 1"); @@ -767,30 +771,38 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, s->bs = g_new0(BlockDriverState *, s->num_children); opened = g_new0(bool, s->num_children); - /* Open by file name or options dict (command line or QMP) */ - if (s->num_children == qlist_size(list)) { - for (i = 0, lentry = qlist_first(list); lentry; - lentry = qlist_next(lentry), i++) { - QDict *d = qobject_to_qdict(lentry->value); - QINCREF(d); - ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL, &local_err); - if (ret < 0) { - goto close_exit; - } - opened[i] = true; + for (i = 0, lentry = qlist_first(list); lentry; + lentry = qlist_next(lentry), i++) { + QDict *d; + QString *string; + + switch (qobject_type(lentry->value)) + { + /* List of options */ + case QTYPE_QDICT: + d = qobject_to_qdict(lentry->value); + QINCREF(d); + ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL, + &local_err); + break; + + /* QMP reference */ + case QTYPE_QSTRING: + string = qobject_to_qstring(lentry->value); + ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL, + flags, NULL, &local_err); + break; + + default: + error_setg(&local_err, "Specification of child block device %i " + "is invalid", i); + ret = -EINVAL; } - /* Open by QMP references */ - } else { - for (i = 0, dentry = qdict_first(sub); dentry; - dentry = qdict_next(sub, dentry), i++) { - QString *string = qobject_to_qstring(dentry->value); - ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL, - flags, NULL, &local_err); - if (ret < 0) { - goto close_exit; - } - opened[i] = true; + + if (ret < 0) { + goto close_exit; } + opened[i] = true; } g_free(opened); From 6141f3bd6904df7cf9519c6444a14a608b9874c4 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 21 Feb 2014 22:30:38 +0100 Subject: [PATCH 54/54] iotests: Mixed quorum child device specifications Add a test case to test 081 for mixing full option dicts and reference strings of specifying the quorum child block devices through QMP. Signed-off-by: Max Reitz Reviewed-by: Benoit Canet Signed-off-by: Kevin Wolf --- tests/qemu-iotests/081 | 51 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/081.out | 15 +++++++++++ 2 files changed, 66 insertions(+) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index be345443c6..f053f11942 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -44,6 +44,18 @@ _supported_fmt raw _supported_proto generic _supported_os Linux +function do_run_qemu() +{ + echo Testing: "$@" | _filter_imgfmt + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io +} + quorum="file.driver=quorum,file.children.0.file.filename=$TEST_DIR/1.raw" quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw" quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2" @@ -79,6 +91,45 @@ echo "== checking quorum correction ==" $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io +echo +echo "== checking mixed reference/option specification ==" + +run_qemu -drive "file=$TEST_DIR/2.raw,format=$IMGFMT,if=none,id=drive2" <