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.
