Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]