On Wed 09 Aug 2017 04:02:54 PM CEST, Manos Pitsidianakis wrote: > +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error > **errp) > { > + ThrottleGroupMember *tgm; > + > /* this BB is not part of any group */ > - if (!blk->public.throttle_group_member.throttle_state) { > + if (!blk->public.throttle_node) { > return; > } > > + tgm = throttle_get_tgm(blk->public.throttle_node); > /* this BB is a part of the same group than the one we want */ > - if > (!g_strcmp0(throttle_group_get_name(&blk->public.throttle_group_member), > - group)) { > + if (!g_strcmp0(throttle_group_get_name(tgm), > + group)) {
You can join these two lines, no need to have 'group' in its own line. > @@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, > Error **errp) > BlockDriverState *bs; > BlockBackend *blk; > AioContext *aio_context; > + BlockDriverState *throttle_node = NULL; > + ThrottleGroupMember *tgm; > + Error *local_err = NULL; > > blk = qmp_get_blk(arg->has_device ? arg->device : NULL, > arg->has_id ? arg->id : NULL, > @@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, > Error **errp) > if (throttle_enabled(&cfg)) { > /* Enable I/O limits if they're not enabled yet, otherwise > * just update the throttling group. */ > - if (!blk_get_public(blk)->throttle_group_member.throttle_state) { > - blk_io_limits_enable(blk, > - arg->has_group ? arg->group : > - arg->has_device ? arg->device : > - arg->id); > - } else if (arg->has_group) { > - blk_io_limits_update_group(blk, arg->group); > + if (!blk_get_public(blk)->throttle_node) { > + blk_io_limits_enable(blk, arg->has_group ? arg->group : > + arg->has_device ? arg->device : arg->id, > + &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + goto out; > + } > } > - /* Set the new throttling configuration */ > - blk_set_io_limits(blk, &cfg); > - } else if (blk_get_public(blk)->throttle_group_member.throttle_state) { > - /* If all throttling settings are set to 0, disable I/O limits */ > + > + if (arg->has_group) { > + /* move throttle node membership to arg->group */ > + blk_io_limits_update_group(blk, arg->group, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + goto out; > + } > + } This used to be if (!blk_get_public(blk)->throttle_group_member.throttle_state) { /* Enable throttling */ } else if (arg->has_group) { /* Update group */ } but now it is if (!blk_get_throttle_node(blk)) { /* Enable throttling */ } if (arg->has_group) { /* Update group */ } Now if I/O limits are not set then the code sets the same group twice. Everything else looks fine. Berto