[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.
- [Qemu-block] [PATCH v7 1/6] block: move ThrottleGroup membership to ThrottleGroupMember, (continued)
- [Qemu-block] [PATCH v7 1/6] block: move ThrottleGroup membership to ThrottleGroupMember, Manos Pitsidianakis, 2017/08/22
- [Qemu-block] [PATCH v7 2/6] block: add aio_context field in ThrottleGroupMember, Manos Pitsidianakis, 2017/08/22
- [Qemu-block] [PATCH v7 3/6] block: tidy ThrottleGroupMember initializations, Manos Pitsidianakis, 2017/08/22
- [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver, Manos Pitsidianakis, 2017/08/22
- [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM, Manos Pitsidianakis, 2017/08/22
- [Qemu-block] [PATCH v7 6/6] qemu-iotests: add 184 for throttle filter driver, Manos Pitsidianakis, 2017/08/22