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