Don't allow burst limits to be lower than the normal limits

Hello,

Gu Nini found this problem and reported it in

https://bugzilla.redhat.com/show_bug.cgi?id=1355665

When setting the throttling configuration, the burst limits can be lower than the normal limits. This does not making any sense and behaves oddly, so let's forbid it.

Berto

Alberto Garcia (2):
throttle: Don't allow burst limits to be lower than the normal limits
throttle: Test burst limits lower than the normal limits

 tests/test-throttle.c | 8 ++++++++
 util/throttle.c       | 6 ++++++
 2 files changed, 14 insertions(+)

2.8.1

Setting FOO_max to a value that is lower than FOO does not make sense, and it produces odd results depending on the value of FOO_max_length. Although the user should not set that configuration in the first place it's better to reject it explicitly.

https://bugzilla.redhat.com/show_bug.cgi?id=1355665

---
 util/throttle.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 654f95c..7a5c619 100644
@@ -348,6 +348,12 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
                        " bps/iops values");
             return false;
         }
+
+        if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) {
+            error_setg(errp, "if bps_max/iops_max is set it cannot be lower"
+                       " than the bps/iops values");
+            return false;
+        }
     }
 
     return true;

2.8.1

That's not strictly true since the default value of bps_max is 0 (unless we forbid setting it explicitly to 0 as well).

Berto

The message can only be emitted if bps_max was set. If bps_max is its default value of 0 (or even if the user explicitly set it to zero), then this message won't appear. Therefore, telling the user that "if this is set" is redundant, because the error is only issued when it IS set.

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

This checks that making FOO_max lower than FOO is not allowed.

We could also forbid having FOO_max == FOO, but that doesn't have any odd side effects and it would require us to update several other tests, so let's keep it simple.

---
 tests/test-throttle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index afe094b..363b59a 100644
@@ -394,6 +394,14 @@ static void test_max_is_missing_limit(void)
         cfg.buckets[i].max = 0;
         cfg.buckets[i].avg = 100;
         g_assert(throttle_is_valid(&cfg, NULL));
+
+        cfg.buckets[i].max = 30;
+        cfg.buckets[i].avg = 100;
+        g_assert(!throttle_is_valid(&cfg, NULL));
+
+        cfg.buckets[i].max = 100;
+        cfg.buckets[i].avg = 100;
+        g_assert(throttle_is_valid(&cfg, NULL));
     }
 }
 

2.8.1