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

[Alberto Garcia]

On Tue 05 Sep 2017 10:28:02 AM 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);
>>> +    }
>>> +    qapi_free_IOThrottleList(fsdev_list);
>>> +}
>>
>> You're passing an Error to qmp_query_fsdev_io_throttle() but then you
>> don't handle it. Use hmp_handle_error() as I said in my previous e-mail.
> OK I will handle it.
>>
>> I know that with the current code qmp_query_fsdev_io_throttle() is never
>> going to fail, but that's no reason to declare an Error and then ignore
>> it.

> I need to pass err because the function
> qmp_query_fsdev_io_throttle(Error *) is created by the scripts. So, I
> have to declare and pass it. I do not is there any way to avoid this.

When a function takes an Error ** like qmp_query_fsdev_io_throttle() and
many others do, there are two alternatives:

a) Declare an Error, pass it _and then handle it_ (if you don't handle
   it, you're leaking it):

   Error *err = NULL;
   some_function(&err);
   if (err) {
      /* handle err */
   }

b) Don't declare the Error and pass NULL instead:

   some_function(NULL);

Note that (b) is perfectly valid, but if the function you're calling
tries to set an Error then you won't get it. It's ok if you know what
you're doing (which usually means: you have other way to determine if
the function failed, _or_ you don't care if the function failed).

Here you have no other way to know if qmp_query_fsdev_io_throttle()
fails, so you should choose (a).

I know that at the moment qmp_query_fsdev_io_throttle() can never fail
so in practice this doesn't matter much _now_, but if in the future that
function can fail then your code should be ready to handle it.

Berto