qemu-block
[Top][All Lists]
Advanced

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

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


From: Stefan Hajnoczi
Subject: Re: [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM
Date: Thu, 24 Aug 2017 19:31:10 +0100
User-agent: Mutt/1.8.3 (2017-05-23)

On Tue, Aug 22, 2017 at 01:15:33PM +0300, Manos Pitsidianakis wrote:
>  /* Increments the reference count of a ThrottleGroup given its name.
>   *
>   * If no ThrottleGroup is found with the given name a new one is
>   * created.
>   *
> + * This function edits throttle_groups and must be called under the global
> + * mutex.
> + *
>   * @name: the name of the ThrottleGroup
>   * @ret:  the ThrottleState member of the ThrottleGroup
>   */
>  ThrottleState *throttle_group_incref(const char *name)
>  {
>      ThrottleGroup *tg = NULL;
> -    ThrottleGroup *iter;
> -
> -    qemu_mutex_lock(&throttle_groups_lock);
>  
>      /* Look for an existing group with that name */
> -    QTAILQ_FOREACH(iter, &throttle_groups, list) {
> -        if (!strcmp(name, iter->name)) {
> -            tg = iter;
> -            break;
> -        }
> -    }
> +    tg = throttle_group_by_name(name);
>  
> -    /* Create a new one if not found */
> -    if (!tg) {
> -        tg = g_new0(ThrottleGroup, 1);
> +    if (tg) {
> +        object_ref(OBJECT(tg));
> +    } else {
> +        /* Create a new one if not found */
> +        /* new ThrottleGroup obj will have a refcnt = 1 */
> +        tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
>          tg->name = g_strdup(name);
> -        tg->clock_type = QEMU_CLOCK_REALTIME;
> -
> -        if (qtest_enabled()) {
> -            /* For testing block IO throttling only */
> -            tg->clock_type = QEMU_CLOCK_VIRTUAL;
> -        }
> -        qemu_mutex_init(&tg->lock);
> -        throttle_init(&tg->ts);
> -        QLIST_INIT(&tg->head);
> -
> -        QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);

This is where the ThrottleGroup needs to be added to the QOM tree:

    object_property_add_child(object_get_objects_root(),
                              id, obj, &local_err);

> +        throttle_group_obj_complete((UserCreatable *)tg, &error_abort);

This cast is risky - if UserCreatable ever adds fields it can break.
Please use a QOM cast: USER_CREATABLE(tg).  It uses the type information
and finds the interface.

> +static ThrottleParamInfo properties[] = {

This array can be const.

> +{

Missing indentation.  Seems unusual but I guess you're trying to avoid
reaching 80 chars?  If checkpatch.pl doesn't complain then you can get
away with it but normal indentation would be best.

> +    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;
> +    }

Please use break statements even for the last case.  If another case is
added later it might accidentally fall through without a break
statement.



reply via email to

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