On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote: > +static bool throttle_group_exists(const char *name) > +{ > + ThrottleGroup *iter; > + bool ret = false; > + > + qemu_mutex_lock(&throttle_groups_lock);
Not sure if this lock or the throttle_groups list are necessary. Have you seen iothread.c:qmp_query_iothreads()? All objects are put into a container (the parent object), so you can just iterate over its children. There's no need for a separate list because QOM already has all the objects. > +typedef struct ThrottleGroupClass { > + /* private */ > + ObjectClass parent_class; > + /* public */ > +} ThrottleGroupClass; This is unused? > + > + > +#define DOUBLE 0 > +#define UINT64 1 > +#define UNSIGNED 2 > + > +typedef struct { > + BucketType type; > + int size; /* field size */ sizeof(double) == sizeof(uint64_t) == 8. This is a datatype field, not a size. > +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp) > +{ > + char *name = NULL; > + Error *local_error = NULL; > + ThrottleGroup *tg = THROTTLE_GROUP(obj); > + > + name = object_get_canonical_path_component(OBJECT(obj)); > + if (throttle_group_exists(name)) { > + error_setg(&local_error, "A throttle group with this name already \ > + exists."); > + goto ret; > + } QOM should enforce unique id=<ID>. I don't think this is necessary. > + > + qemu_mutex_lock(&throttle_groups_lock); > + tg->name = name; > + qemu_mutex_init(&tg->lock); > + QLIST_INIT(&tg->head); > + QTAILQ_INSERT_TAIL(&throttle_groups, tg, list); > + tg->refcount++; > + qemu_mutex_unlock(&throttle_groups_lock); > + > +ret: > + error_propagate(errp, local_error); > + return; > + > +} > + > +static void throttle_group_set(Object *obj, Visitor *v, const char * name, > + void *opaque, Error **errp) > + > +{ > + ThrottleGroup *tg = THROTTLE_GROUP(obj); > + ThrottleConfig cfg = tg->ts.cfg; > + Error *local_err = NULL; > + ThrottleParamInfo *info = opaque; > + int64_t value; What happens if this property is set while QEMU is already running? > + > + visit_type_int64(v, name, &value, &local_err); > + > + if (local_err) { > + goto out; > + } > + if (value < 0) { > + error_setg(&local_err, "%s value must be in range [0, %"PRId64"]", > + "iops-total", INT64_MAX); /* change option string */ iops-total? :) > + goto out; > + } This doesn't validate inputs properly for non int64_t types. I'm also worried that the command-line bps=,iops=,... options do not have unsigned or double types. Input ranges and validation should match the QEMU command-line (I know this is a bit of a pain with QOM since the property types are different from QEMU option types).
signature.asc
Description: PGP signature