[Qemu-devel] [PATCH 0/2] throttle: refuse bps_max/iops_max without bps/iops

[Qemu-devel] [PATCH 0/2] throttle: refuse bps_max/iops_max without bps/iops

[Stefan Hajnoczi]

A bps_max/iops_max value without a corresponding bps/iops does nothing.  Avoid
confusing the user by refusing invalid inputs instead of silently ignoring
them.

Stefan Hajnoczi (2):
  throttle: refuse bps_max/iops_max without bps/iops
  throttle: add throttle_max_is_missing_limit() test

 blockdev.c              |  6 ++++++
 include/qemu/throttle.h |  2 ++
 tests/test-throttle.c   | 21 +++++++++++++++++++++
 util/throttle.c         | 15 +++++++++++++++
 4 files changed, 44 insertions(+)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] throttle: refuse bps_max/iops_max without bps/iops

[Stefan Hajnoczi]

The bps_max/iops_max values are meaningless without corresponding
bps/iops values.  Reported an error if bps_max/iops_max is given without
bps/iops.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c              |  6 ++++++
 include/qemu/throttle.h |  2 ++
 util/throttle.c         | 15 +++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 62a4586..4125ff6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -337,6 +337,12 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
         return false;
     }
 
+    if (throttle_max_is_missing_limit(cfg)) {
+        error_setg(errp, "bps_max/iops_max require corresponding"
+                         " bps/iops values");
+        return false;
+    }
+
     return true;
 }
 
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 995b2d5..12faaad 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -114,6 +114,8 @@ bool throttle_conflicting(ThrottleConfig *cfg);
 
 bool throttle_is_valid(ThrottleConfig *cfg);
 
+bool throttle_max_is_missing_limit(ThrottleConfig *cfg);
+
 void throttle_config(ThrottleState *ts,
                      ThrottleTimers *tt,
                      ThrottleConfig *cfg);
diff --git a/util/throttle.c b/util/throttle.c
index 706c131..1113671 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -300,6 +300,21 @@ bool throttle_is_valid(ThrottleConfig *cfg)
     return !invalid;
 }
 
+/* check if bps_max/iops_max is used without bps/iops
+ * @cfg: the throttling configuration to inspect
+ */
+bool throttle_max_is_missing_limit(ThrottleConfig *cfg)
+{
+    int i;
+
+    for (i = 0; i < BUCKETS_COUNT; i++) {
+        if (cfg->buckets[i].max && !cfg->buckets[i].avg) {
+            return true;
+        }
+    }
+    return false;
+}
+
 /* fix bucket parameters */
 static void throttle_fix_bucket(LeakyBucket *bkt)
 {
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] throttle: add throttle_max_is_missing_limit() test

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/test-throttle.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 0168445..85c9b6c 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -329,6 +329,26 @@ static void test_is_valid(void)
     test_is_valid_for_value(1, true);
 }
 
+static void test_max_is_missing_limit(void)
+{
+    int i;
+
+    for (i = 0; i < BUCKETS_COUNT; i++) {
+        memset(&cfg, 0, sizeof(cfg));
+        cfg.buckets[i].max = 100;
+        cfg.buckets[i].avg = 0;
+        g_assert(throttle_max_is_missing_limit(&cfg));
+
+        cfg.buckets[i].max = 0;
+        cfg.buckets[i].avg = 0;
+        g_assert(!throttle_max_is_missing_limit(&cfg));
+
+        cfg.buckets[i].max = 0;
+        cfg.buckets[i].avg = 100;
+        g_assert(!throttle_max_is_missing_limit(&cfg));
+    }
+}
+
 static void test_have_timer(void)
 {
     /* zero structures */
@@ -591,6 +611,7 @@ int main(int argc, char **argv)
     g_test_add_func("/throttle/config/enabled",     test_enabled);
     g_test_add_func("/throttle/config/conflicting", test_conflicting_config);
     g_test_add_func("/throttle/config/is_valid",    test_is_valid);
+    g_test_add_func("/throttle/config/max",         test_max_is_missing_limit);
     g_test_add_func("/throttle/config_functions",   test_config_functions);
     g_test_add_func("/throttle/accounting",         test_accounting);
     g_test_add_func("/throttle/groups",             test_groups);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/2] throttle: refuse bps_max/iops_max without bps/iops

[Alberto Garcia]

On Tue 04 Aug 2015 12:22:12 PM CEST, Stefan Hajnoczi wrote:
> The bps_max/iops_max values are meaningless without corresponding
> bps/iops values.  Reported an error if bps_max/iops_max is given without
> bps/iops.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 2/2] throttle: add throttle_max_is_missing_limit() test

[Alberto Garcia]

On Tue 04 Aug 2015 12:22:13 PM CEST, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 0/2] throttle: refuse bps_max/iops_max without bps/iops

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Tue, Aug 04, 2015 at 11:22:11AM +0100, Stefan Hajnoczi wrote:
> A bps_max/iops_max value without a corresponding bps/iops does nothing.  Avoid
> confusing the user by refusing invalid inputs instead of silently ignoring
> them.
> 
> Stefan Hajnoczi (2):
>   throttle: refuse bps_max/iops_max without bps/iops
>   throttle: add throttle_max_is_missing_limit() test
> 
>  blockdev.c              |  6 ++++++
>  include/qemu/throttle.h |  2 ++
>  tests/test-throttle.c   | 21 +++++++++++++++++++++
>  util/throttle.c         | 15 +++++++++++++++
>  4 files changed, 44 insertions(+)
> 
> -- 
> 2.4.3
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]