| Message ID | 1e88093dfa1e7b1714a1cc94a01713dade46eed8.1469657519.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/27/2016 04:16 PM, Alberto Garcia wrote: > 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 > > Signed-off-by: Alberto Garcia <berto@igalia.com> > Reported-by: Gu Nini <ngu@redhat.com> > --- > util/throttle.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/util/throttle.c b/util/throttle.c > index 654f95c..7a5c619 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -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"); Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower than bps/iops values". But the idea makes sense.
On Thu 28 Jul 2016 12:33:51 AM CEST, Eric Blake <eblake@redhat.com> wrote: >> + 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"); > > Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower > than bps/iops values". But the idea makes sense. 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
On 07/27/2016 04:45 PM, Alberto Garcia wrote: > On Thu 28 Jul 2016 12:33:51 AM CEST, Eric Blake <eblake@redhat.com> wrote: >>> + 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"); >> >> Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower >> than bps/iops values". But the idea makes sense. > > That's not strictly true since the default value of bps_max is 0 (unless > we forbid setting it explicitly to 0 as well). 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.
diff --git a/util/throttle.c b/util/throttle.c index 654f95c..7a5c619 100644 --- a/util/throttle.c +++ b/util/throttle.c @@ -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;
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 Signed-off-by: Alberto Garcia <berto@igalia.com> Reported-by: Gu Nini <ngu@redhat.com> --- util/throttle.c | 6 ++++++ 1 file changed, 6 insertions(+)