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

[Pradeep Jagadeesh]

On 8/30/2017 11:38 AM, Alberto Garcia wrote:
> 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.
You mean something like below?

fsdev_list = qmp_query_fsdev_io_throttle(&err);
if (err) {
    error_report_err(err);
    return;
}

Regards,
Pradeep
> Berto