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

[Cao jin]

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 <address@hidden>
>
> 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