On Mon 07 Nov 2016 04:45:55 PM CET, Pradeep Jagadeesh wrote:
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,8 +37,83 @@ static QemuOptsList qemu_fsdev_opts = {
> }, {
> .name = "sock_fd",
> .type = QEMU_OPT_NUMBER,
> + }, {
> + .name = "throttling.iops-total",
> + .type = QEMU_OPT_NUMBER,
> + .help = "limit total I/O operations per second",
> + },{
Not very important, but since this version also needs to be corrected
(as you'll see below) you can update this part to keep the sytle
consisent: all other entries in this array have a space between the
command and the opening brace ("}, {"). Most of your entries don't have
that space ("},{")
> +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error *err)
> +{
> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> + qemu_opt_get_number(opts, "throttling.bps-total", 0);
/* ... */
> + fst->cfg.op_size =
> + qemu_opt_get_number(opts, "throttling.iops-size", 0);
> +
> + if (!throttle_is_valid(&fst->cfg, &err)) {
> + return;
> + }
> +}
That last 'if' is a bit odd :) but I won't object if you want to keep
it. You can simply leave the throttle_is_valid() call and a comment that
explains what it does ("Check that the config is valid and update errp
otherwise").
> +void fsdev_throttle_init(FsThrottle *fst)
> +{
> + if (throttle_enabled(&fst->cfg)) {
> + throttle_init(&fst->ts);
This block is indented with 8 whitespaces, not with 4 as it should be.
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1209,6 +1209,7 @@ static int local_parse_opts(QemuOpts *opts, struct
> FsDriverEntry *fse)
> {
> const char *sec_model = qemu_opt_get(opts, "security_model");
> const char *path = qemu_opt_get(opts, "path");
> + Error *err = NULL;
>
> if (!sec_model) {
> error_report("Security model not specified, local fs needs security
> model");
> @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts, struct
> FsDriverEntry *fse)
> error_report("fsdev: No path specified");
> return -1;
> }
> +
> + fsdev_throttle_parse_opts(opts, &fse->fst, err);
> + if (err) {
> + error_reportf_err(err, "Throttle configuration is not valid: ");
> + return -1;
> + }
> +
This is the important part that needs to be changed: 'err' here is NULL,
so you're passing a NULL pointer to fsdev_throttle_parse_opts() and
invalid throttling configurations are not being reported.
You should pass &err instead, and fsdev_throttle_parse_opts() must
receive Error **errp.
Berto
