On Tue 22 Aug 2017 12:15:33 PM CEST, Manos Pitsidianakis wrote:
> +static ThrottleGroup *throttle_group_by_name(const char *name)
> +{
> + ThrottleGroup *iter;
> +
> + /* Look for an existing group with that name */
> + QTAILQ_FOREACH(iter, &throttle_groups, list) {
> + if (!g_strcmp0(name, iter->name)) {
> + return iter;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static bool throttle_group_exists(const char *name)
> +{
> + return throttle_group_by_name(name) ? true : false;
> +}
"foo ? true : false" is redundant, it means
"if foo is true, then true; if foo is false, then false"
I'd leave it as 'throttle_group_by_name(name) != NULL'.
Or you can simply remove the function and put that expression in
throttle_group_obj_complete(), where you call it.
> +#undef THROTTLE_OPT_PREFIX
> +#define THROTTLE_OPT_PREFIX "x-"
> +
> +/* Helper struct and array for QOM property setter/getter */
> +typedef struct {
> + const char *name;
> + BucketType type;
> + enum LeakyBucketField {
> + AVG,
> + MAX,
> + BURST_LENGTH,
> + IOPS_SIZE,
> + } category;
> +} ThrottleParamInfo;
I think it looks much nicer now :) I don't think you need to name the
enum (LeakyBucketField) by the way.
> +/* This function edits throttle_groups and must be called under the global
> + * mutex */
> +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
> +{
> + ThrottleGroup *tg = THROTTLE_GROUP(obj);
> + ThrottleConfig cfg;
> +
> + /* set group name to object id if it exists */
> + if (!tg->name && tg->parent_obj.parent) {
> + tg->name = object_get_canonical_path_component(OBJECT(obj));
> + }
> + /* We must have a group name at this point */
> + assert(tg->name);
> +
> + /* error if name is duplicate */
> + if (throttle_group_exists(tg->name)) {
> + error_setg(errp, "A group with this name already exists");
> + return;
> + }
> +
> + /* check validity */
> + throttle_get_config(&tg->ts, &cfg);
> + if (!throttle_is_valid(&cfg, errp)) {
> + return;
> + }
My fault here because I suggested its removal, but I actually think we
still need to call throttle_config() here so bkt->max is initialized in
throttle_fix_bucket(). I'll take care of updating this call once my
patches to remove throttle_fix_bucket() are applied, but for now we
still need it.
> + cfg = tg->ts.cfg;
> + switch (info->category) {
> + case AVG:
> + cfg.buckets[info->type].avg = value;
> + break;
> + case MAX:
> + cfg.buckets[info->type].max = value;
> + break;
> + case BURST_LENGTH:
> + if (value > UINT_MAX) {
> + error_setg(&local_err, "%s value must be in the"
> + "range [0, %u]", info->name, UINT_MAX);
> + goto ret;
> + }
> + cfg.buckets[info->type].burst_length = value;
> + break;
> + case IOPS_SIZE:
> + cfg.op_size = value;
> + }
> +
> + tg->ts.cfg = cfg;
I like how this looks now. Perhaps we can get rid of the local cfg
variable and use a pointer instead, there's no need to copy the config
back and forth.
Berto
