On Fri, Jun 23, 2017 at 03:46:55PM +0300, Manos Pitsidianakis wrote: > +static int throttle_configure_tgm(BlockDriverState *bs, ThrottleGroupMember > *tgm, > + QDict *options, Error > **errp) > +{ > + int ret = 0; > + ThrottleState *ts; > + ThrottleTimers *tt; > + ThrottleConfig cfg; > + QemuOpts *opts = NULL; > + const char *group_name = NULL; > + Error *local_err = NULL; > + > + opts = qemu_opts_create(&throttle_opts, NULL, 0, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return -EINVAL; > + } > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + goto err; > + } > + > + group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME); > + if (!group_name) { > + group_name = bdrv_get_device_or_node_name(bs); > + if (!strlen(group_name)) { > + error_setg(&local_err, > + "A group name must be specified for this device.");
To make the error message clearer: s/A group name/A throttle group name/ > +static int throttle_open(BlockDriverState *bs, QDict *options, > + int flags, Error **errp) > +{ > + ThrottleGroupMember *tgm = bs->opaque; > + Error *local_err = NULL; > + > + bs->open_flags = flags; Why is this necessary? The other block drivers don't do it. It should be taken care of in block.c. > + bs->file = bdrv_open_child(NULL, options, "file", > + bs, &child_file, false, &local_err); > + > + if (local_err) { > + error_propagate(errp, local_err); > + return -EINVAL; > + } > + > + qdict_flatten(options); > + return throttle_configure_tgm(bs, tgm, options, errp); Who destroys bs->file on error? > +} > + > +static void throttle_close(BlockDriverState *bs) > +{ > + ThrottleGroupMember *tgm = bs->opaque; > + bdrv_drained_begin(bs); Why is this necessary? bdrv_close() already encloses drv->bdrv_close() in a bdrv_drained_begin/end region. > + throttle_group_unregister_tgm(tgm); > + bdrv_drained_end(bs); > + return; Please omit return at the end of a void function. They are a distraction. > +static void throttle_reopen_commit(BDRVReopenState *state) > +{ > + ThrottleGroupMember *tgm = state->bs->opaque; > + BlockDriverState *bs = state->bs; > + > + bdrv_drained_begin(bs); Why is this necessary? bdrv_reopen_multiple() calls bdrv_reopen_commit() from within a bdrv_drain_all_begin()/end() region. > +static BlockDriver bdrv_throttle = { > + .format_name = "throttle", > + .protocol_name = "throttle", > + .instance_size = sizeof(ThrottleGroupMember), > + > + .bdrv_file_open = throttle_open, > + .bdrv_close = throttle_close, > + .bdrv_co_flush = throttle_co_flush, > + > + .bdrv_child_perm = bdrv_filter_default_perms, > + > + .bdrv_getlength = throttle_getlength, > + > + .bdrv_co_preadv = throttle_co_preadv, > + .bdrv_co_pwritev = throttle_co_pwritev, > + > + .bdrv_co_pwrite_zeroes = throttle_co_pwrite_zeroes, > + .bdrv_co_pdiscard = throttle_co_pdiscard, > + > + .bdrv_recurse_is_first_non_filter = bdrv_recurse_is_first_non_filter, > + > + .bdrv_attach_aio_context = throttle_attach_aio_context, > + .bdrv_detach_aio_context = throttle_detach_aio_context, > + > + .bdrv_reopen_prepare = throttle_reopen_prepare, > + .bdrv_reopen_commit = throttle_reopen_commit, > + .bdrv_reopen_abort = throttle_reopen_abort, > + > + .is_filter = true, > +}; Missing: bdrv_co_get_block_status() bdrv_truncate() bdrv_get_info() bdrv_probe_blocksizes() bdrv_probe_geometry() bdrv_media_changed() bdrv_eject() bdrv_lock_medium() bdrv_co_ioctl() See block/raw-format.c. I think most of these could be modified in block.c or block/io.c to automatically call bs->file's function if drv doesn't implement them. This way all block drivers would transparently pass them through by default and block/raw-format.c code could be eliminated.
signature.asc
Description: PGP signature