On Fri, Aug 18, 2017 at 03:05:31PM +0200, Alberto Garcia wrote:
On Fri 18 Aug 2017 05:10:17 AM CEST, Manos Pitsidianakis wrote:* If no ThrottleGroup is found with the given name a new one is * created. * - * @name: the name of the ThrottleGroup + * This function edits throttle_groups and must be called under the global + * mutex. + * + * @name: the name of the ThrottleGroup, NULL means a new anonymous group will + * be created. * @ret: the ThrottleState member of the ThrottleGroup */ ThrottleState *throttle_group_incref(const char *name)If we're not going to have anonymous groups in the end this patch needs changes (name == NULL is no longer allowed).+/* 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), *iter; + ThrottleConfig *cfg = &tg->ts.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)); + } + + if (tg->name) { + /* error if name is duplicate */ + QTAILQ_FOREACH(iter, &throttle_groups, list) { + if (!g_strcmp0(tg->name, iter->name) && tg != iter) {I'm just nitpicking here :) but if you change this and put 'tg != iter' first you'll save one unnecessary string comparison.
Only in the case where tg == iter, otherwise you have one unnecessary pointer comparison for every other group.
+ error_setg(errp, "A group with this name already exists"); + return; + } + } + } + + /* unfix buckets to check validity */ + throttle_get_config(&tg->ts, cfg); + if (!throttle_is_valid(cfg, errp)) { + return; + } + /* fix buckets again */ + throttle_config(&tg->ts, tg->clock_type, cfg);throttle_get_config(ts, cfg) makes a copy of the existing configuration, but the cfg pointer that you are passing already points to the existing configuration, so in practice you are doing *(ts->cfg) = *(ts->cfg); throttle_unfix_bucket(...); and since you "unfixed" the configuration, then you need undo the whole thing by setting the config again: *(ts->cfg) = *(ts->cfg); throttle_fix_bucket(...); You should declare a local ThrottleConfig variable, copy the config there, and run throttle_is_valid() on that copy. The final throttle_config() call is unnecessary.
I figured I didn't have to make an extra copy but this looks bad in retrospect
Once the patches I sent yesterday are merged we'll be able to skip the throttle_get_config() call and do throttle_is_valid(&tg->ts.cfg, errp) directly.+static void throttle_group_set(Object *obj, Visitor *v, const char * name, + void *opaque, Error **errp) + +{ + ThrottleGroup *tg = THROTTLE_GROUP(obj); + ThrottleConfig cfg; + ThrottleParamInfo *info = opaque; + Error *local_err = NULL; + int64_t value; + + /* If we have finished initialization, don't accept individual property + * changes through QOM. Throttle configuration limits must be set in one + * transaction, as certain combinations are invalid. + */ + if (tg->is_initialized) { + error_setg(&local_err, "Property cannot be set after initialization"); + goto ret; + } + + visit_type_int64(v, name, &value, &local_err); + if (local_err) { + goto ret; + } + if (value < 0) { + error_setg(&local_err, "Property values cannot be negative"); + goto ret; + } + + cfg = tg->ts.cfg; + switch (info->data_type) { + case UINT64: + { + uint64_t *field = (void *)&cfg.buckets[info->type] + info->offset; + *field = value; + } + break; + case DOUBLE: + { + double *field = (void *)&cfg.buckets[info->type] + info->offset; + *field = value; + } + break; + case UNSIGNED: + { + if (value > UINT_MAX) { + error_setg(&local_err, "%s value must be in the" + "range [0, %u]", info->name, UINT_MAX); + goto ret; + } + unsigned *field = (void *)&cfg.buckets[info->type] + info->offset; + *field = value; + } + } + + tg->ts.cfg = cfg;There's a bit of black magic here :)
This offset business is indeed black magic! And university makes you think the world runs on ML and Haskell...
you have a user-defined enumeration
(UNSIGNED, DOUBLE, UINT64) to identify C types and I'm worried about
what happens if the data types of LeakyBucket change, will we be able to
detect the problem?
Out of the blue I can think of the following alternative:
- There's 6 different buckets (we have BucketType listing them)
- There's 3 values we can set in each bucket (max, avg,
burst_length). For those we can have an internal enumeration
(probably with one additional value for iops-size).
- In the 'properties' array, for each property we know its category
(and I mean: avg, max, burst-lenght, iops-size) and the bucket
where they belong.
- In throttle_group_set() the switch could be something like this:
switch (info->category) {
case THROTTLE_VALUE_AVG:
cfg->buckets[info->bkt_type].avg = value;
break;
case THROTTLE_VALUE_MAX:
cfg->buckets[info->bkt_type].max = value;
break;
case THROTTLE_VALUE_BURST_LENGTH:
/* Code here to check that value <= UINT_MAX */
cfg->buckets[info->bkt_type].iops_length = value;
break;
case THROTTLE_VALUE_IOPS_SIZE:
cfg->op_size = value;
}
I don't think that solves the type problem, since C uses coercion. For example if hypothetically a double changes to uint in the future, `value` will not be checked for overflow and the compiler would just convert it to uint implicitly. An academic solution would be to use a strongly typed language!
+static void throttle_group_get(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp)And the same approach here, of course.+static void throttle_group_set_limits(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) + +{ + ThrottleGroup *tg = THROTTLE_GROUP(obj); + ThrottleConfig cfg; + ThrottleLimits *arg = NULL; + Error *local_err = NULL; + + arg = g_new0(ThrottleLimits, 1);Do you need to allocate ThrottleLimits in the heap?
I thought you actually do because visit_type_ThrottleLimits() frees arg on error:
if (err && visit_is_input(v)) {
qapi_free_ThrottleLimits(*obj);
*obj = NULL;
}
but I see now it doesn't actually call g_free and only sets the pointer
to NULL. Is that supposed to prevent use of a stack pointer in the
caller after error? I am a bit confused as to why. (This is the
generated qapi-visit.c code but similar to other visitor functions)
I will change this back to
ThrottleLimits arg = { 0 };
ThrottleLimits *argp = &arg;
visit_type_ThrottleLimits(v, name, &argp, &local_err);
+static void throttle_group_get_limits(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + ThrottleGroup *tg = THROTTLE_GROUP(obj); + ThrottleConfig cfg; + ThrottleLimits *arg = NULL; + + qemu_mutex_lock(&tg->lock); + throttle_get_config(&tg->ts, &cfg); + qemu_mutex_unlock(&tg->lock); + + arg = g_new0(ThrottleLimits, 1); + throttle_config_to_limits(&cfg, arg); + + visit_type_ThrottleLimits(v, name, &arg, errp); + g_free(arg);Same question here. Berto
signature.asc
Description: PGP signature
