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

[Alberto Garcia]

On Mon 04 Sep 2017 02:22:40 PM CEST, Pradeep Jagadeesh wrote:
>>> +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;
> }

More or less, but there's hmp_handle_error() already, so you should use
it:

   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);
       }
       qapi_free_IOThrottleList(fsdev_list);

       hmp_handle_error(mon, &err);
   }

Anyway I just checked that fsdev_get_io_throttle() can never return an
Error, so why don't you remove the Error parameter from there
altogether?

You still need it in qmp_query_fsdev_io_throttle() because that's part
of the API, and hmp_info_fsdev_iothrottle() should have the code to
handle it like the one I just pasted, but fsdev_get_io_throttle() does
not need it, or does it?

Berto