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
