[Qemu-devel] [PATCH] blockdev: report error on block latency histogram set error

[Qemu-devel] [PATCH] blockdev: report error on block latency histogram set error

[zhenwei pi]

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 <pizhenwei@bytedance.com>
---
  blockdev.c | 19 ++++++++++++++++---
  1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a8755bd..03b1aa3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4377,6 +4377,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);
@@ -4392,21 +4393,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;
+        }
      }
  }
  
-- 
2.7.4

Re: [Qemu-devel] [PATCH] blockdev: report error on block latency histogram set error

[Vladimir Sementsov-Ogievskiy]

18.10.2018 13:42, zhenwei pi wrote:
> 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.

Good catch, thank you, it's my fault!

The problem of the patch is that in case of error, boundaries for read 
(or write) may be set, when others are not.
Better thing is refactor it somehow so that in case of error nothing 
changed finally.

>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  blockdev.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index a8755bd..03b1aa3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4377,6 +4377,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);
> @@ -4392,21 +4393,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;
> +        }
>      }
>  }
>


-- 
Best regards,
Vladimir