From 1ba4b6a553ad9ff4645af7fab8adfc6e810fcc69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Tue, 22 Apr 2014 17:05:27 +0200 Subject: block: Prevent coroutine stack overflow when recursing in bdrv_open_backing_file. In 1.7.1 qcow2_create2 reopen the file for flushing without the BDRV_O_NO_BACKING flags. As a consequence the code would recursively open the whole backing chain. These three stack arrays would pile up through the recursion and lead to a coroutine stack overflow. Convert these array to malloced buffers in order to streamline the coroutine footprint. Symptoms where freezes or segfaults on production machines while taking QMP externals snapshots. The overflow disturbed coroutine switching. [Resolved conflicts on qemu.git/master since the patch was against v1.7.1 --Stefan] Signed-off-by: Benoit Canet Signed-off-by: Stefan Hajnoczi --- block.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index fc2edd3..bad7fe0 100644 --- a/block.c +++ b/block.c @@ -1068,14 +1068,14 @@ fail: */ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) { - char backing_filename[PATH_MAX]; - int back_flags, ret; + char *backing_filename = g_malloc0(PATH_MAX); + int back_flags, ret = 0; BlockDriver *back_drv = NULL; Error *local_err = NULL; if (bs->backing_hd != NULL) { QDECREF(options); - return 0; + goto free_exit; } /* NULL means an empty set of options */ @@ -1088,10 +1088,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) backing_filename[0] = '\0'; } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { QDECREF(options); - return 0; + goto free_exit; } else { - bdrv_get_full_backing_filename(bs, backing_filename, - sizeof(backing_filename)); + bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX); } if (bs->backing_format[0] != '\0') { @@ -1112,7 +1111,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) error_setg(errp, "Could not open backing file: %s", error_get_pretty(local_err)); error_free(local_err); - return ret; + goto free_exit; } if (bs->backing_hd->file) { @@ -1123,7 +1122,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) /* Recalculate the BlockLimits with the backing file */ bdrv_refresh_limits(bs); - return 0; +free_exit: + g_free(backing_filename); + return ret; } /* @@ -1180,8 +1181,7 @@ done: void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) { /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ - char tmp_filename[PATH_MAX + 1]; - + char *tmp_filename = g_malloc0(PATH_MAX + 1); int64_t total_size; BlockDriver *bdrv_qcow2; QEMUOptionParameter *create_options; @@ -1197,15 +1197,15 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) total_size = bdrv_getlength(bs); if (total_size < 0) { error_setg_errno(errp, -total_size, "Could not get image size"); - return; + goto out; } total_size &= BDRV_SECTOR_MASK; /* Create the temporary image */ - ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename)); + ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); if (ret < 0) { error_setg_errno(errp, -ret, "Could not get temporary filename"); - return; + goto out; } bdrv_qcow2 = bdrv_find_format("qcow2"); @@ -1221,7 +1221,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) "'%s': %s", tmp_filename, error_get_pretty(local_err)); error_free(local_err); - return; + goto out; } /* Prepare a new options QDict for the temporary file */ @@ -1238,10 +1238,13 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) bs->open_flags & ~BDRV_O_SNAPSHOT, bdrv_qcow2, &local_err); if (ret < 0) { error_propagate(errp, local_err); - return; + goto out; } bdrv_append(bs_snapshot, bs); + +out: + g_free(tmp_filename); } /* -- cgit v1.1 From d1db760d7be664c1345670637ba0c5accbf73710 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 23 Apr 2014 13:55:37 +0200 Subject: doc: add -drive rerror=,werror= to qemu --help output These options are already documented on the man page but missing from qemu --help. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf --- qemu-options.hx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qemu-options.hx b/qemu-options.hx index 6457034..98b4002 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -408,7 +408,8 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n" " [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n" " [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n" - " [,serial=s][,addr=A][,id=name][,aio=threads|native]\n" + " [,serial=s][,addr=A][,rerror=ignore|stop|report]\n" + " [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n" " [,readonly=on|off][,copy-on-read=on|off]\n" " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n" " [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n" -- cgit v1.1 From 9974ad40bf36d605f1134b94e51fd53e9970f46a Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Wed, 2 Apr 2014 13:54:07 +0800 Subject: qemu-iotests: Improve and make use of QMPTestCase.wait_until_completed() This eliminates code duplication. Signed-off-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/030 | 50 +++++-------------------------------------- tests/qemu-iotests/056 | 9 +------- tests/qemu-iotests/iotests.py | 5 +++-- 3 files changed, 9 insertions(+), 55 deletions(-) diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 59a34f7..8cb61fd 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -50,15 +50,7 @@ class TestSingleDrive(iotests.QMPTestCase): result = self.vm.qmp('block-stream', device='drive0') self.assert_qmp(result, 'return', {}) - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/type', 'stream') - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp(event, 'data/offset', self.image_len) - self.assert_qmp(event, 'data/len', self.image_len) - completed = True + self.wait_until_completed() self.assert_no_active_block_jobs() self.vm.shutdown() @@ -89,15 +81,7 @@ class TestSingleDrive(iotests.QMPTestCase): self.assert_qmp(result, 'return', {}) self.vm.resume_drive('drive0') - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/type', 'stream') - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp(event, 'data/offset', self.image_len) - self.assert_qmp(event, 'data/len', self.image_len) - completed = True + self.wait_until_completed() self.assert_no_active_block_jobs() self.vm.shutdown() @@ -112,15 +96,7 @@ class TestSingleDrive(iotests.QMPTestCase): result = self.vm.qmp('block-stream', device='drive0', base=mid_img) self.assert_qmp(result, 'return', {}) - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/type', 'stream') - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp(event, 'data/offset', self.image_len) - self.assert_qmp(event, 'data/len', self.image_len) - completed = True + self.wait_until_completed() self.assert_no_active_block_jobs() self.vm.shutdown() @@ -152,15 +128,7 @@ class TestSmallerBackingFile(iotests.QMPTestCase): result = self.vm.qmp('block-stream', device='drive0') self.assert_qmp(result, 'return', {}) - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/type', 'stream') - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp(event, 'data/offset', self.image_len) - self.assert_qmp(event, 'data/len', self.image_len) - completed = True + self.wait_until_completed() self.assert_no_active_block_jobs() self.vm.shutdown() @@ -442,15 +410,7 @@ class TestSetSpeed(iotests.QMPTestCase): result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024) self.assert_qmp(result, 'return', {}) - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/type', 'stream') - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp(event, 'data/offset', self.image_len) - self.assert_qmp(event, 'data/len', self.image_len) - completed = True + self.wait_until_completed() self.assert_no_active_block_jobs() diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056 index 6389342..54e4bd0 100755 --- a/tests/qemu-iotests/056 +++ b/tests/qemu-iotests/056 @@ -57,14 +57,7 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase): format=iotests.imgfmt, target=target_img) self.assert_qmp(result, 'return', {}) - # Custom completed check as we are not copying all data. - completed = False - while not completed: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - self.assert_qmp(event, 'data/device', 'drive0') - self.assert_qmp_absent(event, 'data/error') - completed = True + self.wait_until_completed(check_offset=False) self.assert_no_active_block_jobs() self.vm.shutdown() diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index e4fa9af..f6c437c 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -257,7 +257,7 @@ class QMPTestCase(unittest.TestCase): self.assert_no_active_block_jobs() return result - def wait_until_completed(self, drive='drive0'): + def wait_until_completed(self, drive='drive0', check_offset=True): '''Wait for a block job to finish, returning the event''' completed = False while not completed: @@ -265,7 +265,8 @@ class QMPTestCase(unittest.TestCase): if event['event'] == 'BLOCK_JOB_COMPLETED': self.assert_qmp(event, 'data/device', drive) self.assert_qmp_absent(event, 'data/error') - self.assert_qmp(event, 'data/offset', self.image_len) + if check_offset: + self.assert_qmp(event, 'data/offset', self.image_len) self.assert_qmp(event, 'data/len', self.image_len) completed = True -- cgit v1.1 From 460787605e59526bb5c47002b512028ea1f63ac3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 24 Apr 2014 11:53:39 +0200 Subject: MAINTAINERS: Add qemu-img/io to block subsystem qemu-img and qemu-io were not covered by any MAINTAINERS entry so far. Reported-by: Markus Armbruster Signed-off-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index c66946f..b287ef8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -674,6 +674,8 @@ S: Supported F: block* F: block/ F: hw/block/ +F: qemu-img* +F: qemu-io* T: git git://repo.or.cz/qemu/kevin.git block T: git git://github.com/stefanha/qemu.git block -- cgit v1.1 From a28315ebaf3910b65ef51abefc4ef040265afc19 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 24 Apr 2014 15:02:39 +0200 Subject: block: Expose host_* drivers in blockdev-add All the functionality to use the host_device, host_cdrom and host_floppy drivers is already there, they just need to be added to the schema. The block driver names containing underscores are preexisting and cannot be changed without breaking command line compatibility. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- qapi-schema.json | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/qapi-schema.json b/qapi-schema.json index 391356f..0b00427 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4285,10 +4285,13 @@ # # Drivers that are supported in block device operations. # +# @host_device, @host_cdrom, @host_floppy: Since 2.1 +# # Since: 2.0 ## { 'enum': 'BlockdevDriver', - 'data': [ 'file', 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug', + 'data': [ 'file', 'host_device', 'host_cdrom', 'host_floppy', + 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug', 'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow', 'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] } @@ -4555,6 +4558,9 @@ 'discriminator': 'driver', 'data': { 'file': 'BlockdevOptionsFile', + 'host_device':'BlockdevOptionsFile', + 'host_cdrom': 'BlockdevOptionsFile', + 'host_floppy':'BlockdevOptionsFile', 'http': 'BlockdevOptionsFile', 'https': 'BlockdevOptionsFile', 'ftp': 'BlockdevOptionsFile', -- cgit v1.1 From 0fb6395c0cb5046432a80d608ddde7a3b2f8a9ae Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Apr 2014 16:50:31 +0200 Subject: Use error_is_set() only when necessary (again) error_is_set(&var) is the same as var != NULL, but it takes whole-program analysis to figure that out. Unnecessarily hard for optimizers, static checkers, and human readers. Commit 84d18f0 dumbed it down to obvious, but a few more have crept in since, and documentation was overlooked. Dumb these down, too. Signed-off-by: Markus Armbruster Signed-off-by: Stefan Hajnoczi --- block.c | 2 +- block/mirror.c | 2 +- block/nfs.c | 2 +- block/quorum.c | 4 ++-- docs/writing-qmp-commands.txt | 6 +++--- tests/test-qmp-input-strict.c | 8 ++++---- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/block.c b/block.c index bad7fe0..4745712 100644 --- a/block.c +++ b/block.c @@ -864,7 +864,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, node_name = qdict_get_try_str(options, "node-name"); bdrv_assign_node_name(bs, node_name, &local_err); - if (error_is_set(&local_err)) { + if (local_err) { error_propagate(errp, local_err); return -EINVAL; } diff --git a/block/mirror.c b/block/mirror.c index 2618c37..36f4f8e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -680,7 +680,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, &local_err, &commit_active_job_driver, false, base); - if (error_is_set(&local_err)) { + if (local_err) { error_propagate(errp, local_err); goto error_restore_flags; } diff --git a/block/nfs.c b/block/nfs.c index 98aa363..9fa831f 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -343,7 +343,7 @@ static int nfs_file_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 (error_is_set(&local_err)) { + if (local_err) { error_propagate(errp, local_err); return -EINVAL; } diff --git a/block/quorum.c b/block/quorum.c index 7f580a8..ecec3a5 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -753,7 +753,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, opts = qemu_opts_create(&quorum_runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); - if (error_is_set(&local_err)) { + if (local_err) { ret = -EINVAL; goto exit; } @@ -828,7 +828,7 @@ close_exit: g_free(opened); exit: /* propagate error */ - if (error_is_set(&local_err)) { + if (local_err) { error_propagate(errp, local_err); } QDECREF(list); diff --git a/docs/writing-qmp-commands.txt b/docs/writing-qmp-commands.txt index 8349dec..3930a9b 100644 --- a/docs/writing-qmp-commands.txt +++ b/docs/writing-qmp-commands.txt @@ -311,7 +311,7 @@ void hmp_hello_world(Monitor *mon, const QDict *qdict) Error *errp = NULL; qmp_hello_world(!!message, message, &errp); - if (error_is_set(&errp)) { + if (errp) { monitor_printf(mon, "%s\n", error_get_pretty(errp)); error_free(errp); return; @@ -483,7 +483,7 @@ void hmp_info_alarm_clock(Monitor *mon) Error *errp = NULL; clock = qmp_query_alarm_clock(&errp); - if (error_is_set(&errp)) { + if (errp) { monitor_printf(mon, "Could not query alarm clock information\n"); error_free(errp); return; @@ -634,7 +634,7 @@ void hmp_info_alarm_methods(Monitor *mon) Error *errp = NULL; method_list = qmp_query_alarm_methods(&errp); - if (error_is_set(&errp)) { + if (errp) { monitor_printf(mon, "Could not query alarm methods\n"); error_free(errp); return; diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index 38b5e95..f03353b 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -153,7 +153,7 @@ static void test_validate_union_flat(TestInputVisitorData *data, /* TODO when generator bug is fixed, add 'integer': 41 */ visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp); - g_assert(!error_is_set(&errp)); + g_assert(!errp); qapi_free_UserDefFlatUnion(tmp); } @@ -167,7 +167,7 @@ static void test_validate_union_anon(TestInputVisitorData *data, v = validate_test_init(data, "42"); visit_type_UserDefAnonUnion(v, &tmp, NULL, &errp); - g_assert(!error_is_set(&errp)); + g_assert(!errp); qapi_free_UserDefAnonUnion(tmp); } @@ -240,7 +240,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }"); visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp); - g_assert(error_is_set(&errp)); + g_assert(errp); qapi_free_UserDefFlatUnion(tmp); } @@ -254,7 +254,7 @@ static void test_validate_fail_union_anon(TestInputVisitorData *data, v = validate_test_init(data, "3.14"); visit_type_UserDefAnonUnion(v, &tmp, NULL, &errp); - g_assert(error_is_set(&errp)); + g_assert(errp); qapi_free_UserDefAnonUnion(tmp); } -- cgit v1.1 From 4399c438a431effbb785c6c7314b748a4e13734a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Apr 2014 16:50:32 +0200 Subject: qemu-img: Consistently name Error * objects err, and not errp Signed-off-by: Markus Armbruster Signed-off-by: Stefan Hajnoczi --- qemu-img.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 4dae84a..968b4c8 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -457,12 +457,12 @@ fail: static void dump_json_image_check(ImageCheck *check, bool quiet) { - Error *errp = NULL; + Error *local_err = NULL; QString *str; QmpOutputVisitor *ov = qmp_output_visitor_new(); QObject *obj; visit_type_ImageCheck(qmp_output_get_visitor(ov), - &check, NULL, &errp); + &check, NULL, &local_err); obj = qmp_output_get_qobject(ov); str = qobject_to_json_pretty(obj); assert(str != NULL); @@ -1731,12 +1731,12 @@ static void dump_snapshots(BlockDriverState *bs) static void dump_json_image_info_list(ImageInfoList *list) { - Error *errp = NULL; + Error *local_err = NULL; QString *str; QmpOutputVisitor *ov = qmp_output_visitor_new(); QObject *obj; visit_type_ImageInfoList(qmp_output_get_visitor(ov), - &list, NULL, &errp); + &list, NULL, &local_err); obj = qmp_output_get_qobject(ov); str = qobject_to_json_pretty(obj); assert(str != NULL); @@ -1748,12 +1748,12 @@ static void dump_json_image_info_list(ImageInfoList *list) static void dump_json_image_info(ImageInfo *info) { - Error *errp = NULL; + Error *local_err = NULL; QString *str; QmpOutputVisitor *ov = qmp_output_visitor_new(); QObject *obj; visit_type_ImageInfo(qmp_output_get_visitor(ov), - &info, NULL, &errp); + &info, NULL, &local_err); obj = qmp_output_get_qobject(ov); str = qobject_to_json_pretty(obj); assert(str != NULL); -- cgit v1.1 From 92de9012904cd6ada47565f48db545d53069d933 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Apr 2014 16:50:33 +0200 Subject: nbd: Use return values instead of error_is_set(errp) Using error_is_set(errp) to check whether a function call failed is fragile: it breaks when errp is null. Check perfectly suitable return values instead when possible. errp can't be null there now, but this is more robust and more obviously correct Signed-off-by: Markus Armbruster Signed-off-by: Stefan Hajnoczi --- block/nbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/nbd.c b/block/nbd.c index 5512423..613f258 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -175,7 +175,7 @@ static void nbd_parse_filename(const char *filename, QDict *options, InetSocketAddress *addr = NULL; addr = inet_parse(host_spec, errp); - if (error_is_set(errp)) { + if (!addr) { goto out; } -- cgit v1.1 From f70edf99483e7ee5f89d69fd0c51d04a35f03932 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Apr 2014 16:50:34 +0200 Subject: blockdev: Clean up fragile use of error_is_set() Using error_is_set(ERRP) to find out whether a function failed is either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP may be null, because errors go undetected when it is. It's fragile when proving ERRP non-null involves a non-local argument. Else, it's unnecessarily opaque (see commit 84d18f0). The error_is_set(errp) in internal_snapshot_prepare() is merely fragile, because the caller never passes a null errp argument. Make the code more robust and more obviously correct: receive the error in a local variable, then propagate it through the parameter. Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index 09826f1..952eb60 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1115,6 +1115,7 @@ typedef struct InternalSnapshotState { static void internal_snapshot_prepare(BlkTransactionState *common, Error **errp) { + Error *local_err = NULL; const char *device; const char *name; BlockDriverState *bs; @@ -1163,8 +1164,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common, } /* check whether a snapshot with name exist */ - ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &old_sn, errp); - if (error_is_set(errp)) { + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &old_sn, + &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } else if (ret) { error_setg(errp, -- cgit v1.1 From 172fc4dd33e604dcf868c28e73398c19e161708b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Apr 2014 16:50:35 +0200 Subject: iscsi: Don't use error_is_set() to suppress additional errors Using error_is_set(errp) that way can sweep programming errors under the carpet when we get called incorrectly with an error set. Commit 24d3bd6 added a broken error path to iscsi_do_inquiry(): it first calls error_setg(), then jumps to the preexisting error label, where error_setg() gets called again, triggering an assertion failure. Commit cbee81f fixed this by guarding the second error_setg() with an error_is_set(). Replace this fix by a simpler and safer one: jump right behind the second error_setg(). Signed-off-by: Markus Armbruster Reviewed-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- block/iscsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index a636ea4..a30202b 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1095,16 +1095,15 @@ static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, int lun, *inq = scsi_datain_unmarshall(task); if (*inq == NULL) { error_setg(errp, "iSCSI: failed to unmarshall inquiry datain blob"); - goto fail; + goto fail_with_err; } return task; fail: - if (!error_is_set(errp)) { - error_setg(errp, "iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsi)); - } + error_setg(errp, "iSCSI: Inquiry command failed : %s", + iscsi_get_error(iscsi)); +fail_with_err: if (task != NULL) { scsi_free_scsi_task(task); } -- cgit v1.1