Re: [Qemu-devel] [PATCH v8 5/6] fsdev: hmp interface for throttling

[Alberto Garcia]

On Tue 29 Aug 2017 04:23:06 PM CEST, Pradeep Jagadeesh wrote:

> +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg,
> +                                       Error *err)
> +{
> +    monitor_printf(mon, "%s", fscfg->id);
> +    monitor_printf(mon, "    I/O throttling:"
> +                   " bps=%" PRId64
> +                   " bps_rd=%" PRId64  " bps_wr=%" PRId64
> +                   " bps_max=%" PRId64
> +                   " bps_rd_max=%" PRId64
> +                   " bps_wr_max=%" PRId64
> +                   " iops=%" PRId64 " iops_rd=%" PRId64
> +                   " iops_wr=%" PRId64
> +                   " iops_max=%" PRId64
> +                   " iops_rd_max=%" PRId64
> +                   " iops_wr_max=%" PRId64
> +                   " iops_size=%" PRId64
> +                   "\n",
> +                   fscfg->bps,
> +                   fscfg->bps_rd,
> +                   fscfg->bps_wr,
> +                   fscfg->bps_max,
> +                   fscfg->bps_rd_max,
> +                   fscfg->bps_wr_max,
> +                   fscfg->iops,
> +                   fscfg->iops_rd,
> +                   fscfg->iops_wr,
> +                   fscfg->iops_max,
> +                   fscfg->iops_rd_max,
> +                   fscfg->iops_wr_max,
> +                   fscfg->iops_size);
> +   hmp_handle_error(mon, &err);
> +}
> +
> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    IOThrottleList *fsdev_list, *info;
> +    fsdev_list = qmp_query_fsdev_io_throttle(&err);
> +
> +    for (info = fsdev_list; info; info = info->next) {
> +        print_fsdev_throttle_config(mon, info->value, err);
> +    }
> +    qapi_free_IOThrottleList(fsdev_list);
> +}

I don't think what you're doing with the Error here is right:

   - You store the error returned by qmp_query_fsdev_io_throttle().
   - You report the error for _every_ fsdev in the list. That doesn't
     make much sense.
   - Furthermore, if there's an error then fsdev_list should be empty,
     so you're not reporting anything (and you're leaking the error).
   - Even if the list was not empty, hmp_handle_error() frees the error
     after reporting it. Therefore the second item in the list would
     try to report an error that has already been freed.

Berto