Re: [Qemu-devel] [PATCH v2 1/4] Add Error **errp for xen_host_pci_device_get()

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 01/04/2016 11:15 PM, Stefano Stabellini wrote:
> On Sun, 27 Dec 2015, Cao jin wrote:
>> To catch the error msg. Also modify the caller
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> This looks much better, thanks.
>
>
[...]
>>
>> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> -                            uint8_t bus, uint8_t dev, uint8_t func)
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>> +                            Error **errp)
>>   {
>>       unsigned int v;
>> -    int rc = 0;
>>
>>       d->config_fd = -1;
>>       d->domain = domain;
>> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>       d->dev = dev;
>>       d->func = func;
>>
>> -    rc = xen_host_pci_config_open(d);
>> -    if (rc) {
>> +    xen_host_pci_config_open(d, errp);
>> +    if (*errp) {
>
> I think that errp could be NULL, therefore the right way to do this is:
>
>      Error *err = NULL;
>      foo(arg, &err);
>      if (err) {
>          handle the error...
>          error_propagate(errp, err);
>      }
>
> see the comment at the beginning of include/qapi/error.h.
>

Hi stefano,

     I read that comment, and find something maybe new:

"errp could be NULL", I think it is saying, if we are in a .realize() 
function, yes, *errp* maybe NULL, but reality is, here is the callee of 
.realize(), and we defined a local variable: Error *local_err = NULL in 
.realize() and passed it to all the callee, so, theoretically *errp* 
won`t be NULL. so the way you said above is suitable in .realize() IMHO, 
and I also did it in that way.

comment also says:

  * Receive an error and pass it on to the caller:
  *     Error *err = NULL;
  *     foo(arg, &err);
  *     if (err) {
  *         handle the error...
  *         error_propagate(errp, err);
  *     }
  * where Error **errp is a parameter, by convention the last one.

If I understand the last sentence well, the Error **errp in .realize() 
prototype is *the last one*, so we could call error_propagate(errp, err) 
only in .realize()

The comment also says:

  * But when all you do with the error is pass it on, please use
  *     foo(arg, errp);
  * for readability."

We just pass error on in all the callees, so I guess I also did as 
comment suggest?

How do you think?

[...]
-- 
Yours Sincerely,

Cao Jin