qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v5 4/6] block: convert ThrottleGroup to object with QOM
Date: Mon, 21 Aug 2017 14:04:10 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

On Sat 19 Aug 2017 09:08:59 AM CEST, Manos Pitsidianakis wrote:
>>> +    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.

Right, although calling g_strcmp0() may be more expensive than doing all
the pointer comparisons? Anyway it probably doesn't matter much in the
end.

>>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!
>>

Indeed, but I'd rather have this:

   unsigned int avg;
   double value = 2.5;
   avg = value;

than this:

   unsigned int avg;
   double value = 2.5;
   *((double *)&avg) = value;

>>> +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'm not sure why it does that.

Berto



reply via email to

[Prev in Thread] Current Thread [Next in Thread]