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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

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

On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
>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.

Thanks, QOM was difficult to figure out in general.

>> +
>> +
>> +#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?

I assume you mean setting a property while a group has active members 
and requests? My best answer would be "don't do that". I couldn't figure 
a way to do this cleanly. Individual limit changes may render a 
ThrottleConfig invalid, so it should not be allowed. ThrottleGroups and 
throttle nodes should be destroyed and recreated to change limits with 
this version, but in the next it will be done through 
block_set_io_throttle() which is the existing way to change limits and 
check for validity. This was discussed in the proposal about the new 
syntax we had on the list.

As we also talked about on IRC, clock_type should be a ThrottleGroup 
field in order to set a group's configuration without passing a 
ThrottleGroupMember, so changing this will make group configuring 
possible on QMP.
>
>> +
>> +    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? :)

I even left a comment to remind myself to change this... pah.

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

I don't know what should be done about this, to be honest, except for 
manually checking the limits for each datatype in the QOM setters.

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