From 7cf6376ae80e320fdaa6c9020f3e8ac1eecf1307 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 20 May 2014 09:10:32 +0800 Subject: [PATCH 01/33] qemu-iotests: Handle cache mode option in 091 We should allow testing this on tmpfs. Any cache setting in iotests should try to obey $CACHEMODE. The cache mode is still "none" by default but overridable Signed-off-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/091 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091 index 384b3ace54..32bbd56975 100755 --- a/tests/qemu-iotests/091 +++ b/tests/qemu-iotests/091 @@ -47,6 +47,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file _supported_os Linux +_default_cache_mode "none" +_supported_cache_modes "writethrough" "none" "writeback" size=1G @@ -59,13 +61,13 @@ echo === Starting QEMU VM1 === echo qemu_comm_method="monitor" -_launch_qemu -drive file="${TEST_IMG}",cache=none,id=disk +_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk h1=$QEMU_HANDLE echo echo === Starting QEMU VM2 === echo -_launch_qemu -drive file="${TEST_IMG}",cache=none,id=disk \ +_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk \ -incoming "exec: cat '${MIG_FIFO}'" h2=$QEMU_HANDLE From 4ba6fabfb447661ee44ee593b2c454c7ffd1183f Mon Sep 17 00:00:00 2001 From: Leandro Dorileo Date: Mon, 19 May 2014 18:53:55 -0300 Subject: [PATCH 02/33] QemuOpt: add unit tests Cover basic aspects and API usage for QemuOpt. The current implementation covers the API's planned to be changed by Chunyan Liu in his QEMUOptionParameter replacement/cleanup job. Other APIs should be covered in future improvements. [Squashing in a small fix "QemuOpt: use qemu_find_opts_err() to avoid output on stderr in tests". qemu_find_opts() calls error_report() instead of propagating the Error object. It is undesirable to clutter test case output with error messages from a passing test. Use qemu_find_opts_err() to avoid the output on stderr. --Stefan] Signed-off-by: Leandro Dorileo Signed-off-by: Stefan Hajnoczi --- tests/Makefile | 3 + tests/test-qemu-opts.c | 441 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 444 insertions(+) create mode 100644 tests/test-qemu-opts.c diff --git a/tests/Makefile b/tests/Makefile index 9f7ca61ae3..8f71e0d664 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -60,6 +60,8 @@ check-unit-y += tests/test-qdev-global-props$(EXESUF) check-unit-y += tests/check-qom-interface$(EXESUF) gcov-files-check-qom-interface-y = qom/object.c check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF) +check-unit-y += tests/test-qemu-opts$(EXESUF) +gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh @@ -319,6 +321,7 @@ tests/intel-hda-test$(EXESUF): tests/intel-hda-test.o tests/ioh3420-test$(EXESUF): tests/ioh3420-test.o tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o +tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a # QTest rules diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c new file mode 100644 index 0000000000..3653507f56 --- /dev/null +++ b/tests/test-qemu-opts.c @@ -0,0 +1,441 @@ +/* + * QemuOpts unit-tests. + * + * Copyright (C) 2014 Leandro Dorileo + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ + +#include "qapi/error.h" +#include "qapi/qmp/qstring.h" +#include "qemu/config-file.h" + +#include +#include + +static QemuOptsList opts_list_01 = { + .name = "opts_list_01", + .head = QTAILQ_HEAD_INITIALIZER(opts_list_01.head), + .desc = { + { + .name = "str1", + .type = QEMU_OPT_STRING, + },{ + .name = "str2", + .type = QEMU_OPT_STRING, + },{ + .name = "str3", + .type = QEMU_OPT_STRING, + },{ + .name = "number1", + .type = QEMU_OPT_NUMBER, + }, + { /* end of list */ } + }, +}; + +static QemuOptsList opts_list_02 = { + .name = "opts_list_02", + .head = QTAILQ_HEAD_INITIALIZER(opts_list_02.head), + .desc = { + { + .name = "str1", + .type = QEMU_OPT_STRING, + },{ + .name = "bool1", + .type = QEMU_OPT_BOOL, + },{ + .name = "str2", + .type = QEMU_OPT_STRING, + },{ + .name = "size1", + .type = QEMU_OPT_SIZE, + }, + { /* end of list */ } + }, +}; + +QemuOptsList opts_list_03 = { + .name = "opts_list_03", + .head = QTAILQ_HEAD_INITIALIZER(opts_list_03.head), + .desc = { + /* no elements => accept any params */ + { /* end of list */ } + }, +}; + +static void register_opts(void) +{ + qemu_add_opts(&opts_list_01); + qemu_add_opts(&opts_list_02); + qemu_add_opts(&opts_list_03); +} + +static void test_find_unknown_opts(void) +{ + QemuOptsList *list; + Error *err = NULL; + + /* should not return anything, we don't have an "unknown" option */ + list = qemu_find_opts_err("unknown", &err); + g_assert(list == NULL); + g_assert(err); + error_free(err); +} + +static void test_qemu_find_opts(void) +{ + QemuOptsList *list; + + /* we have an "opts_list_01" option, should return it */ + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert_cmpstr(list->name, ==, "opts_list_01"); +} + +static void test_qemu_opts_create(void) +{ + QemuOptsList *list; + QemuOpts *opts; + + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_01"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* now we've create the opts, must find it */ + opts = qemu_opts_find(list, NULL); + g_assert(opts != NULL); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opt_get(void) +{ + QemuOptsList *list; + QemuOpts *opts; + const char *opt = NULL; + + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_01"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* haven't set anything to str2 yet */ + opt = qemu_opt_get(opts, "str2"); + g_assert(opt == NULL); + + qemu_opt_set(opts, "str2", "value"); + + /* now we have set str2, should know about it */ + opt = qemu_opt_get(opts, "str2"); + g_assert_cmpstr(opt, ==, "value"); + + qemu_opt_set(opts, "str2", "value2"); + + /* having reset the value, the returned should be the reset one */ + opt = qemu_opt_get(opts, "str2"); + g_assert_cmpstr(opt, ==, "value2"); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opt_get_bool(void) +{ + QemuOptsList *list; + QemuOpts *opts; + bool opt; + int ret; + + list = qemu_find_opts("opts_list_02"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_02"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* haven't set anything to bool1 yet, so defval should be returned */ + opt = qemu_opt_get_bool(opts, "bool1", false); + g_assert(opt == false); + + ret = qemu_opt_set_bool(opts, "bool1", true); + g_assert(ret == 0); + + /* now we have set bool1, should know about it */ + opt = qemu_opt_get_bool(opts, "bool1", false); + g_assert(opt == true); + + /* having reset the value, opt should be the reset one not defval */ + ret = qemu_opt_set_bool(opts, "bool1", false); + g_assert(ret == 0); + + opt = qemu_opt_get_bool(opts, "bool1", true); + g_assert(opt == false); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opt_get_number(void) +{ + QemuOptsList *list; + QemuOpts *opts; + uint64_t opt; + int ret; + + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_01"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* haven't set anything to number1 yet, so defval should be returned */ + opt = qemu_opt_get_number(opts, "number1", 5); + g_assert(opt == 5); + + ret = qemu_opt_set_number(opts, "number1", 10); + g_assert(ret == 0); + + /* now we have set number1, should know about it */ + opt = qemu_opt_get_number(opts, "number1", 5); + g_assert(opt == 10); + + /* having reset it, the returned should be the reset one not defval */ + ret = qemu_opt_set_number(opts, "number1", 15); + g_assert(ret == 0); + + opt = qemu_opt_get_number(opts, "number1", 5); + g_assert(opt == 15); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opt_get_size(void) +{ + QemuOptsList *list; + QemuOpts *opts; + uint64_t opt; + QDict *dict; + + list = qemu_find_opts("opts_list_02"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_02"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* haven't set anything to size1 yet, so defval should be returned */ + opt = qemu_opt_get_size(opts, "size1", 5); + g_assert(opt == 5); + + dict = qdict_new(); + g_assert(dict != NULL); + + qdict_put(dict, "size1", qstring_from_str("10")); + + qemu_opts_absorb_qdict(opts, dict, &error_abort); + g_assert(error_abort == NULL); + + /* now we have set size1, should know about it */ + opt = qemu_opt_get_size(opts, "size1", 5); + g_assert(opt == 10); + + /* reset value */ + qdict_put(dict, "size1", qstring_from_str("15")); + + qemu_opts_absorb_qdict(opts, dict, &error_abort); + g_assert(error_abort == NULL); + + /* test the reset value */ + opt = qemu_opt_get_size(opts, "size1", 5); + g_assert(opt == 15); + + qdict_del(dict, "size1"); + g_free(dict); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opt_unset(void) +{ + QemuOpts *opts; + const char *value; + int ret; + + /* dynamically initialized (parsed) opts */ + opts = qemu_opts_parse(&opts_list_03, "key=value", 0); + g_assert(opts != NULL); + + /* check default/parsed value */ + value = qemu_opt_get(opts, "key"); + g_assert_cmpstr(value, ==, "value"); + + /* reset it to value2 */ + qemu_opt_set(opts, "key", "value2"); + + value = qemu_opt_get(opts, "key"); + g_assert_cmpstr(value, ==, "value2"); + + /* unset, valid only for "accept any" */ + ret = qemu_opt_unset(opts, "key"); + g_assert(ret == 0); + + /* after reset the value should be the parsed/default one */ + value = qemu_opt_get(opts, "key"); + g_assert_cmpstr(value, ==, "value"); + + qemu_opts_del(opts); +} + +static void test_qemu_opts_reset(void) +{ + QemuOptsList *list; + QemuOpts *opts; + uint64_t opt; + int ret; + + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_01"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* create the opts */ + opts = qemu_opts_create(list, NULL, 0, &error_abort); + g_assert(opts != NULL); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* haven't set anything to number1 yet, so defval should be returned */ + opt = qemu_opt_get_number(opts, "number1", 5); + g_assert(opt == 5); + + ret = qemu_opt_set_number(opts, "number1", 10); + g_assert(ret == 0); + + /* now we have set number1, should know about it */ + opt = qemu_opt_get_number(opts, "number1", 5); + g_assert(opt == 10); + + qemu_opts_reset(list); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +static void test_qemu_opts_set(void) +{ + QemuOptsList *list; + QemuOpts *opts; + int ret; + const char *opt; + + list = qemu_find_opts("opts_list_01"); + g_assert(list != NULL); + g_assert(QTAILQ_EMPTY(&list->head)); + g_assert_cmpstr(list->name, ==, "opts_list_01"); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); + + /* implicitly create opts and set str3 value */ + ret = qemu_opts_set(list, NULL, "str3", "value"); + g_assert(ret == 0); + g_assert(!QTAILQ_EMPTY(&list->head)); + + /* get the just created opts */ + opts = qemu_opts_find(list, NULL); + g_assert(opts != NULL); + + /* check the str3 value */ + opt = qemu_opt_get(opts, "str3"); + g_assert_cmpstr(opt, ==, "value"); + + qemu_opts_del(opts); + + /* should not find anything at this point */ + opts = qemu_opts_find(list, NULL); + g_assert(opts == NULL); +} + +int main(int argc, char *argv[]) +{ + register_opts(); + g_test_init(&argc, &argv, NULL); + g_test_add_func("/qemu-opts/find_unknown_opts", test_find_unknown_opts); + g_test_add_func("/qemu-opts/find_opts", test_qemu_find_opts); + g_test_add_func("/qemu-opts/opts_create", test_qemu_opts_create); + g_test_add_func("/qemu-opts/opt_get", test_qemu_opt_get); + g_test_add_func("/qemu-opts/opt_get_bool", test_qemu_opt_get_bool); + g_test_add_func("/qemu-opts/opt_get_number", test_qemu_opt_get_number); + g_test_add_func("/qemu-opts/opt_get_size", test_qemu_opt_get_size); + g_test_add_func("/qemu-opts/opt_unset", test_qemu_opt_unset); + g_test_add_func("/qemu-opts/opts_reset", test_qemu_opts_reset); + g_test_add_func("/qemu-opts/opts_set", test_qemu_opts_set); + g_test_run(); + return 0; +} From bd60436936ecebf2c683eef9ea561f2b949700f3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 19 May 2014 17:56:01 +0200 Subject: [PATCH 03/33] qcow2: Fix memory leak in COW error path This triggers if bs->drv becomes NULL in a concurrent request. This is currently only the case when corruption prevention kicks in (i.e. at most once per image, and after that it produces I/O errors). Signed-off-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- block/qcow2-cluster.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 76d2bcf63a..4208dc08b5 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -379,7 +379,8 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs, BLKDBG_EVENT(bs->file, BLKDBG_COW_READ); if (!bs->drv) { - return -ENOMEDIUM; + ret = -ENOMEDIUM; + goto out; } /* Call .bdrv_co_readv() directly instead of using the public block-layer From 271c0f68b4eae72691721243a1c37f46a3232d61 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Wed, 21 May 2014 10:42:13 +0800 Subject: [PATCH 04/33] aio: Fix use-after-free in cancellation path The current flow of canceling a thread from THREAD_ACTIVE state is: 1) Caller wants to cancel a request, so it calls thread_pool_cancel. 2) thread_pool_cancel waits on the conditional variable elem->check_cancel. 3) The worker thread changes state to THREAD_DONE once the task is done, and notifies elem->check_cancel to allow thread_pool_cancel to continue execution, and signals the notifier (pool->notifier) to allow callback function to be called later. But because of the global mutex, the notifier won't get processed until step 4) and 5) are done. 4) thread_pool_cancel continues, leaving the notifier signaled, it just returns to caller. 5) Caller thinks the request is already canceled successfully, so it releases any related data, such as freeing elem->common.opaque. 6) In the next main loop iteration, the notifier handler, event_notifier_ready, is called. It finds the canceled thread in THREAD_DONE state, so calls elem->common.cb, with an (likely) dangling opaque pointer. This is a use-after-free. Fix it by calling event_notifier_ready before leaving thread_pool_cancel. Test case update: This change will let cancel complete earlier than test-thread-pool.c expects, so update the code to check this case: if it's already done, done_cb sets .aiocb to NULL, skip calling bdrv_aio_cancel on them. Reported-by: Ulrich Obergfell Suggested-by: Paolo Bonzini Signed-off-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- tests/test-thread-pool.c | 2 +- thread-pool.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c index c1f8e13a9f..aa156bcd32 100644 --- a/tests/test-thread-pool.c +++ b/tests/test-thread-pool.c @@ -180,7 +180,7 @@ static void test_cancel(void) /* Canceling the others will be a blocking operation. */ for (i = 0; i < 100; i++) { - if (data[i].n != 3) { + if (data[i].aiocb && data[i].n != 3) { bdrv_aio_cancel(data[i].aiocb); } } diff --git a/thread-pool.c b/thread-pool.c index fbdd3ffa3a..dfb699dd94 100644 --- a/thread-pool.c +++ b/thread-pool.c @@ -224,6 +224,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb) pool->pending_cancellations--; } qemu_mutex_unlock(&pool->lock); + event_notifier_ready(&pool->notifier); } static const AIOCBInfo thread_pool_aiocb_info = { From 0bf7488afbf42ccde75eb390afe5ea494c47f0c9 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 20 May 2014 22:43:19 +0200 Subject: [PATCH 05/33] iotests: Use _img_info in test 089 Currently, test 089 uses $QEMU_IMG info manually in order to obtain the according output. However, the iotests should generally use _img_info as this filters out more irrelevant information such as the host image size or format specific information. Therefore, test 089 should use _img_info as well. Signed-off-by: Max Reitz Reported-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/089 | 3 +-- tests/qemu-iotests/089.out | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089 index ef476010c0..dffc977e1c 100755 --- a/tests/qemu-iotests/089 +++ b/tests/qemu-iotests/089 @@ -107,8 +107,7 @@ echo echo "=== Testing qemu-img info output ===" echo -$QEMU_IMG info "json:{\"driver\":\"qcow2\",\"file.filename\":\"$TEST_IMG\"}" \ - | _filter_testdir | _filter_imgfmt +TEST_IMG="json:{\"driver\":\"qcow2\",\"file.filename\":\"$TEST_IMG\"}" _img_info echo diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out index d6cf7835d8..4ca2f88e6a 100644 --- a/tests/qemu-iotests/089.out +++ b/tests/qemu-iotests/089.out @@ -31,11 +31,7 @@ read failed: Input/output error image: TEST_DIR/t.IMGFMT file format: IMGFMT virtual size: 64M (67108864 bytes) -disk size: 324K cluster_size: 65536 -Format specific information: - compat: 1.1 - lazy refcounts: false === Testing option merging === From 8574575f90e73ec93c2ef4bda4ae41c92fc8e644 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:41 +0800 Subject: [PATCH 06/33] block: Add BlockOpType enum This adds the enum of all the operations that can be taken on a block device. Signed-off-by: Fam Zheng Reviewed-by: Benoit Canet Reviewed-by: Jeff Cody Reviewed-by: Stefan Hajnoczi Signed-off-by: Stefan Hajnoczi --- include/block/block.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/include/block/block.h b/include/block/block.h index 59be83f3c2..cc4cc160ec 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -162,6 +162,25 @@ typedef struct BDRVReopenState { void *opaque; } BDRVReopenState; +/* + * Block operation types + */ +typedef enum BlockOpType { + BLOCK_OP_TYPE_BACKUP_SOURCE, + BLOCK_OP_TYPE_BACKUP_TARGET, + BLOCK_OP_TYPE_CHANGE, + BLOCK_OP_TYPE_COMMIT, + BLOCK_OP_TYPE_DATAPLANE, + BLOCK_OP_TYPE_DRIVE_DEL, + BLOCK_OP_TYPE_EJECT, + BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, + BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, + BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, + BLOCK_OP_TYPE_MIRROR, + BLOCK_OP_TYPE_RESIZE, + BLOCK_OP_TYPE_STREAM, + BLOCK_OP_TYPE_MAX, +} BlockOpType; void bdrv_iostatus_enable(BlockDriverState *bs); void bdrv_iostatus_reset(BlockDriverState *bs); From fbe40ff780564526e6f639b3b78366727d34955c Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:42 +0800 Subject: [PATCH 07/33] block: Introduce op_blockers to BlockDriverState BlockDriverState.op_blockers is an array of lists with BLOCK_OP_TYPE_MAX elements. Each list is a list of blockers of an operation type (BlockOpType), that marks this BDS as currently blocked for a certain type of operation with reason errors stored in the list. The rule of usage is: * BDS user who wants to take an operation should check if there's any blocker of the type with bdrv_op_is_blocked(). * BDS user who wants to block certain types of operation, should call bdrv_op_block (or bdrv_op_block_all to block all types of operations, which is similar to the existing bdrv_set_in_use()). * A blocker is only referenced by op_blockers, so the lifecycle is managed by caller, and shouldn't be lost until unblock, so typically a caller does these: - Allocate a blocker with error_setg or similar, call bdrv_op_block() to block some operations. - Hold the blocker, do his job. - Unblock operations that it blocked, with the same reason pointer passed to bdrv_op_unblock(). - Release the blocker with error_free(). Signed-off-by: Fam Zheng Reviewed-by: Benoit Canet Reviewed-by: Jeff Cody Reviewed-by: Stefan Hajnoczi Signed-off-by: Stefan Hajnoczi --- block.c | 76 +++++++++++++++++++++++++++++++++++++++ include/block/block.h | 7 ++++ include/block/block_int.h | 5 +++ 3 files changed, 88 insertions(+) diff --git a/block.c b/block.c index 40c5e1aa73..589c6e3376 100644 --- a/block.c +++ b/block.c @@ -335,6 +335,7 @@ void bdrv_register(BlockDriver *bdrv) BlockDriverState *bdrv_new(const char *device_name, Error **errp) { BlockDriverState *bs; + int i; if (bdrv_find(device_name)) { error_setg(errp, "Device with id '%s' already exists", @@ -353,6 +354,9 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp) if (device_name[0] != '\0') { QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list); } + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + QLIST_INIT(&bs->op_blockers[i]); + } bdrv_iostatus_disable(bs); notifier_list_init(&bs->close_notifiers); notifier_with_return_list_init(&bs->before_write_notifiers); @@ -1952,6 +1956,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name), bs_src->device_name); bs_dest->device_list = bs_src->device_list; + memcpy(bs_dest->op_blockers, bs_src->op_blockers, + sizeof(bs_dest->op_blockers)); } /* @@ -5325,6 +5331,76 @@ void bdrv_unref(BlockDriverState *bs) } } +struct BdrvOpBlocker { + Error *reason; + QLIST_ENTRY(BdrvOpBlocker) list; +}; + +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp) +{ + BdrvOpBlocker *blocker; + assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX); + if (!QLIST_EMPTY(&bs->op_blockers[op])) { + blocker = QLIST_FIRST(&bs->op_blockers[op]); + if (errp) { + error_setg(errp, "Device '%s' is busy: %s", + bs->device_name, error_get_pretty(blocker->reason)); + } + return true; + } + return false; +} + +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason) +{ + BdrvOpBlocker *blocker; + assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX); + + blocker = g_malloc0(sizeof(BdrvOpBlocker)); + blocker->reason = reason; + QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); +} + +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason) +{ + BdrvOpBlocker *blocker, *next; + assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX); + QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) { + if (blocker->reason == reason) { + QLIST_REMOVE(blocker, list); + g_free(blocker); + } + } +} + +void bdrv_op_block_all(BlockDriverState *bs, Error *reason) +{ + int i; + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + bdrv_op_block(bs, i, reason); + } +} + +void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason) +{ + int i; + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + bdrv_op_unblock(bs, i, reason); + } +} + +bool bdrv_op_blocker_is_empty(BlockDriverState *bs) +{ + int i; + + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + if (!QLIST_EMPTY(&bs->op_blockers[i])) { + return false; + } + } + return true; +} + void bdrv_set_in_use(BlockDriverState *bs, int in_use) { assert(bs->in_use != in_use); diff --git a/include/block/block.h b/include/block/block.h index cc4cc160ec..7f0448c19f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -475,6 +475,13 @@ void bdrv_unref(BlockDriverState *bs); void bdrv_set_in_use(BlockDriverState *bs, int in_use); int bdrv_in_use(BlockDriverState *bs); +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason); +void bdrv_op_block_all(BlockDriverState *bs, Error *reason); +void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason); +bool bdrv_op_blocker_is_empty(BlockDriverState *bs); + #ifdef CONFIG_LINUX_AIO int raw_get_aio_fd(BlockDriverState *bs); #else diff --git a/include/block/block_int.h b/include/block/block_int.h index b8cc926bfe..fdf2a7da8f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -270,6 +270,8 @@ typedef struct BlockLimits { size_t opt_mem_alignment; } BlockLimits; +typedef struct BdrvOpBlocker BdrvOpBlocker; + /* * Note: the function bdrv_append() copies and swaps contents of * BlockDriverStates, so if you add new fields to this struct, please @@ -360,6 +362,9 @@ struct BlockDriverState { QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; + /* operation blockers */ + QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX]; + /* long-running background operation */ BlockJob *job; From 3718d8ab65f68de2acccbe6a315907805f54e3cc Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:43 +0800 Subject: [PATCH 08/33] block: Replace in_use with operation blocker This drops BlockDriverState.in_use with op_blockers: - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1). - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0). - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs). The specific types are used, e.g. in place of starting block backup, bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...). There is one exception in block_job_create, where bdrv_op_blocker_is_empty() is used, because we don't know the operation type here. This doesn't matter because in a few commits away we will drop the check and move it to callers that _do_ know the type. - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use). Note: there is only bdrv_op_block_all and bdrv_op_unblock_all callers at this moment. So although the checks are specific to op types, this changes can still be seen as identical logic with previously with in_use. The difference is error message are improved because of blocker error info. Signed-off-by: Fam Zheng Reviewed-by: Jeff Cody Reviewed-by: Stefan Hajnoczi Signed-off-by: Stefan Hajnoczi --- block-migration.c | 7 +++++-- block.c | 24 +++++++----------------- blockdev.c | 19 +++++++++---------- blockjob.c | 14 +++++++++----- hw/block/dataplane/virtio-blk.c | 18 ++++++++++++------ include/block/block.h | 2 -- include/block/block_int.h | 1 - include/block/blockjob.h | 3 +++ 8 files changed, 45 insertions(+), 43 deletions(-) diff --git a/block-migration.c b/block-migration.c index 56951e09ae..16562709c8 100644 --- a/block-migration.c +++ b/block-migration.c @@ -59,6 +59,7 @@ typedef struct BlkMigDevState { unsigned long *aio_bitmap; int64_t completed_sectors; BdrvDirtyBitmap *dirty_bitmap; + Error *blocker; } BlkMigDevState; typedef struct BlkMigBlock { @@ -361,7 +362,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs) bmds->completed_sectors = 0; bmds->shared_base = block_mig_state.shared_base; alloc_aio_bitmap(bmds); - bdrv_set_in_use(bs, 1); + error_setg(&bmds->blocker, "block device is in use by migration"); + bdrv_op_block_all(bs, bmds->blocker); bdrv_ref(bs); block_mig_state.total_sector_sum += sectors; @@ -599,7 +601,8 @@ static void blk_mig_cleanup(void) blk_mig_lock(); while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) { QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry); - bdrv_set_in_use(bmds->bs, 0); + bdrv_op_unblock_all(bmds->bs, bmds->blocker); + error_free(bmds->blocker); bdrv_unref(bmds->bs); g_free(bmds->aio_bitmap); g_free(bmds); diff --git a/block.c b/block.c index 589c6e3376..64c48a54a1 100644 --- a/block.c +++ b/block.c @@ -1949,7 +1949,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, bs_dest->refcnt = bs_src->refcnt; /* job */ - bs_dest->in_use = bs_src->in_use; bs_dest->job = bs_src->job; /* keep the same entry in bdrv_states */ @@ -1992,7 +1991,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); assert(bs_new->job == NULL); assert(bs_new->dev == NULL); - assert(bs_new->in_use == 0); + assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); @@ -2011,7 +2010,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) /* Check a few fields that should remain attached to the device */ assert(bs_new->dev == NULL); assert(bs_new->job == NULL); - assert(bs_new->in_use == 0); + assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); @@ -2056,7 +2055,7 @@ static void bdrv_delete(BlockDriverState *bs) { assert(!bs->dev); assert(!bs->job); - assert(!bs->in_use); + assert(bdrv_op_blocker_is_empty(bs)); assert(!bs->refcnt); assert(QLIST_EMPTY(&bs->dirty_bitmaps)); @@ -2238,7 +2237,8 @@ int bdrv_commit(BlockDriverState *bs) return -ENOTSUP; } - if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) { + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, NULL) || + bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, NULL)) { return -EBUSY; } @@ -3500,8 +3500,9 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset) return -ENOTSUP; if (bs->read_only) return -EACCES; - if (bdrv_in_use(bs)) + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) { return -EBUSY; + } ret = drv->bdrv_truncate(bs, offset); if (ret == 0) { ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); @@ -5401,17 +5402,6 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs) return true; } -void bdrv_set_in_use(BlockDriverState *bs, int in_use) -{ - assert(bs->in_use != in_use); - bs->in_use = in_use; -} - -int bdrv_in_use(BlockDriverState *bs) -{ - return bs->in_use; -} - void bdrv_iostatus_enable(BlockDriverState *bs) { bs->iostatus_enabled = true; diff --git a/blockdev.c b/blockdev.c index 1cbcc1c785..0a519029b8 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1334,8 +1334,8 @@ static void external_snapshot_prepare(BlkTransactionState *common, return; } - if (bdrv_in_use(state->old_bs)) { - error_set(errp, QERR_DEVICE_IN_USE, device); + if (bdrv_op_is_blocked(state->old_bs, + BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { return; } @@ -1557,8 +1557,7 @@ exit: static void eject_device(BlockDriverState *bs, int force, Error **errp) { - if (bdrv_in_use(bs)) { - error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)); + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) { return; } if (!bdrv_dev_has_removable_media(bs)) { @@ -1760,14 +1759,16 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) { const char *id = qdict_get_str(qdict, "id"); BlockDriverState *bs; + Error *local_err = NULL; bs = bdrv_find(id); if (!bs) { qerror_report(QERR_DEVICE_NOT_FOUND, id); return -1; } - if (bdrv_in_use(bs)) { - qerror_report(QERR_DEVICE_IN_USE, id); + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) { + error_report("%s", error_get_pretty(local_err)); + error_free(local_err); return -1; } @@ -2023,8 +2024,7 @@ void qmp_drive_backup(const char *device, const char *target, } } - if (bdrv_in_use(bs)) { - error_set(errp, QERR_DEVICE_IN_USE, device); + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { return; } @@ -2157,8 +2157,7 @@ void qmp_drive_mirror(const char *device, const char *target, } } - if (bdrv_in_use(bs)) { - error_set(errp, QERR_DEVICE_IN_USE, device); + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) { return; } diff --git a/blockjob.c b/blockjob.c index cd4784f053..60e72f5d90 100644 --- a/blockjob.c +++ b/blockjob.c @@ -41,14 +41,16 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, { BlockJob *job; - if (bs->job || bdrv_in_use(bs)) { + if (bs->job || !bdrv_op_blocker_is_empty(bs)) { error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)); return NULL; } bdrv_ref(bs); - bdrv_set_in_use(bs, 1); - job = g_malloc0(driver->instance_size); + error_setg(&job->blocker, "block device is in use by block job: %s", + BlockJobType_lookup[driver->job_type]); + bdrv_op_block_all(bs, job->blocker); + job->driver = driver; job->bs = bs; job->cb = cb; @@ -63,8 +65,9 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, block_job_set_speed(job, speed, &local_err); if (local_err) { bs->job = NULL; + bdrv_op_unblock_all(bs, job->blocker); + error_free(job->blocker); g_free(job); - bdrv_set_in_use(bs, 0); error_propagate(errp, local_err); return NULL; } @@ -79,8 +82,9 @@ void block_job_completed(BlockJob *job, int ret) assert(bs->job == job); job->cb(job->opaque, ret); bs->job = NULL; + bdrv_op_unblock_all(bs, job->blocker); + error_free(job->blocker); g_free(job); - bdrv_set_in_use(bs, 0); } void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 70b8a5ab75..e49c2536b1 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -70,6 +70,9 @@ struct VirtIOBlockDataPlane { queue */ unsigned int num_reqs; + + /* Operation blocker on BDS */ + Error *blocker; }; /* Raise an interrupt to signal guest, if necessary */ @@ -350,6 +353,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, { VirtIOBlockDataPlane *s; int fd; + Error *local_err = NULL; *dataplane = NULL; @@ -372,9 +376,10 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, /* If dataplane is (re-)enabled while the guest is running there could be * block jobs that can conflict. */ - if (bdrv_in_use(blk->conf.bs)) { - error_setg(errp, - "cannot start dataplane thread while device is in use"); + if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) { + error_report("cannot start dataplane thread: %s", + error_get_pretty(local_err)); + error_free(local_err); return; } @@ -406,8 +411,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, } s->ctx = iothread_get_aio_context(s->iothread); - /* Prevent block operations that conflict with data plane thread */ - bdrv_set_in_use(blk->conf.bs, 1); + error_setg(&s->blocker, "block device is in use by data plane"); + bdrv_op_block_all(blk->conf.bs, s->blocker); *dataplane = s; } @@ -420,7 +425,8 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) } virtio_blk_data_plane_stop(s); - bdrv_set_in_use(s->blk->conf.bs, 0); + bdrv_op_unblock_all(s->blk->conf.bs, s->blocker); + error_free(s->blocker); object_unref(OBJECT(s->iothread)); g_free(s); } diff --git a/include/block/block.h b/include/block/block.h index 7f0448c19f..22935f1cbd 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -472,8 +472,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs); void bdrv_ref(BlockDriverState *bs); void bdrv_unref(BlockDriverState *bs); -void bdrv_set_in_use(BlockDriverState *bs, int in_use); -int bdrv_in_use(BlockDriverState *bs); bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); diff --git a/include/block/block_int.h b/include/block/block_int.h index fdf2a7da8f..7b1c013c30 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -358,7 +358,6 @@ struct BlockDriverState { QTAILQ_ENTRY(BlockDriverState) device_list; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; int refcnt; - int in_use; /* users other than guest access, eg. block migration */ QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d76de62a46..c0a787530b 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -106,6 +106,9 @@ struct BlockJob { /** The completion function that will be called when the job completes. */ BlockDriverCompletionFunc *cb; + /** Block other operations when block job is running */ + Error *blocker; + /** The opaque value that is passed to the completion function. */ void *opaque; }; From 628ff683034c83ce54a1ae91d898d44e34f4851a Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:44 +0800 Subject: [PATCH 09/33] block: Move op_blocker check from block_job_create to its caller It makes no sense to check for "any" blocker on bs, we are here only because of the mechanical conversion from in_use to op_blockers. Remove it now, and let the callers check specific operation types. Backup and mirror already have it, add checker to stream and commit. Signed-off-by: Fam Zheng Reviewed-by: Benoit Canet Reviewed-by: Jeff Cody Reviewed-by: Stefan Hajnoczi Signed-off-by: Stefan Hajnoczi --- blockdev.c | 8 ++++++++ blockjob.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 0a519029b8..9a9bdec7da 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1889,6 +1889,10 @@ void qmp_block_stream(const char *device, bool has_base, return; } + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { + return; + } + if (base) { base_bs = bdrv_find_backing_image(bs, base); if (base_bs == NULL) { @@ -1933,6 +1937,10 @@ void qmp_block_commit(const char *device, return; } + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { + return; + } + /* default top_bs is the active layer */ top_bs = bs; diff --git a/blockjob.c b/blockjob.c index 60e72f5d90..7d84ca1d6c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -41,7 +41,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, { BlockJob *job; - if (bs->job || !bdrv_op_blocker_is_empty(bs)) { + if (bs->job) { error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)); return NULL; } From 8d24cce1e325c4bd47a8b1984d6db599f5a7a8e9 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:45 +0800 Subject: [PATCH 10/33] block: Add bdrv_set_backing_hd() This is the common but non-trivial steps to assign or change the backing_hd of BDS. Signed-off-by: Fam Zheng Reviewed-by: Stefan Hajnoczi Reviewed-by: Jeff Cody Signed-off-by: Stefan Hajnoczi --- block.c | 36 +++++++++++++++++++++++------------- include/block/block.h | 1 + 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/block.c b/block.c index 64c48a54a1..911ba6816e 100644 --- a/block.c +++ b/block.c @@ -1094,6 +1094,21 @@ fail: return ret; } +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) +{ + + bs->backing_hd = backing_hd; + if (!backing_hd) { + goto out; + } + bs->open_flags &= ~BDRV_O_NO_BACKING; + pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); + pstrcpy(bs->backing_format, sizeof(bs->backing_format), + backing_hd->drv ? backing_hd->drv->format_name : ""); +out: + bdrv_refresh_limits(bs); +} + /* * Opens the backing file for a BlockDriverState if not yet open * @@ -1107,6 +1122,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) char *backing_filename = g_malloc0(PATH_MAX); int ret = 0; BlockDriver *back_drv = NULL; + BlockDriverState *backing_hd; Error *local_err = NULL; if (bs->backing_hd != NULL) { @@ -1129,27 +1145,26 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX); } + backing_hd = bdrv_new("", errp); + if (bs->backing_format[0] != '\0') { back_drv = bdrv_find_format(bs->backing_format); } assert(bs->backing_hd == NULL); - ret = bdrv_open(&bs->backing_hd, + ret = bdrv_open(&backing_hd, *backing_filename ? backing_filename : NULL, NULL, options, bdrv_backing_flags(bs->open_flags), back_drv, &local_err); if (ret < 0) { - bs->backing_hd = NULL; + bdrv_unref(backing_hd); + backing_hd = NULL; bs->open_flags |= BDRV_O_NO_BACKING; error_setg(errp, "Could not open backing file: %s", error_get_pretty(local_err)); error_free(local_err); goto free_exit; } - - if (bs->backing_hd->file) { - pstrcpy(bs->backing_file, sizeof(bs->backing_file), - bs->backing_hd->file->filename); - } + bdrv_set_backing_hd(bs, backing_hd); /* Recalculate the BlockLimits with the backing file */ bdrv_refresh_limits(bs); @@ -2043,12 +2058,7 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) /* The contents of 'tmp' will become bs_top, as we are * swapping bs_new and bs_top contents. */ - bs_top->backing_hd = bs_new; - bs_top->open_flags &= ~BDRV_O_NO_BACKING; - pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file), - bs_new->filename); - pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format), - bs_new->drv ? bs_new->drv->format_name : ""); + bdrv_set_backing_hd(bs_top, bs_new); } static void bdrv_delete(BlockDriverState *bs) diff --git a/include/block/block.h b/include/block/block.h index 22935f1cbd..faee3aa246 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -216,6 +216,7 @@ 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 allow_none, Error **errp); +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); int bdrv_open(BlockDriverState **pbs, const char *filename, From 920beae1037dc7d98cd876da8c83b402234c7dba Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:46 +0800 Subject: [PATCH 11/33] block: Use bdrv_set_backing_hd everywhere We need to handle the coming backing_blocker properly, so don't open code the assignment, instead, call bdrv_set_backing_hd to change backing_hd. Signed-off-by: Fam Zheng Reviewed-by: Jeff Cody Signed-off-by: Stefan Hajnoczi --- block.c | 6 ++---- block/stream.c | 4 ++-- block/vvfat.c | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/block.c b/block.c index 911ba6816e..1271dd2acd 100644 --- a/block.c +++ b/block.c @@ -2652,13 +2652,11 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, if (ret) { goto exit; } - new_top_bs->backing_hd = base_bs; - - bdrv_refresh_limits(new_top_bs); + bdrv_set_backing_hd(new_top_bs, base_bs); QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { /* so that bdrv_close() does not recursively close the chain */ - intermediate_state->bs->backing_hd = NULL; + bdrv_set_backing_hd(intermediate_state->bs, NULL); bdrv_unref(intermediate_state->bs); } ret = 0; diff --git a/block/stream.c b/block/stream.c index dd0b4ac3d2..91d18a2db7 100644 --- a/block/stream.c +++ b/block/stream.c @@ -60,7 +60,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base, /* Must assign before bdrv_delete() to prevent traversing dangling pointer * while we delete backing image instances. */ - top->backing_hd = base; + bdrv_set_backing_hd(top, base); while (intermediate) { BlockDriverState *unused; @@ -72,7 +72,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base, unused = intermediate; intermediate = intermediate->backing_hd; - unused->backing_hd = NULL; + bdrv_set_backing_hd(unused, NULL); bdrv_unref(unused); } diff --git a/block/vvfat.c b/block/vvfat.c index c3af7ff4c5..417e96fe9b 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2947,7 +2947,7 @@ static int enable_write_target(BDRVVVFATState *s) unlink(s->qcow_filename); #endif - s->bs->backing_hd = bdrv_new("", &error_abort); + bdrv_set_backing_hd(s->bs, bdrv_new("", &error_abort)); s->bs->backing_hd->drv = &vvfat_write_target; s->bs->backing_hd->opaque = g_malloc(sizeof(void*)); *(void**)s->bs->backing_hd->opaque = s; From 826b6ca0b0c00bf27562a85bc073f800dad1259b Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:47 +0800 Subject: [PATCH 12/33] block: Add backing_blocker in BlockDriverState This makes use of op_blocker and blocks all the operations except for commit target, on each BlockDriverState->backing_hd. The asserts for op_blocker in bdrv_swap are removed because with this change, the target of block commit has at least the backing blocker of its child, so the assertion is not true. Callers should do their check. Signed-off-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- block.c | 23 +++++++++++++++++++---- block/mirror.c | 2 +- include/block/block_int.h | 3 +++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index 1271dd2acd..aa9b5abf5d 100644 --- a/block.c +++ b/block.c @@ -1097,14 +1097,30 @@ fail: void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) { + if (bs->backing_hd) { + assert(bs->backing_blocker); + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); + } else if (backing_hd) { + error_setg(&bs->backing_blocker, + "device is used as backing hd of '%s'", + bs->device_name); + } + bs->backing_hd = backing_hd; if (!backing_hd) { + error_free(bs->backing_blocker); + bs->backing_blocker = NULL; goto out; } bs->open_flags &= ~BDRV_O_NO_BACKING; pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_hd->drv ? backing_hd->drv->format_name : ""); + + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); + /* Otherwise we won't be able to commit due to check in bdrv_commit */ + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, + bs->backing_blocker); out: bdrv_refresh_limits(bs); } @@ -1803,8 +1819,9 @@ void bdrv_close(BlockDriverState *bs) if (bs->drv) { if (bs->backing_hd) { - bdrv_unref(bs->backing_hd); - bs->backing_hd = NULL; + BlockDriverState *backing_hd = bs->backing_hd; + bdrv_set_backing_hd(bs, NULL); + bdrv_unref(backing_hd); } bs->drv->bdrv_close(bs); g_free(bs->opaque); @@ -2006,7 +2023,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); assert(bs_new->job == NULL); assert(bs_new->dev == NULL); - assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); @@ -2025,7 +2041,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) /* Check a few fields that should remain attached to the device */ assert(bs_new->dev == NULL); assert(bs_new->job == NULL); - assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); diff --git a/block/mirror.c b/block/mirror.c index 1c38aa8f77..94c8661777 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -498,7 +498,7 @@ immediate_exit: /* drop the bs loop chain formed by the swap: break the loop then * trigger the unref from the top one */ BlockDriverState *p = s->base->backing_hd; - s->base->backing_hd = NULL; + bdrv_set_backing_hd(s->base, NULL); bdrv_unref(p); } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 7b1c013c30..f2e753f632 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -369,6 +369,9 @@ struct BlockDriverState { QDict *options; BlockdevDetectZeroesOptions detect_zeroes; + + /* The error object in use for blocking operations on backing_hd */ + Error *backing_blocker; }; int get_tmp_filename(char *filename, int size); From ce782938b86087bed21023ad6cfcd76a51d04f9e Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 23 May 2014 21:29:48 +0800 Subject: [PATCH 13/33] block: Drop redundant bdrv_refresh_limits The above bdrv_set_backing_hd already does this. Signed-off-by: Fam Zheng Reviewed-by: Jeff Cody Signed-off-by: Stefan Hajnoczi --- block.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/block.c b/block.c index aa9b5abf5d..a517d72d71 100644 --- a/block.c +++ b/block.c @@ -1182,9 +1182,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) } bdrv_set_backing_hd(bs, backing_hd); - /* Recalculate the BlockLimits with the backing file */ - bdrv_refresh_limits(bs); - free_exit: g_free(backing_filename); return ret; From 6815bce5421ddb4987cd98d0146baf65200eabcd Mon Sep 17 00:00:00 2001 From: Maria Kustova Date: Fri, 23 May 2014 17:41:29 +0400 Subject: [PATCH 14/33] docs: Define refcount_bits value The 'refcount_bits' term used in the description of refcount block entry is not defined in the specification. The definition is added in the 'refcount_order' section where refcount_bits was used as 'width in bits'. Signed-off-by: Maria Kustova Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- docs/specs/qcow2.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt index f19536a46f..3f713a6447 100644 --- a/docs/specs/qcow2.txt +++ b/docs/specs/qcow2.txt @@ -107,8 +107,9 @@ in the description of a field. 96 - 99: refcount_order Describes the width of a reference count block entry (width - in bits = 1 << refcount_order). For version 2 images, the - order is always assumed to be 4 (i.e. the width is 16 bits). + in bits: refcount_bits = 1 << refcount_order). For version 2 + images, the order is always assumed to be 4 + (i.e. refcount_bits = 16). 100 - 103: header_length Length of the header structure in bytes. For version 2 From e8817e7b0e0c355690d34bb499d50373137d240f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:08 +0200 Subject: [PATCH 15/33] blockdev: Don't use qerror_report_err() in drive_init() qerror_report_err() is a transitional interface to help with converting existing HMP commands to QMP. It should not be used elsewhere. drive_init() is not meant to be used by QMP commands. It uses both qerror_report_err() and error_report(). Convert the former to the latter. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- blockdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index 9a9bdec7da..264b07ca4e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -730,7 +730,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) &error_abort); qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err); if (local_err) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); goto fail; } @@ -942,7 +942,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) dinfo = blockdev_init(filename, bs_opts, &local_err); if (dinfo == NULL) { if (local_err) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); } goto fail; From b1422f20400e9dccd4388a4d107df5b67dba78d8 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:09 +0200 Subject: [PATCH 16/33] blockdev: Don't use qerror_report() in do_drive_del() qerror_report() is a transitional interface to help with converting existing HMP commands to QMP. It should not be used elsewhere. do_drive_del() is an HMP command that won't be converted to QMP (we'll create a new QMP command instead). It uses both qerror_report() and error_report(). Convert the former to the latter. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- blockdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index 264b07ca4e..8cc42fb1d0 100644 --- a/blockdev.c +++ b/blockdev.c @@ -34,7 +34,6 @@ #include "hw/block/block.h" #include "block/blockjob.h" #include "monitor/monitor.h" -#include "qapi/qmp/qerror.h" #include "qemu/option.h" #include "qemu/config-file.h" #include "qapi/qmp/types.h" @@ -1763,7 +1762,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) bs = bdrv_find(id); if (!bs) { - qerror_report(QERR_DEVICE_NOT_FOUND, id); + error_report("Device '%s' not found", id); return -1; } if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) { From 3775ec6f5a15da154bd8fbda284d34a3be580ee6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:10 +0200 Subject: [PATCH 17/33] qemu-nbd: Don't use qerror_report() qerror_report() is a transitional interface to help with converting existing HMP commands to QMP. It should not be used elsewhere. Replace by error_report(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- qemu-nbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index cd6bd50fae..ba6043680a 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -223,7 +223,7 @@ static int tcp_socket_incoming(const char *address, uint16_t 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_report("%s", error_get_pretty(local_err)); error_free(local_err); } return fd; @@ -235,7 +235,7 @@ static int unix_socket_incoming(const char *path) int fd = unix_listen(path, NULL, 0, &local_err); if (local_err != NULL) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); } return fd; @@ -247,7 +247,7 @@ static int unix_socket_outgoing(const char *path) int fd = unix_connect(path, &local_err); if (local_err != NULL) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); } return fd; From d61563b2356eba9643ee711d900f0ad3c1f1f298 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:11 +0200 Subject: [PATCH 18/33] block/rbd: Propagate errors to open and create methods Completes the conversion to Error started in commit 015a103^..d5124c0. Cc: Josh Durgin Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/rbd.c | 71 +++++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index dbc79f4525..09af48426e 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -105,7 +105,7 @@ typedef struct BDRVRBDState { static int qemu_rbd_next_tok(char *dst, int dst_len, char *src, char delim, const char *name, - char **p) + char **p, Error **errp) { int l; char *end; @@ -128,10 +128,10 @@ static int qemu_rbd_next_tok(char *dst, int dst_len, } l = strlen(src); if (l >= dst_len) { - error_report("%s too long", name); + error_setg(errp, "%s too long", name); return -EINVAL; } else if (l == 0) { - error_report("%s too short", name); + error_setg(errp, "%s too short", name); return -EINVAL; } @@ -157,13 +157,15 @@ static int qemu_rbd_parsename(const char *filename, char *pool, int pool_len, char *snap, int snap_len, char *name, int name_len, - char *conf, int conf_len) + char *conf, int conf_len, + Error **errp) { const char *start; char *p, *buf; int ret; if (!strstart(filename, "rbd:", &start)) { + error_setg(errp, "File name must start with 'rbd:'"); return -EINVAL; } @@ -172,7 +174,8 @@ static int qemu_rbd_parsename(const char *filename, *snap = '\0'; *conf = '\0'; - ret = qemu_rbd_next_tok(pool, pool_len, p, '/', "pool name", &p); + ret = qemu_rbd_next_tok(pool, pool_len, p, + '/', "pool name", &p, errp); if (ret < 0 || !p) { ret = -EINVAL; goto done; @@ -180,21 +183,25 @@ static int qemu_rbd_parsename(const char *filename, qemu_rbd_unescape(pool); if (strchr(p, '@')) { - ret = qemu_rbd_next_tok(name, name_len, p, '@', "object name", &p); + ret = qemu_rbd_next_tok(name, name_len, p, + '@', "object name", &p, errp); if (ret < 0) { goto done; } - ret = qemu_rbd_next_tok(snap, snap_len, p, ':', "snap name", &p); + ret = qemu_rbd_next_tok(snap, snap_len, p, + ':', "snap name", &p, errp); qemu_rbd_unescape(snap); } else { - ret = qemu_rbd_next_tok(name, name_len, p, ':', "object name", &p); + ret = qemu_rbd_next_tok(name, name_len, p, + ':', "object name", &p, errp); } qemu_rbd_unescape(name); if (ret < 0 || !p) { goto done; } - ret = qemu_rbd_next_tok(conf, conf_len, p, '\0', "configuration", &p); + ret = qemu_rbd_next_tok(conf, conf_len, p, + '\0', "configuration", &p, errp); done: g_free(buf); @@ -229,7 +236,7 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname) return NULL; } -static int qemu_rbd_set_conf(rados_t cluster, const char *conf) +static int qemu_rbd_set_conf(rados_t cluster, const char *conf, Error **errp) { char *p, *buf; char name[RBD_MAX_CONF_NAME_SIZE]; @@ -241,20 +248,20 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf) while (p) { ret = qemu_rbd_next_tok(name, sizeof(name), p, - '=', "conf option name", &p); + '=', "conf option name", &p, errp); if (ret < 0) { break; } qemu_rbd_unescape(name); if (!p) { - error_report("conf option %s has no value", name); + error_setg(errp, "conf option %s has no value", name); ret = -EINVAL; break; } ret = qemu_rbd_next_tok(value, sizeof(value), p, - ':', "conf option value", &p); + ':', "conf option value", &p, errp); if (ret < 0) { break; } @@ -263,7 +270,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf) if (strcmp(name, "conf") == 0) { ret = rados_conf_read_file(cluster, value); if (ret < 0) { - error_report("error reading conf file %s", value); + error_setg(errp, "error reading conf file %s", value); break; } } else if (strcmp(name, "id") == 0) { @@ -271,7 +278,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf) } else { ret = rados_conf_set(cluster, name, value); if (ret < 0) { - error_report("invalid conf option %s", name); + error_setg(errp, "invalid conf option %s", name); ret = -EINVAL; break; } @@ -285,6 +292,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf) static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, Error **errp) { + Error *local_err = NULL; int64_t bytes = 0; int64_t objsize; int obj_order = 0; @@ -301,7 +309,8 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, if (qemu_rbd_parsename(filename, pool, sizeof(pool), snap_buf, sizeof(snap_buf), name, sizeof(name), - conf, sizeof(conf)) < 0) { + conf, sizeof(conf), &local_err) < 0) { + error_propagate(errp, local_err); return -EINVAL; } @@ -313,11 +322,11 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, if (options->value.n) { objsize = options->value.n; if ((objsize - 1) & objsize) { /* not a power of 2? */ - error_report("obj size needs to be power of 2"); + error_setg(errp, "obj size needs to be power of 2"); return -EINVAL; } if (objsize < 4096) { - error_report("obj size too small"); + error_setg(errp, "obj size too small"); return -EINVAL; } obj_order = ffs(objsize) - 1; @@ -328,7 +337,7 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, clientname = qemu_rbd_parse_clientname(conf, clientname_buf); if (rados_create(&cluster, clientname) < 0) { - error_report("error initializing"); + error_setg(errp, "error initializing"); return -EIO; } @@ -338,20 +347,20 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options, } if (conf[0] != '\0' && - qemu_rbd_set_conf(cluster, conf) < 0) { - error_report("error setting config options"); + qemu_rbd_set_conf(cluster, conf, &local_err) < 0) { rados_shutdown(cluster); + error_propagate(errp, local_err); return -EIO; } if (rados_connect(cluster) < 0) { - error_report("error connecting"); + error_setg(errp, "error connecting"); rados_shutdown(cluster); return -EIO; } if (rados_ioctx_create(cluster, pool, &io_ctx) < 0) { - error_report("error opening pool %s", pool); + error_setg(errp, "error opening pool %s", pool); rados_shutdown(cluster); return -EIO; } @@ -441,8 +450,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, 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); qemu_opts_del(opts); return -EINVAL; } @@ -452,7 +460,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, if (qemu_rbd_parsename(filename, pool, sizeof(pool), snap_buf, sizeof(snap_buf), s->name, sizeof(s->name), - conf, sizeof(conf)) < 0) { + conf, sizeof(conf), errp) < 0) { r = -EINVAL; goto failed_opts; } @@ -460,7 +468,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, clientname = qemu_rbd_parse_clientname(conf, clientname_buf); r = rados_create(&s->cluster, clientname); if (r < 0) { - error_report("error initializing"); + error_setg(&local_err, "error initializing"); goto failed_opts; } @@ -488,28 +496,27 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } if (conf[0] != '\0') { - r = qemu_rbd_set_conf(s->cluster, conf); + r = qemu_rbd_set_conf(s->cluster, conf, errp); if (r < 0) { - error_report("error setting config options"); goto failed_shutdown; } } r = rados_connect(s->cluster); if (r < 0) { - error_report("error connecting"); + error_setg(&local_err, "error connecting"); goto failed_shutdown; } r = rados_ioctx_create(s->cluster, pool, &s->io_ctx); if (r < 0) { - error_report("error opening pool %s", pool); + error_setg(&local_err, "error opening pool %s", pool); goto failed_shutdown; } r = rbd_open(s->io_ctx, s->name, &s->image, s->snap); if (r < 0) { - error_report("error reading header from %s", s->name); + error_setg(&local_err, "error reading header from %s", s->name); goto failed_open; } From 04bc7c0e38fc77afc116031f1e25af80374b1971 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:12 +0200 Subject: [PATCH 19/33] block/ssh: Drop superfluous libssh2_session_last_errno() calls libssh2_session_last_error() already returns the error code. Signed-off-by: Markus Armbruster Reviewed-by: Richard W.M. Jones Signed-off-by: Stefan Hajnoczi --- block/ssh.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index aa63c9d20e..e38d2322e9 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -121,10 +121,9 @@ session_error_report(BDRVSSHState *s, const char *fs, ...) char *ssh_err; int ssh_err_code; - libssh2_session_last_error((s)->session, &ssh_err, NULL, 0); /* This is not an errno. See . */ - ssh_err_code = libssh2_session_last_errno((s)->session); - + ssh_err_code = libssh2_session_last_error(s->session, + &ssh_err, NULL, 0); error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code); } @@ -145,9 +144,9 @@ sftp_error_report(BDRVSSHState *s, const char *fs, ...) int ssh_err_code; unsigned long sftp_err_code; - libssh2_session_last_error((s)->session, &ssh_err, NULL, 0); /* This is not an errno. See . */ - ssh_err_code = libssh2_session_last_errno((s)->session); + ssh_err_code = libssh2_session_last_error(s->session, + &ssh_err, NULL, 0); /* See . */ sftp_err_code = libssh2_sftp_last_error((s)->sftp); From 01c2b265fce921d6460e06f5af4dfb405119cbab Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:13 +0200 Subject: [PATCH 20/33] block/ssh: Propagate errors through check_host_key() Signed-off-by: Markus Armbruster Reviewed-by: Richard W.M. Jones Signed-off-by: Stefan Hajnoczi --- block/ssh.c | 68 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index e38d2322e9..1df1946d68 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -106,6 +106,31 @@ static void ssh_state_free(BDRVSSHState *s) } } +static void GCC_FMT_ATTR(3, 4) +session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...) +{ + va_list args; + char *msg; + + va_start(args, fs); + msg = g_strdup_vprintf(fs, args); + va_end(args); + + if (s->session) { + char *ssh_err; + int ssh_err_code; + + /* This is not an errno. See . */ + ssh_err_code = libssh2_session_last_error(s->session, + &ssh_err, NULL, 0); + error_setg(errp, "%s: %s (libssh2 error code: %d)", + msg, ssh_err, ssh_err_code); + } else { + error_setg(errp, "%s", msg); + } + g_free(msg); +} + /* Wrappers around error_report which make sure to dump as much * information from libssh2 as possible. */ @@ -242,7 +267,7 @@ static void ssh_parse_filename(const char *filename, QDict *options, } static int check_host_key_knownhosts(BDRVSSHState *s, - const char *host, int port) + const char *host, int port, Error **errp) { const char *home; char *knh_file = NULL; @@ -256,14 +281,15 @@ static int check_host_key_knownhosts(BDRVSSHState *s, hostkey = libssh2_session_hostkey(s->session, &len, &type); if (!hostkey) { ret = -EINVAL; - session_error_report(s, "failed to read remote host key"); + session_error_setg(errp, s, "failed to read remote host key"); goto out; } knh = libssh2_knownhost_init(s->session); if (!knh) { ret = -EINVAL; - session_error_report(s, "failed to initialize known hosts support"); + session_error_setg(errp, s, + "failed to initialize known hosts support"); goto out; } @@ -288,21 +314,23 @@ static int check_host_key_knownhosts(BDRVSSHState *s, break; case LIBSSH2_KNOWNHOST_CHECK_MISMATCH: ret = -EINVAL; - session_error_report(s, "host key does not match the one in known_hosts (found key %s)", - found->key); + session_error_setg(errp, s, + "host key does not match the one in known_hosts" + " (found key %s)", found->key); goto out; case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND: ret = -EINVAL; - session_error_report(s, "no host key was found in known_hosts"); + session_error_setg(errp, s, "no host key was found in known_hosts"); goto out; case LIBSSH2_KNOWNHOST_CHECK_FAILURE: ret = -EINVAL; - session_error_report(s, "failure matching the host key with known_hosts"); + session_error_setg(errp, s, + "failure matching the host key with known_hosts"); goto out; default: ret = -EINVAL; - session_error_report(s, "unknown error matching the host key with known_hosts (%d)", - r); + session_error_setg(errp, s, "unknown error matching the host key" + " with known_hosts (%d)", r); goto out; } @@ -357,20 +385,20 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len, static int check_host_key_hash(BDRVSSHState *s, const char *hash, - int hash_type, size_t fingerprint_len) + int hash_type, size_t fingerprint_len, Error **errp) { const char *fingerprint; fingerprint = libssh2_hostkey_hash(s->session, hash_type); if (!fingerprint) { - session_error_report(s, "failed to read remote host key"); + session_error_setg(errp, s, "failed to read remote host key"); return -EINVAL; } if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len, hash) != 0) { - error_report("remote host key does not match host_key_check '%s'", - hash); + error_setg(errp, "remote host key does not match host_key_check '%s'", + hash); return -EPERM; } @@ -378,7 +406,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash, } static int check_host_key(BDRVSSHState *s, const char *host, int port, - const char *host_key_check) + const char *host_key_check, Error **errp) { /* host_key_check=no */ if (strcmp(host_key_check, "no") == 0) { @@ -388,21 +416,21 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port, /* host_key_check=md5:xx:yy:zz:... */ if (strncmp(host_key_check, "md5:", 4) == 0) { return check_host_key_hash(s, &host_key_check[4], - LIBSSH2_HOSTKEY_HASH_MD5, 16); + LIBSSH2_HOSTKEY_HASH_MD5, 16, errp); } /* host_key_check=sha1:xx:yy:zz:... */ if (strncmp(host_key_check, "sha1:", 5) == 0) { return check_host_key_hash(s, &host_key_check[5], - LIBSSH2_HOSTKEY_HASH_SHA1, 20); + LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp); } /* host_key_check=yes */ if (strcmp(host_key_check, "yes") == 0) { - return check_host_key_knownhosts(s, host, port); + return check_host_key_knownhosts(s, host, port, errp); } - error_report("unknown host_key_check setting (%s)", host_key_check); + error_setg(errp, "unknown host_key_check setting (%s)", host_key_check); return -EINVAL; } @@ -541,8 +569,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } /* Check the remote host's key against known_hosts. */ - ret = check_host_key(s, host, port, host_key_check); + ret = check_host_key(s, host, port, host_key_check, &err); if (ret < 0) { + qerror_report_err(err); + error_free(err); goto err; } From 4618e658e6dadd1ba53585157984eac71cb706c6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:14 +0200 Subject: [PATCH 21/33] block/ssh: Propagate errors through authenticate() Signed-off-by: Markus Armbruster Reviewed-by: Richard W.M. Jones Signed-off-by: Stefan Hajnoczi --- block/ssh.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 1df1946d68..18186ba81f 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -434,7 +434,7 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port, return -EINVAL; } -static int authenticate(BDRVSSHState *s, const char *user) +static int authenticate(BDRVSSHState *s, const char *user, Error **errp) { int r, ret; const char *userauthlist; @@ -445,7 +445,8 @@ static int authenticate(BDRVSSHState *s, const char *user) userauthlist = libssh2_userauth_list(s->session, user, strlen(user)); if (strstr(userauthlist, "publickey") == NULL) { ret = -EPERM; - error_report("remote server does not support \"publickey\" authentication"); + error_setg(errp, + "remote server does not support \"publickey\" authentication"); goto out; } @@ -453,17 +454,18 @@ static int authenticate(BDRVSSHState *s, const char *user) agent = libssh2_agent_init(s->session); if (!agent) { ret = -EINVAL; - session_error_report(s, "failed to initialize ssh-agent support"); + session_error_setg(errp, s, "failed to initialize ssh-agent support"); goto out; } if (libssh2_agent_connect(agent)) { ret = -ECONNREFUSED; - session_error_report(s, "failed to connect to ssh-agent"); + session_error_setg(errp, s, "failed to connect to ssh-agent"); goto out; } if (libssh2_agent_list_identities(agent)) { ret = -EINVAL; - session_error_report(s, "failed requesting identities from ssh-agent"); + session_error_setg(errp, s, + "failed requesting identities from ssh-agent"); goto out; } @@ -474,7 +476,8 @@ static int authenticate(BDRVSSHState *s, const char *user) } if (r < 0) { ret = -EINVAL; - session_error_report(s, "failed to obtain identity from ssh-agent"); + session_error_setg(errp, s, + "failed to obtain identity from ssh-agent"); goto out; } r = libssh2_agent_userauth(agent, user, identity); @@ -488,8 +491,8 @@ static int authenticate(BDRVSSHState *s, const char *user) } ret = -EPERM; - error_report("failed to authenticate using publickey authentication " - "and the identities held by your ssh-agent"); + error_setg(errp, "failed to authenticate using publickey authentication " + "and the identities held by your ssh-agent"); out: if (agent != NULL) { @@ -577,8 +580,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } /* Authenticate. */ - ret = authenticate(s, user); + ret = authenticate(s, user, &err); if (ret < 0) { + qerror_report_err(err); + error_free(err); goto err; } From 5f0c39e59822fdfd6a730824eded06209942e495 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:15 +0200 Subject: [PATCH 22/33] block/ssh: Propagate errors through connect_to_ssh() Signed-off-by: Markus Armbruster Reviewed-by: Richard W.M. Jones Signed-off-by: Stefan Hajnoczi --- block/ssh.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 18186ba81f..26078c41ca 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -506,10 +506,9 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp) } static int connect_to_ssh(BDRVSSHState *s, QDict *options, - int ssh_flags, int creat_mode) + int ssh_flags, int creat_mode, Error **errp) { int r, ret; - Error *err = NULL; const char *host, *user, *path, *host_key_check; int port; @@ -528,6 +527,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } else { user = g_get_user_name(); if (!user) { + error_setg_errno(errp, errno, "Can't get user name"); ret = -errno; goto err; } @@ -544,11 +544,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, s->hostport = g_strdup_printf("%s:%d", host, port); /* Open the socket and connect. */ - s->sock = inet_connect(s->hostport, &err); - if (err != NULL) { + s->sock = inet_connect(s->hostport, errp); + if (s->sock < 0) { ret = -errno; - qerror_report_err(err); - error_free(err); goto err; } @@ -556,7 +554,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, s->session = libssh2_session_init(); if (!s->session) { ret = -EINVAL; - session_error_report(s, "failed to initialize libssh2 session"); + session_error_setg(errp, s, "failed to initialize libssh2 session"); goto err; } @@ -567,30 +565,26 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, r = libssh2_session_handshake(s->session, s->sock); if (r != 0) { ret = -EINVAL; - session_error_report(s, "failed to establish SSH session"); + session_error_setg(errp, s, "failed to establish SSH session"); goto err; } /* Check the remote host's key against known_hosts. */ - ret = check_host_key(s, host, port, host_key_check, &err); + ret = check_host_key(s, host, port, host_key_check, errp); if (ret < 0) { - qerror_report_err(err); - error_free(err); goto err; } /* Authenticate. */ - ret = authenticate(s, user, &err); + ret = authenticate(s, user, errp); if (ret < 0) { - qerror_report_err(err); - error_free(err); goto err; } /* Start SFTP. */ s->sftp = libssh2_sftp_init(s->session); if (!s->sftp) { - session_error_report(s, "failed to initialize sftp handle"); + session_error_setg(errp, s, "failed to initialize sftp handle"); ret = -EINVAL; goto err; } @@ -645,6 +639,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, Error **errp) { + Error *local_err = NULL; BDRVSSHState *s = bs->opaque; int ret; int ssh_flags; @@ -657,8 +652,10 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, } /* Start up SSH. */ - ret = connect_to_ssh(s, options, ssh_flags, 0); + ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err); if (ret < 0) { + qerror_report_err(local_err); + error_free(local_err); goto err; } @@ -718,8 +715,11 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options, r = connect_to_ssh(&s, uri_options, LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE| - LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, 0644); + LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, + 0644, &local_err); if (r < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = r; goto out; } From 5496fb1aebb624b379a503a8b2a156922a275fa1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:16 +0200 Subject: [PATCH 23/33] block/ssh: Propagate errors to open and create methods Completes the conversion to Error started in commit 015a103^..d5124c0. Signed-off-by: Markus Armbruster Reviewed-by: Richard W.M. Jones Signed-off-by: Stefan Hajnoczi --- block/ssh.c | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 26078c41ca..b2129714bc 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -131,29 +131,34 @@ session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...) g_free(msg); } -/* Wrappers around error_report which make sure to dump as much - * information from libssh2 as possible. - */ -static void GCC_FMT_ATTR(2, 3) -session_error_report(BDRVSSHState *s, const char *fs, ...) +static void GCC_FMT_ATTR(3, 4) +sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...) { va_list args; + char *msg; va_start(args, fs); - error_vprintf(fs, args); + msg = g_strdup_vprintf(fs, args); + va_end(args); - if ((s)->session) { + if (s->sftp) { char *ssh_err; int ssh_err_code; + unsigned long sftp_err_code; /* This is not an errno. See . */ ssh_err_code = libssh2_session_last_error(s->session, &ssh_err, NULL, 0); - error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code); - } + /* See . */ + sftp_err_code = libssh2_sftp_last_error((s)->sftp); - va_end(args); - error_printf("\n"); + error_setg(errp, + "%s: %s (libssh2 error code: %d, sftp error code: %lu)", + msg, ssh_err, ssh_err_code, sftp_err_code); + } else { + error_setg(errp, "%s", msg); + } + g_free(msg); } static void GCC_FMT_ATTR(2, 3) @@ -594,14 +599,14 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, path, ssh_flags, creat_mode); s->sftp_handle = libssh2_sftp_open(s->sftp, path, ssh_flags, creat_mode); if (!s->sftp_handle) { - session_error_report(s, "failed to open remote file '%s'", path); + session_error_setg(errp, s, "failed to open remote file '%s'", path); ret = -EINVAL; goto err; } r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs); if (r < 0) { - sftp_error_report(s, "failed to read file attributes"); + sftp_error_setg(errp, s, "failed to read file attributes"); return -EINVAL; } @@ -639,7 +644,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, Error **errp) { - Error *local_err = NULL; BDRVSSHState *s = bs->opaque; int ret; int ssh_flags; @@ -652,10 +656,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, } /* Start up SSH. */ - ret = connect_to_ssh(s, options, ssh_flags, 0, &local_err); + ret = connect_to_ssh(s, options, ssh_flags, 0, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); goto err; } @@ -686,7 +688,6 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options, Error **errp) { int r, ret; - Error *local_err = NULL; int64_t total_size = 0; QDict *uri_options = NULL; BDRVSSHState s; @@ -705,10 +706,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options, DPRINTF("total_size=%" PRIi64, total_size); uri_options = qdict_new(); - r = parse_uri(filename, uri_options, &local_err); + r = parse_uri(filename, uri_options, errp); if (r < 0) { - qerror_report_err(local_err); - error_free(local_err); ret = r; goto out; } @@ -716,10 +715,8 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options, r = connect_to_ssh(&s, uri_options, LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE| LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, - 0644, &local_err); + 0644, errp); if (r < 0) { - qerror_report_err(local_err); - error_free(local_err); ret = r; goto out; } @@ -728,7 +725,7 @@ static int ssh_create(const char *filename, QEMUOptionParameter *options, libssh2_sftp_seek64(s.sftp_handle, total_size-1); r2 = libssh2_sftp_write(s.sftp_handle, c, 1); if (r2 < 0) { - sftp_error_report(&s, "truncate failed"); + sftp_error_setg(errp, &s, "truncate failed"); ret = -EINVAL; goto out; } From 68c70af16d75fc2f4d111536c7cf81cdd63851fa Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:17 +0200 Subject: [PATCH 24/33] block/vvfat: Propagate errors through enable_write_target() Continues the conversion of the open method to Error started in commit 015a103. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/vvfat.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/block/vvfat.c b/block/vvfat.c index 417e96fe9b..9b9b8f1037 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -979,7 +979,7 @@ static int init_directories(BDRVVVFATState* s, static BDRVVVFATState *vvv = NULL; #endif -static int enable_write_target(BDRVVVFATState *s); +static int enable_write_target(BDRVVVFATState *s, Error **errp); static int is_consistent(BDRVVVFATState *s); static void vvfat_rebind(BlockDriverState *bs) @@ -1160,7 +1160,7 @@ DLOG(if (stderr == NULL) { s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); if (qemu_opt_get_bool(opts, "rw", false)) { - ret = enable_write_target(s); + ret = enable_write_target(s, errp); if (ret < 0) { goto fail; } @@ -2904,11 +2904,10 @@ static BlockDriver vvfat_write_target = { .bdrv_close = write_target_close, }; -static int enable_write_target(BDRVVVFATState *s) +static int enable_write_target(BDRVVVFATState *s, Error **errp) { BlockDriver *bdrv_qcow; QEMUOptionParameter *options; - Error *local_err = NULL; int ret; int size = sector2cluster(s, s->sector_count); s->used_clusters = calloc(size, 1); @@ -2918,6 +2917,7 @@ static int enable_write_target(BDRVVVFATState *s) s->qcow_filename = g_malloc(1024); ret = get_tmp_filename(s->qcow_filename, 1024); if (ret < 0) { + error_setg_errno(errp, -ret, "can't create temporary file"); goto err; } @@ -2926,20 +2926,16 @@ static int enable_write_target(BDRVVVFATState *s) set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512); set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:"); - ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err); + ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); goto err; } s->qcow = 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); + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, + bdrv_qcow, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); goto err; } From d11c8917b2b286dbea93c7b05b2f368831e50724 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:18 +0200 Subject: [PATCH 25/33] block/vvfat: Propagate errors through init_directories() Completes the conversion of the open method to Error started in commit 015a103. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/vvfat.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/block/vvfat.c b/block/vvfat.c index 9b9b8f1037..8f5114bd16 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -831,7 +831,8 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num) } static int init_directories(BDRVVVFATState* s, - const char *dirname, int heads, int secs) + const char *dirname, int heads, int secs, + Error **errp) { bootsector_t* bootsector; mapping_t* mapping; @@ -892,8 +893,8 @@ static int init_directories(BDRVVVFATState* s, if (mapping->mode & MODE_DIRECTORY) { mapping->begin = cluster; if(read_directory(s, i)) { - fprintf(stderr, "Could not read directory %s\n", - mapping->path); + error_setg(errp, "Could not read directory %s", + mapping->path); return -1; } mapping = array_get(&(s->mapping), i); @@ -919,9 +920,10 @@ static int init_directories(BDRVVVFATState* s, cluster = mapping->end; if(cluster > s->cluster_count) { - fprintf(stderr,"Directory does not fit in FAT%d (capacity %.2f MB)\n", - s->fat_type, s->sector_count / 2000.0); - return -EINVAL; + error_setg(errp, + "Directory does not fit in FAT%d (capacity %.2f MB)", + s->fat_type, s->sector_count / 2000.0); + return -1; } /* fix fat for entry */ @@ -1169,7 +1171,7 @@ DLOG(if (stderr == NULL) { bs->total_sectors = cyls * heads * secs; - if (init_directories(s, dirname, heads, secs)) { + if (init_directories(s, dirname, heads, secs, errp)) { ret = -EIO; goto fail; } From dfb12bf86eeb6cde388582ba5286b4fdcb0cfa4c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:19 +0200 Subject: [PATCH 26/33] block/sheepdog: Propagate errors through connect_to_sdog() Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 77 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 22 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 2c3fb016a8..ff0aa896b6 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -526,17 +526,16 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov, return acb; } -static int connect_to_sdog(BDRVSheepdogState *s) +static int connect_to_sdog(BDRVSheepdogState *s, Error **errp) { int fd; - Error *err = NULL; if (s->is_unix) { - fd = unix_connect(s->host_spec, &err); + fd = unix_connect(s->host_spec, errp); } else { - fd = inet_connect(s->host_spec, &err); + fd = inet_connect(s->host_spec, errp); - if (err == NULL) { + if (fd >= 0) { int ret = socket_set_nodelay(fd); if (ret < 0) { error_report("%s", strerror(errno)); @@ -544,10 +543,7 @@ static int connect_to_sdog(BDRVSheepdogState *s) } } - if (err != NULL) { - qerror_report_err(err); - error_free(err); - } else { + if (fd >= 0) { qemu_set_nonblock(fd); } @@ -916,10 +912,13 @@ static void co_write_request(void *opaque) */ static int get_sheep_fd(BDRVSheepdogState *s) { + Error *local_err = NULL; int fd; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return fd; } @@ -1063,14 +1062,17 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, uint32_t snapid, const char *tag, uint32_t *vid, bool lock) { + Error *local_err = NULL; int ret, fd; SheepdogVdiReq hdr; SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; unsigned int wlen, rlen = 0; char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN]; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return fd; } @@ -1263,12 +1265,15 @@ static int write_object(int fd, char *buf, uint64_t oid, uint8_t copies, /* update inode with the latest state */ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag) { + Error *local_err = NULL; SheepdogInode *inode; int ret = 0, fd; uint32_t vid = 0; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return -EIO; } @@ -1436,8 +1441,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, s->is_snapshot = true; } - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = fd; goto out; } @@ -1474,14 +1481,17 @@ out: static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot) { + Error *local_err = NULL; SheepdogVdiReq hdr; SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; int fd, ret; unsigned int wlen, rlen = 0; char buf[SD_MAX_VDI_LEN]; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return fd; } @@ -1730,6 +1740,7 @@ out: static void sd_close(BlockDriverState *bs) { + Error *local_err = NULL; BDRVSheepdogState *s = bs->opaque; SheepdogVdiReq hdr; SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; @@ -1738,8 +1749,10 @@ static void sd_close(BlockDriverState *bs) DPRINTF("%s\n", s->name); - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return; } @@ -1774,6 +1787,7 @@ static int64_t sd_getlength(BlockDriverState *bs) static int sd_truncate(BlockDriverState *bs, int64_t offset) { + Error *local_err = NULL; BDRVSheepdogState *s = bs->opaque; int ret, fd; unsigned int datalen; @@ -1786,8 +1800,10 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset) return -EINVAL; } - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return fd; } @@ -1846,6 +1862,7 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb) /* Delete current working VDI on the snapshot chain */ static bool sd_delete(BDRVSheepdogState *s) { + Error *local_err = NULL; unsigned int wlen = SD_MAX_VDI_LEN, rlen = 0; SheepdogVdiReq hdr = { .opcode = SD_OP_DEL_VDI, @@ -1856,8 +1873,10 @@ static bool sd_delete(BDRVSheepdogState *s) SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; int fd, ret; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return false; } @@ -1885,6 +1904,7 @@ static bool sd_delete(BDRVSheepdogState *s) */ static int sd_create_branch(BDRVSheepdogState *s) { + Error *local_err = NULL; int ret, fd; uint32_t vid; char *buf; @@ -1907,8 +1927,10 @@ static int sd_create_branch(BDRVSheepdogState *s) DPRINTF("%" PRIx32 " is created.\n", vid); - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = fd; goto out; } @@ -2122,6 +2144,7 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs) static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) { + Error *local_err = NULL; BDRVSheepdogState *s = bs->opaque; int ret, fd; uint32_t new_vid; @@ -2151,8 +2174,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id); /* refresh inode. */ - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = fd; goto cleanup; } @@ -2249,6 +2274,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) { + Error *local_err = NULL; BDRVSheepdogState *s = bs->opaque; SheepdogReq req; int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long); @@ -2263,8 +2289,10 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) vdi_inuse = g_malloc(max); - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = fd; goto out; } @@ -2290,8 +2318,10 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) hval = fnv_64a_buf(s->name, strlen(s->name), FNV1A_64_INIT); start_nr = hval & (SD_NR_VDIS - 1); - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = fd; goto out; } @@ -2341,6 +2371,7 @@ out: static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, int64_t pos, int size, int load) { + Error *local_err = NULL; bool create; int fd, ret = 0, remaining = size; unsigned int data_len; @@ -2349,8 +2380,10 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, uint32_t vdi_index; uint32_t vdi_id = load ? s->inode.parent_vdi_id : s->inode.vdi_id; - fd = connect_to_sdog(s); + fd = connect_to_sdog(s, &local_err); if (fd < 0) { + qerror_report_err(local_err); + error_free(local_err); return fd; } From 356b4ca2bb8fe2e907a1dbdd7ed0a1a7f11c2645 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:20 +0200 Subject: [PATCH 27/33] block/sheepdog: Propagate errors through get_sheep_fd() Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index ff0aa896b6..b932d6816a 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -668,7 +668,7 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req, enum AIOCBState aiocb_type); static void coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req); static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag); -static int get_sheep_fd(BDRVSheepdogState *s); +static int get_sheep_fd(BDRVSheepdogState *s, Error **errp); static void co_write_request(void *opaque); static AIOReq *find_pending_req(BDRVSheepdogState *s, uint64_t oid) @@ -705,6 +705,7 @@ static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid) static coroutine_fn void reconnect_to_sdog(void *opaque) { + Error *local_err = NULL; BDRVSheepdogState *s = opaque; AIOReq *aio_req, *next; @@ -719,9 +720,11 @@ static coroutine_fn void reconnect_to_sdog(void *opaque) /* Try to reconnect the sheepdog server every one second. */ while (s->fd < 0) { - s->fd = get_sheep_fd(s); + s->fd = get_sheep_fd(s, &local_err); if (s->fd < 0) { DPRINTF("Wait for connection to be established\n"); + qerror_report_err(local_err); + error_free(local_err); co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME, 1000000000ULL); } @@ -910,15 +913,12 @@ static void co_write_request(void *opaque) * We cannot use this descriptor for other operations because * the block driver may be on waiting response from the server. */ -static int get_sheep_fd(BDRVSheepdogState *s) +static int get_sheep_fd(BDRVSheepdogState *s, Error **errp) { - Error *local_err = NULL; int fd; - fd = connect_to_sdog(s, &local_err); + fd = connect_to_sdog(s, errp); if (fd < 0) { - qerror_report_err(local_err); - error_free(local_err); return fd; } @@ -1415,8 +1415,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, if (ret < 0) { goto out; } - s->fd = get_sheep_fd(s); + s->fd = get_sheep_fd(s, &local_err); if (s->fd < 0) { + qerror_report_err(local_err); + error_free(local_err); ret = s->fd; goto out; } From 318df29e103ab8a3a5fe25106d0120e956db31aa Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:21 +0200 Subject: [PATCH 28/33] block/sheepdog: Propagate errors through sd_prealloc() Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index b932d6816a..4df45a1591 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1537,21 +1537,18 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot) return 0; } -static int sd_prealloc(const char *filename) +static int sd_prealloc(const char *filename, Error **errp) { BlockDriverState *bs = NULL; uint32_t idx, max_idx; int64_t vdi_size; void *buf = g_malloc0(SD_DATA_OBJ_SIZE); - Error *local_err = NULL; int ret; ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, - NULL, &local_err); + NULL, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); - goto out; + goto out_with_err_set; } vdi_size = bdrv_getlength(bs); @@ -1575,7 +1572,12 @@ static int sd_prealloc(const char *filename) goto out; } } + out: + if (ret < 0) { + error_setg_errno(errp, -ret, "Can't pre-allocate"); + } +out_with_err_set: if (bs) { bdrv_unref(bs); } @@ -1734,7 +1736,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, goto out; } - ret = sd_prealloc(filename); + ret = sd_prealloc(filename, &local_err); + if (ret < 0) { + qerror_report_err(local_err); + error_free(local_err); + } out: g_free(s); return ret; From 7d2d3e74e56b9a341e5206b8749b3d4e67c66959 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:22 +0200 Subject: [PATCH 29/33] block/sheepdog: Propagate errors through do_sd_create() Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 4df45a1591..d01cc8867c 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1481,19 +1481,17 @@ out: return ret; } -static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot) +static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot, + Error **errp) { - Error *local_err = NULL; SheepdogVdiReq hdr; SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; int fd, ret; unsigned int wlen, rlen = 0; char buf[SD_MAX_VDI_LEN]; - fd = connect_to_sdog(s, &local_err); + fd = connect_to_sdog(s, errp); if (fd < 0) { - qerror_report_err(local_err); - error_free(local_err); return fd; } @@ -1522,11 +1520,12 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot) closesocket(fd); if (ret) { + error_setg_errno(errp, -ret, "create failed"); return ret; } if (rsp->result != SD_RES_SUCCESS) { - error_report("%s, %s", sd_strerror(rsp->result), s->inode.name); + error_setg(errp, "%s, %s", sd_strerror(rsp->result), s->inode.name); return -EIO; } @@ -1731,15 +1730,19 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, bdrv_unref(bs); } - ret = do_sd_create(s, &vid, 0); - if (!prealloc || ret) { + ret = do_sd_create(s, &vid, 0, &local_err); + if (ret) { + qerror_report_err(local_err); + error_free(local_err); goto out; } - ret = sd_prealloc(filename, &local_err); - if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); + if (prealloc) { + ret = sd_prealloc(filename, &local_err); + if (ret < 0) { + qerror_report_err(local_err); + error_free(local_err); + } } out: g_free(s); @@ -1928,8 +1931,10 @@ static int sd_create_branch(BDRVSheepdogState *s) * false bail out. */ deleted = sd_delete(s); - ret = do_sd_create(s, &vid, !deleted); + ret = do_sd_create(s, &vid, !deleted, &local_err); if (ret) { + qerror_report_err(local_err); + error_free(local_err); goto out; } @@ -2197,8 +2202,10 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) goto cleanup; } - ret = do_sd_create(s, &new_vid, 1); + ret = do_sd_create(s, &new_vid, 1, &local_err); if (ret < 0) { + qerror_report_err(local_err); + error_free(local_err); error_report("failed to create inode for snapshot. %s", strerror(errno)); goto cleanup; From dc83cd427b4709cec905f2657272d3e554e7abc2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:23 +0200 Subject: [PATCH 30/33] block/sheepdog: Propagate errors through find_vdi_name() Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index d01cc8867c..b72c31828e 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1060,19 +1060,16 @@ static int parse_vdiname(BDRVSheepdogState *s, const char *filename, static int find_vdi_name(BDRVSheepdogState *s, const char *filename, uint32_t snapid, const char *tag, uint32_t *vid, - bool lock) + bool lock, Error **errp) { - Error *local_err = NULL; int ret, fd; SheepdogVdiReq hdr; SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; unsigned int wlen, rlen = 0; char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN]; - fd = connect_to_sdog(s, &local_err); + fd = connect_to_sdog(s, errp); if (fd < 0) { - qerror_report_err(local_err); - error_free(local_err); return fd; } @@ -1097,12 +1094,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, ret = do_req(fd, (SheepdogReq *)&hdr, buf, &wlen, &rlen); if (ret) { + error_setg_errno(errp, -ret, "cannot get vdi info"); goto out; } if (rsp->result != SD_RES_SUCCESS) { - error_report("cannot get vdi info, %s, %s %" PRIu32 " %s", - sd_strerror(rsp->result), filename, snapid, tag); + error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", + sd_strerror(rsp->result), filename, snapid, tag); if (rsp->result == SD_RES_NO_VDI) { ret = -ENOENT; } else { @@ -1279,8 +1277,10 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag) inode = g_malloc(sizeof(s->inode)); - ret = find_vdi_name(s, s->name, snapid, tag, &vid, false); + ret = find_vdi_name(s, s->name, snapid, tag, &vid, false, &local_err); if (ret) { + qerror_report_err(local_err); + error_free(local_err); goto out; } @@ -1423,8 +1423,10 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, goto out; } - ret = find_vdi_name(s, vdi, snapid, tag, &vid, true); + ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, &local_err); if (ret) { + qerror_report_err(local_err); + error_free(local_err); goto out; } From e67c399363e39a4b78fa43f9cd0324ea1a7813bc Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:24 +0200 Subject: [PATCH 31/33] block/sheepdog: Propagate errors to open and create methods Completes the conversion to Error started in commit 015a103^..d5124c0, except for a few bugs fixed in the next commit. Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index b72c31828e..6e2f9812fe 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1391,8 +1391,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, 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; } @@ -1415,18 +1414,14 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, if (ret < 0) { goto out; } - s->fd = get_sheep_fd(s, &local_err); + s->fd = get_sheep_fd(s, errp); if (s->fd < 0) { - qerror_report_err(local_err); - error_free(local_err); ret = s->fd; goto out; } - ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, &local_err); + ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp); if (ret) { - qerror_report_err(local_err); - error_free(local_err); goto out; } @@ -1445,10 +1440,8 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, s->is_snapshot = true; } - fd = connect_to_sdog(s, &local_err); + fd = connect_to_sdog(s, errp); if (fd < 0) { - qerror_report_err(local_err); - error_free(local_err); ret = fd; goto out; } @@ -1651,7 +1644,6 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, char tag[SD_MAX_VDI_TAG_LEN]; uint32_t snapid; bool prealloc = false; - Error *local_err = NULL; s = g_malloc0(sizeof(BDRVSheepdogState)); @@ -1676,8 +1668,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, } else if (!strcmp(options->value.s, "full")) { prealloc = true; } else { - error_report("Invalid preallocation mode: '%s'", - options->value.s); + error_setg(errp, "Invalid preallocation mode: '%s'", + options->value.s); ret = -EINVAL; goto out; } @@ -1693,7 +1685,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, } if (s->inode.vdi_size > SD_MAX_VDI_SIZE) { - error_report("too big image size"); + error_setg(errp, "too big image size"); ret = -EINVAL; goto out; } @@ -1706,24 +1698,22 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, /* Currently, only Sheepdog backing image is supported. */ drv = bdrv_find_protocol(backing_file, true); if (!drv || strcmp(drv->protocol_name, "sheepdog") != 0) { - error_report("backing_file must be a sheepdog image"); + error_setg(errp, "backing_file must be a sheepdog image"); ret = -EINVAL; goto out; } bs = NULL; ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL, - &local_err); + errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); goto out; } base = bs->opaque; if (!is_snapshot(&base->inode)) { - error_report("cannot clone from a non snapshot vdi"); + error_setg(errp, "cannot clone from a non snapshot vdi"); bdrv_unref(bs); ret = -EINVAL; goto out; @@ -1732,19 +1722,13 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, bdrv_unref(bs); } - ret = do_sd_create(s, &vid, 0, &local_err); + ret = do_sd_create(s, &vid, 0, errp); if (ret) { - qerror_report_err(local_err); - error_free(local_err); goto out; } if (prealloc) { - ret = sd_prealloc(filename, &local_err); - if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); - } + ret = sd_prealloc(filename, errp); } out: g_free(s); From efde4b625216b7066b56707d70ae3212edad0eea Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:25 +0200 Subject: [PATCH 32/33] block/sheepdog: Fix silent sd_open(), sd_create() failures Open and create methods must set an error when they fail. Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/sheepdog.c b/block/sheepdog.c index 6e2f9812fe..7bf1275546 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1412,6 +1412,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, ret = parse_vdiname(s, filename, vdi, &snapid, tag); } if (ret < 0) { + error_setg(errp, "Can't parse filename"); goto out; } s->fd = get_sheep_fd(s, errp); @@ -1453,6 +1454,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, closesocket(fd); if (ret) { + error_setg(errp, "Can't read snapshot inode"); goto out; } @@ -1654,6 +1656,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, ret = parse_vdiname(s, filename, s->name, &snapid, tag); } if (ret < 0) { + error_setg(errp, "Can't parse filename"); goto out; } @@ -1677,6 +1680,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, if (options->value.s) { ret = parse_redundancy(s, options->value.s); if (ret < 0) { + error_setg(errp, "Invalid redundancy mode: '%s'", + options->value.s); goto out; } } From fbab9ccbdbb4d8a18abf07b6dc05b3b4a2164cd0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 16 May 2014 11:00:26 +0200 Subject: [PATCH 33/33] block/sheepdog: Don't use qerror_report() qerror_report() is a transitional interface to help with converting existing HMP commands to QMP. It should not be used elsewhere. Replace by error_report(). Cc: MORITA Kazutaka Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/sheepdog.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 7bf1275546..39f746157a 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -723,7 +723,7 @@ static coroutine_fn void reconnect_to_sdog(void *opaque) s->fd = get_sheep_fd(s, &local_err); if (s->fd < 0) { DPRINTF("Wait for connection to be established\n"); - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err)); error_free(local_err); co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME, 1000000000ULL); @@ -1270,7 +1270,7 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); return -EIO; } @@ -1279,7 +1279,7 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag) ret = find_vdi_name(s, s->name, snapid, tag, &vid, false, &local_err); if (ret) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); goto out; } @@ -1753,7 +1753,7 @@ static void sd_close(BlockDriverState *bs) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); return; } @@ -1804,7 +1804,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); return fd; } @@ -1877,7 +1877,7 @@ static bool sd_delete(BDRVSheepdogState *s) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); return false; } @@ -1924,7 +1924,7 @@ static int sd_create_branch(BDRVSheepdogState *s) deleted = sd_delete(s); ret = do_sd_create(s, &vid, !deleted, &local_err); if (ret) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); goto out; } @@ -1933,7 +1933,7 @@ static int sd_create_branch(BDRVSheepdogState *s) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); ret = fd; goto out; @@ -2180,7 +2180,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) /* refresh inode. */ fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); ret = fd; goto cleanup; @@ -2195,7 +2195,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) ret = do_sd_create(s, &new_vid, 1, &local_err); if (ret < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); error_report("failed to create inode for snapshot. %s", strerror(errno)); @@ -2297,7 +2297,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); ret = fd; goto out; @@ -2326,7 +2326,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); ret = fd; goto out; @@ -2388,7 +2388,7 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data, fd = connect_to_sdog(s, &local_err); if (fd < 0) { - qerror_report_err(local_err); + error_report("%s", error_get_pretty(local_err));; error_free(local_err); return fd; }