[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 4/6] block: convert ThrottleGroup to object w
From: |
Manos Pitsidianakis |
Subject: |
Re: [Qemu-devel] [PATCH v5 4/6] block: convert ThrottleGroup to object with QOM |
Date: |
Sat, 19 Aug 2017 10:08:59 +0300 |
User-agent: |
NeoMutt/20170609-57-1e93be (1.8.3) |
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