On Thu, Aug 10, 2017 at 03:54:02PM +0200, Alberto Garcia wrote:
On Wed 09 Aug 2017 12:07:32 PM CEST, Manos Pitsidianakis wrote:
+/* Extract ThrottleConfig options. Assumes cfg is initialized and will be
+ * checked for validity.
+ *
+ * Returns -1 and sets errp if a burst_length value is over UINT_MAX.
+ */
+static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg,
+                                    Error **errp)
+{
+#define IF_OPT_SET(rvalue, opt_name) \
+    if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX opt_name)) { \
+        rvalue = qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX opt_name, 0); }
+
+    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_TOTAL].avg, QEMU_OPT_BPS_TOTAL);
+    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_READ].avg, QEMU_OPT_BPS_READ);
+    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_WRITE].avg, QEMU_OPT_BPS_WRITE);
+    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_TOTAL].avg, QEMU_OPT_IOPS_TOTAL);
+    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_READ].avg, QEMU_OPT_IOPS_READ);
+    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_WRITE].avg, QEMU_OPT_IOPS_WRITE);
+    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_TOTAL].max, QEMU_OPT_BPS_TOTAL_MAX);
 [...]

This is all the code that I was saying that we'd save if we don't allow
setting limits here.

+static int throttle_configure_tgm(BlockDriverState *bs,
+                                  ThrottleGroupMember *tgm,
+                                  QDict *options, Error **errp)
+{
+    int ret;
+    ThrottleConfig cfg;
+    const char *group_name = NULL;

No need to set it to NULL here.

I know, I do it out of habit!


+    Error *local_err = NULL;
+ QemuOpts *opts = qemu_opts_create(&throttle_opts, NULL, 0, &error_abort);
+
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto err;
+    }
+
+    /* If group_name is NULL, an anonymous group will be created */
+    group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
+
+    /* Register membership to group with name group_name */
+    throttle_group_register_tgm(tgm, group_name, bdrv_get_aio_context(bs));
+
+    /* Copy previous configuration */
+    throttle_group_get_config(tgm, &cfg);
+
+    /* Change limits if user has specified them */
+    if (throttle_extract_options(opts, &cfg, errp) ||
+        !throttle_is_valid(&cfg, errp)) {
+        throttle_group_unregister_tgm(tgm);
+        goto err;
+    }
+    /* Update group configuration */
+    throttle_group_config(tgm, &cfg);

We'd also spare this, and this function would remain much simpler.

+
+    ret = 0;
+    goto fin;
+
+err:
+    ret = -EINVAL;
+fin:
+    qemu_opts_del(opts);
+    return ret;
+}

If you set ret = -EINVAL before calling goto err you can simplify this
part as well, but feel free to ignore this suggestion.

+static int throttle_reopen_prepare(BDRVReopenState *reopen_state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    ThrottleGroupMember *tgm = NULL;
+
+    assert(reopen_state != NULL);
+    assert(reopen_state->bs != NULL);
+
+    reopen_state->opaque = g_new0(ThrottleGroupMember, 1);
+    tgm = reopen_state->opaque;
+
+    return throttle_configure_tgm(reopen_state->bs, tgm, reopen_state->options,
+            errp);
+}

I would rename 'reopen_state' as 'state' for consistency with the other
two functions.

The function signatures in block_int.h have reopen_state, so maybe for consistency I should change the other two to reopen_state as well, instead.


+static void throttle_reopen_commit(BDRVReopenState *state)
+{
+    ThrottleGroupMember *tgm = state->bs->opaque;
+
+    throttle_group_unregister_tgm(tgm);
+    g_free(state->bs->opaque);
+    state->bs->opaque = state->opaque;
+    state->opaque = NULL;
+}

I also find the mixing of state->bs->opaque and tgm a bit confusing.
Here's a suggestion, but feel free to ignore it:

You're right, though it's only a few lines it might require a second read. I will rewrite those more clearly, too.

Attachment: signature.asc
Description: PGP signature

Reply via email to