Re: [Qemu-block] [PATCH v5 4/6] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

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