From 63d5341f8553ff78ae99407ff8ad5a6648b95a30 Mon Sep 17 00:00:00 2001 From: zhenwei pi Date: Mon, 5 Nov 2018 11:04:56 +0800 Subject: blockdev: handle error on block latency histogram set error Function block_latency_histogram_set may return error, but qapi ignore this. This can be reproduced easily by qmp command: virsh qemu-monitor-command INSTANCE '{"execute":"x-block-latency-histogram-set", "arguments":{"device":"drive-virtio-disk1","boundaries":[10,200,40]}}' In fact this command does not work, but we still get success result. qmp_x_block_latency_histogram_set is a batch setting API, report error ASAP. Signed-off-by: zhenwei pi Signed-off-by: Kevin Wolf --- blockdev.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'blockdev.c') diff --git a/blockdev.c b/blockdev.c index e5b5eb46e2..9310ff3e7c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -4413,6 +4413,7 @@ void qmp_x_block_latency_histogram_set( { BlockBackend *blk = blk_by_name(device); BlockAcctStats *stats; + int ret; if (!blk) { error_setg(errp, "Device '%s' not found", device); @@ -4428,21 +4429,33 @@ void qmp_x_block_latency_histogram_set( } if (has_boundaries || has_boundaries_read) { - block_latency_histogram_set( + ret = block_latency_histogram_set( stats, BLOCK_ACCT_READ, has_boundaries_read ? boundaries_read : boundaries); + if (ret) { + error_setg(errp, "Device '%s' set read boundaries fail", device); + return; + } } if (has_boundaries || has_boundaries_write) { - block_latency_histogram_set( + ret = block_latency_histogram_set( stats, BLOCK_ACCT_WRITE, has_boundaries_write ? boundaries_write : boundaries); + if (ret) { + error_setg(errp, "Device '%s' set write boundaries fail", device); + return; + } } if (has_boundaries || has_boundaries_flush) { - block_latency_histogram_set( + ret = block_latency_histogram_set( stats, BLOCK_ACCT_FLUSH, has_boundaries_flush ? boundaries_flush : boundaries); + if (ret) { + error_setg(errp, "Device '%s' set flush boundaries fail", device); + return; + } } } -- cgit 1.4.1 From d52e1a0e967a532867149c2d95c2dd664ca22fe8 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 1 Nov 2018 16:30:37 +0000 Subject: blockdev: Consistently use snapshot_node_name in external_snapshot_prepare() In the function external_snapshot_prepare() we have a BlockdevSnapshotSync struct, which has the usual combination of has_snapshot_node_name and snapshot_node_name fields for an optional field. We set up a local variable const char *snapshot_node_name = s->has_snapshot_node_name ? s->snapshot_node_name : NULL; and then mostly use "if (!snapshot_node_name)" for checking whether we have a snapshot node name. The exception is that in one place we check s->has_snapshot_node_name instead. This confuses Coverity (CID 1396473), which thinks it might be possible to get here with s->has_snapshot_node_name true but snapshot_node_name NULL, and warns that the call to qdict_put_str() will segfault in that case. Make the code consistent and unconfuse Coverity by using the same check for this conditional that we do in the rest of the surrounding code. Signed-off-by: Peter Maydell Reviewed-by: Alberto Garcia Signed-off-by: Kevin Wolf --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'blockdev.c') diff --git a/blockdev.c b/blockdev.c index 9310ff3e7c..81f95d920b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1640,7 +1640,7 @@ static void external_snapshot_prepare(BlkActionState *common, } options = qdict_new(); - if (s->has_snapshot_node_name) { + if (snapshot_node_name) { qdict_put_str(options, "node-name", snapshot_node_name); } qdict_put_str(options, "driver", format); -- cgit 1.4.1