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
