qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to obje


From: Manos Pitsidianakis
Subject: Re: [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM
Date: Mon, 26 Jun 2017 18:24:09 +0300
User-agent: NeoMutt/20170609-57-1e93be (1.8.3)

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]